[PATCH] Bug 9907: Master-Child Closing Crash

[Richard Heck]

This is a repost now that the lists work again.

=

See http://www.lyx.org/trac/ticket/9907.

Here's a proper patch intended to fix this bug. It actually does a lot
less than it appears to do. The old BufferList::releaseChild(parent,
child) routine did a few different things: First, it checked whether the
child in question was open as a child of some other parent; if not, it
released it; if so, it set its parent to 0. The new patch replaces that
routine with BufferList::isOthersChild, which only does the first of
these things. The other tasks are performed by the caller, depending
upon the return value.

So the change made in Buffer.cpp (should!) actually make no difference
at all.

The advantage is that we can now check, in GuiView::closeBuffer, whether
the child is open as a child of some other Buffer before trying to close it.

Richard


>From d749c635dce01418b8623710d4287d1c9fd62cb3 Mon Sep 17 00:00:00 2001
From: Richard Heck 
Date: Sun, 20 Dec 2015 11:50:18 -0500
Subject: [PATCH] Fix bug 9907. The problem was that, when closing one Buffer,
 we were closing all of its children, even if they were open as children of
 some other Buffer.

---
 src/Buffer.cpp|  8 ++--
 src/BufferList.cpp| 47 ---
 src/BufferList.h  |  9 +
 src/frontends/qt4/GuiView.cpp | 26 +++-
 4 files changed, 50 insertions(+), 40 deletions(-)

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index eac9821..115a542 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -517,8 +517,12 @@ Buffer::~Buffer()
 		Impl::BufferPositionMap::iterator end = d->children_positions.end();
 		for (; it != end; ++it) {
 			Buffer * child = const_cast(it->first);
-			if (theBufferList().isLoaded(child))
-theBufferList().releaseChild(this, child);
+			if (theBufferList().isLoaded(child)) { 
+			 if (theBufferList().isOthersChild(this, child))
+ child->setParent(0);
+			 else
+theBufferList().release(child);
+			}
 		}
 
 		if (!isClean()) {
diff --git a/src/BufferList.cpp b/src/BufferList.cpp
index 68a1e80..ff99a09 100644
--- a/src/BufferList.cpp
+++ b/src/BufferList.cpp
@@ -267,6 +267,28 @@ bool BufferList::exists(FileName const & fname) const
 }
 
 
+bool BufferList::isOthersChild(Buffer * parent, Buffer * child)
+{
+	LASSERT(parent, return false);
+	LASSERT(child, return false);
+	LASSERT(parent->isChild(child), return false);
+	
+	// Does child document have a different parent?
+	Buffer const * parent_ = child->parent();
+	if (parent_ && parent_ != parent)
+		return true;
+	
+	BufferStorage::iterator it = bstore.begin();
+	BufferStorage::iterator end = bstore.end();
+	for (; it != end; ++it) {
+		Buffer * buf = *it;
+		if (buf != parent && buf->isChild(child))
+			return true;
+	}
+	return false;
+}
+
+
 namespace {
 
 struct equivalent_to : public binary_function
@@ -364,31 +386,6 @@ int BufferList::bufferNum(FileName const & fname) const
 }
 
 
-bool BufferList::releaseChild(Buffer * parent, Buffer * child)
-{
-	LASSERT(parent, return false);
-	LASSERT(child, return false);
-	LASSERT(parent->isChild(child), return false);
-
-	// Child document has a different parent, don't close it.
-	Buffer const * parent_ = child->parent();
-	if (parent_ && parent_ != parent)
-		return false;
-
-	BufferStorage::iterator it = bstore.begin();
-	BufferStorage::iterator end = bstore.end();
-	for (; it != end; ++it) {
-		Buffer * buf = *it;
-		if (buf != parent && buf->isChild(child)) {
-			child->setParent(0);
-			return false;
-		}
-	}
-	release(child);
-	return true;
-}
-
-
 void BufferList::changed(bool update_metrics) const
 {
 	BufferStorage::const_iterator it = bstore.begin();
diff --git a/src/BufferList.h b/src/BufferList.h
index 242eff0..690bf6a 100644
--- a/src/BufferList.h
+++ b/src/BufferList.h
@@ -55,13 +55,14 @@ public:
 	/// \return 0 if the Buffer creation is not possible for whatever reason.
 	Buffer * newInternalBuffer(std::string const & s);
 
+	/// Is child a child of some Buffer other than parent?
+	/// NOTE: child must be a child of parent, and both must be non-null.
+	/// Otherwise we assert.
+	bool isOthersChild(Buffer * parent, Buffer * child);
+
 	/// delete a buffer
 	void release(Buffer * b);
 
-	/// Release \p child if it really is a child and is not used elsewhere.
-	/// \return true is the file was closed.
-	bool releaseChild(Buffer * parent, Buffer * child);
-
 	/// Close all open buffers.
 	void closeAll();
 
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 548381b..c2101f3 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -2849,23 +2849,31 @@ bool GuiView::closeBuffer(Buffer & buf)
 		ListOfBuffers::const_iterator it = clist.begin();
 		ListOfBuffers::const_iterator const bend = clist.end();
 		for (; it != bend; ++it) {
-			// If a child is dirty, do not close
-			// without user intervention
-			//FIXME: 

[PATCH] Bug 9907: Master-Child Closing Crash

[Richard Heck]

See http://www.lyx.org/trac/ticket/9907.

Here's a proper patch intended to fix this bug. It actually does a lot
less than it appears to do. The old BufferList::releaseChild(parent,
child) routine did a few different things: First, it checked whether the
child in question was open as a child of some other parent; if not, it
released it; if so, it set its parent to 0. The new patch replaces that
routine with BufferList::isOthersChild, which only does the first of
these things. The other tasks are performed by the caller, depending
upon the return value.

So the change made in Buffer.cpp (should!) actually make no difference
at all.

The advantage is that we can now check, in GuiView::closeBuffer, whether
the child is open as a child of some other Buffer before trying to close it.

Richard

>From d749c635dce01418b8623710d4287d1c9fd62cb3 Mon Sep 17 00:00:00 2001
From: Richard Heck 
Date: Sun, 20 Dec 2015 11:50:18 -0500
Subject: [PATCH] Fix bug 9907. The problem was that, when closing one Buffer,
 we were closing all of its children, even if they were open as children of
 some other Buffer.

---
 src/Buffer.cpp|  8 ++--
 src/BufferList.cpp| 47 ---
 src/BufferList.h  |  9 +
 src/frontends/qt4/GuiView.cpp | 26 +++-
 4 files changed, 50 insertions(+), 40 deletions(-)

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index eac9821..115a542 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -517,8 +517,12 @@ Buffer::~Buffer()
 		Impl::BufferPositionMap::iterator end = d->children_positions.end();
 		for (; it != end; ++it) {
 			Buffer * child = const_cast(it->first);
-			if (theBufferList().isLoaded(child))
-theBufferList().releaseChild(this, child);
+			if (theBufferList().isLoaded(child)) { 
+			 if (theBufferList().isOthersChild(this, child))
+ child->setParent(0);
+			 else
+theBufferList().release(child);
+			}
 		}
 
 		if (!isClean()) {
diff --git a/src/BufferList.cpp b/src/BufferList.cpp
index 68a1e80..ff99a09 100644
--- a/src/BufferList.cpp
+++ b/src/BufferList.cpp
@@ -267,6 +267,28 @@ bool BufferList::exists(FileName const & fname) const
 }
 
 
+bool BufferList::isOthersChild(Buffer * parent, Buffer * child)
+{
+	LASSERT(parent, return false);
+	LASSERT(child, return false);
+	LASSERT(parent->isChild(child), return false);
+	
+	// Does child document have a different parent?
+	Buffer const * parent_ = child->parent();
+	if (parent_ && parent_ != parent)
+		return true;
+	
+	BufferStorage::iterator it = bstore.begin();
+	BufferStorage::iterator end = bstore.end();
+	for (; it != end; ++it) {
+		Buffer * buf = *it;
+		if (buf != parent && buf->isChild(child))
+			return true;
+	}
+	return false;
+}
+
+
 namespace {
 
 struct equivalent_to : public binary_function
@@ -364,31 +386,6 @@ int BufferList::bufferNum(FileName const & fname) const
 }
 
 
-bool BufferList::releaseChild(Buffer * parent, Buffer * child)
-{
-	LASSERT(parent, return false);
-	LASSERT(child, return false);
-	LASSERT(parent->isChild(child), return false);
-
-	// Child document has a different parent, don't close it.
-	Buffer const * parent_ = child->parent();
-	if (parent_ && parent_ != parent)
-		return false;
-
-	BufferStorage::iterator it = bstore.begin();
-	BufferStorage::iterator end = bstore.end();
-	for (; it != end; ++it) {
-		Buffer * buf = *it;
-		if (buf != parent && buf->isChild(child)) {
-			child->setParent(0);
-			return false;
-		}
-	}
-	release(child);
-	return true;
-}
-
-
 void BufferList::changed(bool update_metrics) const
 {
 	BufferStorage::const_iterator it = bstore.begin();
diff --git a/src/BufferList.h b/src/BufferList.h
index 242eff0..690bf6a 100644
--- a/src/BufferList.h
+++ b/src/BufferList.h
@@ -55,13 +55,14 @@ public:
 	/// \return 0 if the Buffer creation is not possible for whatever reason.
 	Buffer * newInternalBuffer(std::string const & s);
 
+	/// Is child a child of some Buffer other than parent?
+	/// NOTE: child must be a child of parent, and both must be non-null.
+	/// Otherwise we assert.
+	bool isOthersChild(Buffer * parent, Buffer * child);
+
 	/// delete a buffer
 	void release(Buffer * b);
 
-	/// Release \p child if it really is a child and is not used elsewhere.
-	/// \return true is the file was closed.
-	bool releaseChild(Buffer * parent, Buffer * child);
-
 	/// Close all open buffers.
 	void closeAll();
 
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 548381b..c2101f3 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -2849,23 +2849,31 @@ bool GuiView::closeBuffer(Buffer & buf)
 		ListOfBuffers::const_iterator it = clist.begin();
 		ListOfBuffers::const_iterator const bend = clist.end();
 		for (; it != bend; ++it) {
-			// If a child is dirty, do not close
-			// without user intervention
-			//FIXME: should we look in other tabworkareas?
 			Buffer * child_buf