Pavel reported a regression crash in branch in this thread:
http://marc.info/?t=123552919800002&r=1&w=2&n=21
The problem turned out to be multiple deletions of cloned child buffers.
In this commit:
http://www.lyx.org/trac/changeset/40205
I tried to fix the crash by having something kind of like a BufferList
to keep track of the cloned buffers and I then added an assertion in
this commit:
http://www.lyx.org/trac/changeset/40208
to catch attempts to reuse the cloned_buffer_list_, as the code assumed
we'd have only one set of clones at a time.
Unfortunately, Kornel reported here:
http://marc.info/?l=lyx-devel&m=132172531628079&w=2
that the assertion was being triggered at autosave, and the problem
turned out to be that, while we do make sure we only do one export at a
time, more than one autosave thread can be active at a time.
So, what to do? I see a couple options for branch:
1) Another version of the patch, which is here:
http://marc.info/?l=lyx-devel&m=132148645632711&w=2
keeps the cloned_bufer_list in the master buffer of the cloned document
set, so there's no danger of a conflict. I worried, however, that, since
this is being accessed during the destruction process, we might run into
a crash due to deletion of the master before the child. The code is not
structured that way, so maybe it's not a serious worry, so perhaps this
is a decent option.
2) The assertion that's being triggered tests to make sure
cloned_buffer_list is empty, which is also a kind of check that all the
clones have been deleted. But this could simply be eliminated:
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index d01ff7b..a9743a6 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -450,7 +450,6 @@ Buffer * Buffer::clone() const
Buffer * cloned_buffer = bit->second;
// record the list of cloned buffers
- cloned_buffer_list.clear();
BufferMap::iterator it = bufmap.begin();
BufferMap::iterator en = bufmap.end();
for (; it != en; ++it)
There is, in principle, a kind of race condition that could happen here.
Suppose the same Buffer is included twice in some master. We delete it,
and delete it from this list, but then autosave triggers again, and a
new Buffer with the same memory location as the deleted child is
created. Now when we try to delete the second child, it will seem to be
there, and we will actually delete the unrelated Buffer from the second
autosave. This seems extremely unlikely, but it could happen.
3) As Vincent essentially noted, it's anyway wrong to clone all the
children, etc, when we are just doing autosave. The attached patch
addresses that issue. So with it, we won't see the assertion that Kornel
reported. My worry here is that this patch seems intrusive, and it's
kind of late in the day to commit it to 2.0.2. Longer term, though, we
definitely want this patch.
Overall, then, (1) looks safest for 2.0.2, so long as my concerns about
it are misplaced. If not, then I guess we should go with (2), for 2.0.2,
since the crash I described is extremely unlikely ever to happen. For
trunk, we should certainly do (3) and commit the same patch to 2.0.3svn
as soon as it is open. That would allow for the option of going back to
the global variable as we have it now, for the reasons mentioned.
Comments?
Richard
>From c8d9378ab5efc650e5e7d1fcc1a098ecf08e9c75 Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Tue, 15 Nov 2011 11:08:50 -0500
Subject: [PATCH] Don't clone all the children on autosave.
---
src/Buffer.cpp | 16 ++++++++++++----
src/Buffer.h | 9 ++++++---
src/frontends/qt4/GuiView.cpp | 4 ++--
3 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 0badaae..123ef35 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -448,10 +448,10 @@ Buffer::~Buffer()
}
-Buffer * Buffer::clone() const
+Buffer * Buffer::cloneFromMaster() const
{
BufferMap bufmap;
- masterBuffer()->clone(bufmap);
+ masterBuffer()->cloneWithChildren(bufmap);
// make sure we got cloned
BufferMap::const_iterator bit = bufmap.find(this);
@@ -472,7 +472,7 @@ Buffer * Buffer::clone() const
}
-void Buffer::clone(BufferMap & bufmap) const
+void Buffer::cloneWithChildren(BufferMap & bufmap) const
{
// have we already been cloned?
if (bufmap.find(this) != bufmap.end())
@@ -493,7 +493,7 @@ void Buffer::clone(BufferMap & bufmap) const
dit.setBuffer(buffer_clone);
Buffer * child = const_cast<Buffer *>(it->second.second);
- child->clone(bufmap);
+ child->cloneWithChildren(bufmap);
BufferMap::iterator const bit = bufmap.find(child);
LASSERT(bit != bufmap.end(), continue);
Buffer * child_clone = bit->second;
@@ -511,6 +511,14 @@ void Buffer::clone(BufferMap & bufmap) const
}
+Buffer * Buffer::cloneBufferOnly() const {
+ Buffer * buffer_clone = new Buffer(fileName().absFileName(), false, this);
+ // we won't be cloning the children
+ buffer_clone->d->children_positions.clear();
+ return buffer_clone;
+}
+
+
bool Buffer::isClone() const
{
return d->cloned_buffer_;
diff --git a/src/Buffer.h b/src/Buffer.h
index bf2a9ea..463ff66 100644
--- a/src/Buffer.h
+++ b/src/Buffer.h
@@ -166,8 +166,11 @@ public:
/// Destructor
~Buffer();
- ///
- Buffer * clone() const;
+ /// Clones the entire structure of which this Buffer is part, starting
+ /// with the master and cloning all the children, too.
+ Buffer * cloneFromMaster() const;
+ /// Just clones this single Buffer. For autosave.
+ Buffer * cloneBufferOnly() const;
///
bool isClone() const;
@@ -230,7 +233,7 @@ private:
///
typedef std::map<Buffer const *, Buffer *> BufferMap;
///
- void clone(BufferMap &) const;
+ void cloneWithChildren(BufferMap &) const;
/// save timestamp and checksum of the given file.
void saveCheckSum() const;
/// read a new file
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 2409823..48bbc05 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -1575,7 +1575,7 @@ void GuiView::autoSave()
#if (QT_VERSION >= 0x040400)
GuiViewPrivate::busyBuffers.insert(buffer);
QFuture<docstring> f = QtConcurrent::run(GuiViewPrivate::autosaveAndDestroy,
- buffer, buffer->clone());
+ buffer, buffer->cloneBufferOnly());
d.autosave_watcher_.setFuture(f);
#else
buffer->autoSave();
@@ -3122,7 +3122,7 @@ bool GuiView::GuiViewPrivate::asyncBufferProcessing(
QFuture<Buffer::ExportStatus> f = QtConcurrent::run(
asyncFunc,
used_buffer,
- used_buffer->clone(),
+ used_buffer->cloneFromMaster(),
format);
setPreviewFuture(f);
last_export_format = used_buffer->params().bufferFormat();
--
1.7.4.4
