On 11/14/2011 11:27 PM, Vincent van Ravesteijn wrote:
>
>
> Op 15 nov. 2011 03:35 schreef "Richard Heck" <rgh...@comcast.net
> <mailto:rgh...@comcast.net>> het volgende:
> >
> > On 11/14/2011 04:36 PM, Pavel Sanda wrote:
> >>
> >> Richard Heck wrote:
> >>>
> >>> Something like that may be happening. Pavel, can you produce a
> minimal set
> >>> just replicating the structure?
> >>
> >> confirming that the cause is r39599.
> >>
> >> the structure causing warning is:
> >> parent->child1
> >> parent->child2->child1
> >>
> > Weird. I'll have a look, obviously before 2.0.2. I guess we knew
> these double parent issues could cause problems....
> >
> > rh
>
> Yes, thats why I added the warning. I think I guarded for this as well.
>
> Do we clone all children when running asynchronous autosave? We shouldnt.
>
I think we probably do, because the cloning happens the same way as with
export. What I added was a more complex cloning mechanism to fix
various other bugs. But we have always cloned the children in
Buffer::clone(). What I changed was how we do so. In particular, we
always clone the whole buffer structure: If we are cloning a child, we
still start by cloning the master buffer, since lots of things need
access to that.
The crash happens, though, with export, too, so it's not specific to
autosave. What's actually causing it, I think, is the fact that we only
clone each child once. Before, each child got cloned as many times as it
appeared in the structure. Unfortunately, I didn't account properly for
the sort of structure Pavel has.
The attached patch seems to fix the crash, by keeping a list of the
cloned Buffers in the master of the cloned set that we can then check as
we go to make sure we don't double delete. So it's doing the same sort
of work as the BufferList in the other case. Another option would be to
have some global list here. Comments welcome.
Richard
>From 4a1533560bb4a900f7352dd2bdb7f345ab7336b4 Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Tue, 15 Nov 2011 10:52:41 -0500
Subject: [PATCH] Fix crash reported by Pavel. We need some structure that
will do the work the BufferList does in the non-cloned
deletion case.
---
src/Buffer.cpp | 29 ++++++++++++++++++++++++-----
src/Buffer.h | 2 ++
2 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 625c0ff..3242683 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -400,9 +400,12 @@ Buffer::~Buffer()
Impl::BufferPositionMap::iterator end = d->children_positions.end();
for (; it != end; ++it) {
Buffer * child = const_cast<Buffer *>(it->first);
- if (d->cloned_buffer_)
- delete child;
// The child buffer might have been closed already.
+ if (d->cloned_buffer_) {
+ Buffer const * master = masterBuffer();
+ if (master->clone_list_.erase(child))
+ delete child;
+ }
else if (theBufferList().isLoaded(child))
theBufferList().releaseChild(this, child);
}
@@ -434,9 +437,25 @@ Buffer * Buffer::clone() const
{
BufferMap bufmap;
masterBuffer()->clone(bufmap);
- BufferMap::iterator it = bufmap.find(this);
- LASSERT(it != bufmap.end(), return 0);
- return it->second;
+
+ // make sure we got cloned
+ BufferMap::const_iterator bit = bufmap.find(this);
+ LASSERT(bit != bufmap.end(), return 0);
+ Buffer * cloned_buffer = bit->second;
+
+ // record the list of cloned buffers in the cloned master
+ bit = bufmap.find(masterBuffer());
+ LASSERT(bit != bufmap.end(), return 0);
+ Buffer * cloned_master = bit->second;
+
+ set<Buffer *> & clist = cloned_master->clone_list_;
+ clist.clear();
+ BufferMap::iterator it = bufmap.begin();
+ BufferMap::iterator en = bufmap.end();
+ for (; it != en; ++it)
+ clist.insert(it->second);
+
+ return cloned_buffer;
}
diff --git a/src/Buffer.h b/src/Buffer.h
index bf2a9ea..d6f82ef 100644
--- a/src/Buffer.h
+++ b/src/Buffer.h
@@ -707,6 +707,8 @@ private:
Buffer(Buffer const &);
void operator=(Buffer const &);
+ /// a list of Buffers we cloned
+ mutable std::set<Buffer *> clone_list_;
/// Use the Pimpl idiom to hide the internals.
class Impl;
/// The pointer never changes although *pimpl_'s contents may.
--
1.7.4.4
