Another option is attached. Now, each clone set has its own list of
cloned buffers, and each clone has a pointer to this list, so there's no
need to find the master buffer, etc. This should solve Kornel's crash,
and it has the advantage that it would allow us, in principle, to have
multiple export threads, too, if at some point we wanted to do that.
The only downside here is that, over time, we will end up with lots of
empty members of the cloned_buffers list, but presumably these take up
very little memory. The second patch would address that problem, but
maybe we should do this only in trunk for now, or add it to 2.0.3svn
when that is open. It seems a bit more dangerous.
Richard
>From 8f7e5ca2ec730378e79a78681af837966c0b7901 Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Sun, 20 Nov 2011 12:09:56 -0500
Subject: [PATCH 1/2] New attempt to fix Pavel's crash. How we keep more than
one of these lists at the same time, which would make
multiple export possible, and just keep a pointer to
them.
---
src/Buffer.cpp | 35 ++++++++++++++++-------------------
src/Buffer.h | 4 +++-
2 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 0badaae..8abdcc8 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -139,11 +139,11 @@ void showPrintError(string const & name)
Alert::error(_("Print document failed"), str);
}
-/// a list of Buffers we cloned
-set<Buffer *> cloned_buffer_list;
+} // namespace anon
-} // namespace anon
+// A storehouse for the cloned buffers.
+list<CloneList> cloned_buffers;
class Buffer::Impl
@@ -305,6 +305,8 @@ public:
/// If non zero, this buffer is a clone of existing buffer \p cloned_buffer_
/// This one is useful for preview detached in a thread.
Buffer const * cloned_buffer_;
+ ///
+ CloneList * clone_list_;
/// are we in the process of exporting this buffer?
mutable bool doing_export;
@@ -341,7 +343,7 @@ Buffer::Impl::Impl(Buffer * owner, FileName const & file, bool readonly_,
toc_backend(owner), macro_lock(false), timestamp_(0), checksum_(0),
wa_(0), gui_(0), undo_(*owner), bibinfo_cache_valid_(false),
bibfile_cache_valid_(false), cite_labels_valid_(false),
- preview_loader_(0), cloned_buffer_(cloned_buffer),
+ preview_loader_(0), cloned_buffer_(cloned_buffer), clone_list_(0),
doing_export(false), parent_buffer(0)
{
if (!cloned_buffer_) {
@@ -402,13 +404,13 @@ Buffer::~Buffer()
if (isClone()) {
// this is in case of recursive includes: we won't try to delete
// ourselves as a child.
- cloned_buffer_list.erase(this);
+ d->clone_list_->erase(this);
// loop over children
Impl::BufferPositionMap::iterator it = d->children_positions.begin();
Impl::BufferPositionMap::iterator end = d->children_positions.end();
for (; it != end; ++it) {
Buffer * child = const_cast<Buffer *>(it->first);
- if (cloned_buffer_list.erase(child))
+ if (d->clone_list_->erase(child))
delete child;
}
// FIXME Do we really need to do this right before we delete d?
@@ -451,28 +453,21 @@ Buffer::~Buffer()
Buffer * Buffer::clone() const
{
BufferMap bufmap;
- masterBuffer()->clone(bufmap);
+ cloned_buffers.push_back(CloneList());
+ CloneList * clones = &cloned_buffers.back();
+
+ masterBuffer()->clone(bufmap, clones);
// make sure we got cloned
BufferMap::const_iterator bit = bufmap.find(this);
LASSERT(bit != bufmap.end(), return 0);
Buffer * cloned_buffer = bit->second;
- // this should be empty. if not, then either we have left
- // some buffer undeleted, or else we are trying to export
- // two buffers at once. either way is a problem.
- LASSERT(cloned_buffer_list.empty(), return 0);
- // record the list of cloned buffers in the cloned master
- BufferMap::iterator it = bufmap.begin();
- BufferMap::iterator en = bufmap.end();
- for (; it != en; ++it)
- cloned_buffer_list.insert(it->second);
-
return cloned_buffer;
}
-void Buffer::clone(BufferMap & bufmap) const
+void Buffer::clone(BufferMap & bufmap, CloneList * clones) const
{
// have we already been cloned?
if (bufmap.find(this) != bufmap.end())
@@ -480,6 +475,8 @@ void Buffer::clone(BufferMap & bufmap) const
Buffer * buffer_clone = new Buffer(fileName().absFileName(), false, this);
bufmap[this] = buffer_clone;
+ clones->insert(buffer_clone);
+ buffer_clone->d->clone_list_ = clones;
buffer_clone->d->macro_lock = true;
buffer_clone->d->children_positions.clear();
// FIXME (Abdel 09/01/2010): this is too complicated. The whole children_positions and
@@ -493,7 +490,7 @@ void Buffer::clone(BufferMap & bufmap) const
dit.setBuffer(buffer_clone);
Buffer * child = const_cast<Buffer *>(it->second.second);
- child->clone(bufmap);
+ child->clone(bufmap, clones);
BufferMap::iterator const bit = bufmap.find(child);
LASSERT(bit != bufmap.end(), continue);
Buffer * child_clone = bit->second;
diff --git a/src/Buffer.h b/src/Buffer.h
index bf2a9ea..0c15c0e 100644
--- a/src/Buffer.h
+++ b/src/Buffer.h
@@ -79,6 +79,8 @@ class PreviewLoader;
class Buffer;
typedef std::list<Buffer *> ListOfBuffers;
+/// a list of Buffers we cloned
+typedef std::set<Buffer *> CloneList;
/** The buffer object.
@@ -230,7 +232,7 @@ private:
///
typedef std::map<Buffer const *, Buffer *> BufferMap;
///
- void clone(BufferMap &) const;
+ void clone(BufferMap &, CloneList *) const;
/// save timestamp and checksum of the given file.
void saveCheckSum() const;
/// read a new file
--
1.7.4.4
>From 08fa1e97400ab5126415d70affea1fdeefbfbc30 Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Sun, 20 Nov 2011 12:29:32 -0500
Subject: [PATCH 2/2] Add assertion to check for memory leaks and remove the
list when we are done.
---
src/Buffer.cpp | 18 +++++++++++++++---
1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 8abdcc8..cd3dcb8 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -143,7 +143,7 @@ void showPrintError(string const & name)
// A storehouse for the cloned buffers.
-list<CloneList> cloned_buffers;
+list<CloneList *> cloned_buffers;
class Buffer::Impl
@@ -413,6 +413,18 @@ Buffer::~Buffer()
if (d->clone_list_->erase(child))
delete child;
}
+ // if we're the master buffer, then we should get rid of the list
+ // of clones
+ if (!parent()) {
+ // if this is not empty, we have leaked something. worse, one of the
+ // children still has a reference to this list.
+ LASSERT(d->clone_list_->empty(), /* */);
+ list<CloneList *>::iterator it =
+ find(cloned_buffers.begin(), cloned_buffers.end(), d->clone_list_);
+ LASSERT(it != cloned_buffers.end(), /* */);
+ cloned_buffers.erase(it);
+ delete d->clone_list_;
+ }
// FIXME Do we really need to do this right before we delete d?
// clear references to children in macro tables
d->children_positions.clear();
@@ -453,8 +465,8 @@ Buffer::~Buffer()
Buffer * Buffer::clone() const
{
BufferMap bufmap;
- cloned_buffers.push_back(CloneList());
- CloneList * clones = &cloned_buffers.back();
+ cloned_buffers.push_back(new CloneList());
+ CloneList * clones = cloned_buffers.back();
masterBuffer()->clone(bufmap, clones);
--
1.7.4.4
