On 12/20/2015 11:48 AM, Richard Heck wrote:
> On 12/20/2015 09:51 AM, Richard Heck wrote:
>> On 12/20/2015 09:42 AM, Guillaume Munch wrote:
>>> Yes, all this parent/child relationship seems quite complicated and ad
>>> hoc. While testing what would happen when a document has two parents, I
>>> found the following crash: <http://www.lyx.org/trac/ticket/9907>.
>> The multi-parent problem has caused crashes before. I'll have a look at
>> this one.
>
> Recipe from bug:
>
> 1. open master1.lyx and master2.lyx at the same time (which both have
> child1.lyx as a child)
> 2. make dummy modifications to master1.lyx, master2.lyx and child1.lyx
> 3. close master2.lyx, don't save
> 4. close master1.lyx, enjoy SIGSEGV
>
> So the immediate problem is that the child gets closed with master2.
> When we go to close master1, it isn't there, and we get a SIGSEV.
>
> This is not unlike bug #6603.
>
> Consider a different case: Where we just switch between tabs. In this
> case, we get a focusInEvent and, as a result, run updateBuffer, which
> eventually runs InsetInclude::loadIfNeeded. This guarantees that the
> child is there. When we return to master1 after closing master2,
> however, we do not get that signal, and we do not run updateBuffer.
>
> This happens only if we close master2 using the X on the tab. If you
> close it using the menu, then updateBuffer gets run. The reason for
> the difference is that closing using the X does not activate the
> dispatch machinery.
>
> This also only happens if we are currently editing master1 when we
> close master2. If we are editing master2 itself, then we get the
> focusInEvent when we go to master1. But if we were already editing
> master1, then we do not get a focusInEvent.
>
> We also only get the crash if the parent of the child is currently set
> as master2. If it's instead master1, then the child does not get
> closed but only hidden.
>
> So, under these specific circumstances, updateBuffer never gets run
> when we return to master1, so the child is never re-loaded.
>
> One solution is to figure out somewhere updateBuffer can be run.
>
> Another solution would be to check somewhere whether this particular
> child is also the child of some other buffer before closing it. The
> comment before GuiView::closeWorkArea says:
>
> // We only want to close the buffer if it is not visible in other
> workareas
> // of the same view, nor in other views, and if this is not a child
>
> Unfourtunately, this never gets checked for children that are getting
> closed along with the master.
The attached patch is really proof of concept. There are some not so
nice things about it, namely, that it asks whether to save the child,
which it doesn't need to do, since the child will stay open. Probably
the right thing to do is factor out some "Am I a child of some other
buffer?" routine from the current BufferList::releaseChild method and
use that somewhere.
> Likely other bug: When we switch from master1 to master2, we do NOT
> reset the parent of the child. It seems to me we should do so,
> probably in InsetInclude::loadIfNeeded.
It seems we do this when we have to reload the child, but not when it is
already loaded. It is easy to do this, as in the attached. The downside
to doing it this way is that parent of a child that is included multiple
times will now be set to whatever the parent is the last time it is
included. I don't expect this is a problem, but it could change some
behavior somewhere. Currently, the parent will be set to whatever it is
the first time the file is included. Multiple inclusions aren't handled
all that cleanly anyway, though.
Richard
>From 9edf06bed497c09e18c2d035ad4d697344ce0dfd Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Sun, 20 Dec 2015 11:50:18 -0500
Subject: [PATCH] Basic idea for fixing child crash.
---
src/BufferList.cpp | 4 ++--
src/frontends/qt4/GuiView.cpp | 13 ++++++++-----
src/frontends/qt4/GuiView.h | 4 ++--
3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/BufferList.cpp b/src/BufferList.cpp
index 68a1e80..ec6a9a1 100644
--- a/src/BufferList.cpp
+++ b/src/BufferList.cpp
@@ -366,9 +366,9 @@ int BufferList::bufferNum(FileName const & fname) const
bool BufferList::releaseChild(Buffer * parent, Buffer * child)
{
- LASSERT(parent, return false);
+ // LASSERT(parent, return false);
LASSERT(child, return false);
- LASSERT(parent->isChild(child), return false);
+ LASSERT(!parent || parent->isChild(child), return false);
// Child document has a different parent, don't close it.
Buffer const * parent_ = child->parent();
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 548381b..f80aae5 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -2812,7 +2812,7 @@ bool GuiView::closeWorkAreaAll()
}
-bool GuiView::closeWorkArea(GuiWorkArea * wa, bool close_buffer)
+bool GuiView::closeWorkArea(GuiWorkArea * wa, bool close_buffer, bool as_child)
{
if (!wa)
return false;
@@ -2826,7 +2826,7 @@ bool GuiView::closeWorkArea(GuiWorkArea * wa, bool close_buffer)
}
if (close_buffer)
- return closeBuffer(buf);
+ return closeBuffer(buf, as_child);
else {
if (!inMultiTabs(wa))
if (!saveBufferIfNeeded(buf, true))
@@ -2837,7 +2837,7 @@ bool GuiView::closeWorkArea(GuiWorkArea * wa, bool close_buffer)
}
-bool GuiView::closeBuffer(Buffer & buf)
+bool GuiView::closeBuffer(Buffer & buf, bool as_child)
{
// If we are in a close_event all children will be closed in some time,
// so no need to do it here. This will ensure that the children end up
@@ -2855,7 +2855,7 @@ bool GuiView::closeBuffer(Buffer & buf)
Buffer * child_buf = *it;
GuiWorkArea * child_wa = workArea(*child_buf);
if (child_wa) {
- if (!closeWorkArea(child_wa, true)) {
+ if (!closeWorkArea(child_wa, true, true)) {
success = false;
break;
}
@@ -2872,7 +2872,10 @@ bool GuiView::closeBuffer(Buffer & buf)
if (saveBufferIfNeeded(buf, false)) {
buf.removeAutosaveFile();
- theBufferList().release(&buf);
+ if (as_child)
+ theBufferList().releaseChild(const_cast<Buffer *>(buf.parent()), &buf);
+ else
+ theBufferList().release(&buf);
return true;
}
}
diff --git a/src/frontends/qt4/GuiView.h b/src/frontends/qt4/GuiView.h
index c3ccdbd..eb722cd 100644
--- a/src/frontends/qt4/GuiView.h
+++ b/src/frontends/qt4/GuiView.h
@@ -155,7 +155,7 @@ public:
/// closes workarea; close buffer only if no other workareas point to it
bool closeWorkArea(GuiWorkArea * wa);
/// closes the buffer
- bool closeBuffer(Buffer & buf);
+ bool closeBuffer(Buffer & buf, bool as_child);
///
void openDocument(std::string const & filename);
///
@@ -401,7 +401,7 @@ private:
bool saveBuffer(Buffer & b, support::FileName const & fn);
/// closes a workarea, if close_buffer is true the buffer will
/// also be released, otherwise the buffer will be hidden.
- bool closeWorkArea(GuiWorkArea * wa, bool close_buffer);
+ bool closeWorkArea(GuiWorkArea * wa, bool close_buffer, bool as_child = false);
/// closes the tabworkarea and all tabs. If we are in a close event,
/// all buffers will be closed, otherwise they will be hidden.
bool closeTabWorkArea(TabWorkArea * twa);
--
2.1.0
>From daf8da89e90bd65e63053c14e8524bfc5f14099c Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Sun, 20 Dec 2015 11:57:33 -0500
Subject: [PATCH] Set parent in loadIfNeeded.
---
src/insets/InsetInclude.cpp | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/insets/InsetInclude.cpp b/src/insets/InsetInclude.cpp
index e9b9600..30cf075 100644
--- a/src/insets/InsetInclude.cpp
+++ b/src/insets/InsetInclude.cpp
@@ -433,11 +433,17 @@ Buffer * InsetInclude::loadIfNeeded() const
if (theBufferList().isLoaded(child_buffer_)
// additional sanity check: make sure the Buffer really is
// associated with the file we want.
- && child_buffer_ == theBufferList().getBuffer(included_file))
+ && child_buffer_ == theBufferList().getBuffer(included_file)) {
+ // The current parent could be some unrelated file if this child
+ // is the child of more than one parent.
+ child_buffer_->setParent(&buffer());
return child_buffer_;
+ }
// Buffer vanished, so invalidate cache and try to reload.
child_buffer_ = 0;
}
+
+ // NOTE: child_buffer_ is now guaranteed to be 0
if (!isLyXFileName(included_file.absFileName()))
return 0;
--
2.1.0
