On 12/29/2016 01:21 AM, Guillaume Munch wrote: > Le 27/12/2016 à 23:11, Scott Kostyshak a écrit : >> On Tue, May 31, 2016 at 01:15:38AM +0200, Guillaume Munch wrote: >>> commit 1cc14a31ca8320d881b674f93c34a09cf1666cca >>> Author: Guillaume Munch <g...@lyx.org> >>> Date: Mon May 30 21:42:08 2016 +0100 >>> >>> TocWidget: fix an erroneous collapse and optimise updates based >>> on profiling >>> >>> TocModels::reset() in GuiView::structureChanged() collapses the >>> TocWidget, and >>> therefore requires an update right after, which was missing. >>> >>> In fact, profiling TocWidget::updateView() shows that delaying >>> the update is >>> good only for fast keypresses (essentially movement). It costs >>> 5% of a >>> char-forward operation in a document with approx. 100 table of >>> contents >>> items. The update optimisation has been rewritten to take this >>> data into >>> account. >> >> A git bisect suggests this causes a SIGSEGV. To reproduce: >> >> 1. open the attached file >> 2. open the outline pane >> 3. put the cursor just to the left of "Hello" (but inside the caption >> inset) >> 4. "Save As" to a new filename. >> >> Can you reproduce? >> >> Backtrace is attached. >> > > Hi, > > The root of the issue is in Buffer::reload() called during "Save As". > After loadLyXFile() there, all the insets have been deleted, and > therefore the Inset pointer > GuiView::documentBufferView()->cursor().inset() is dangling. Immediately > after loadLyXFile(), reload() calls updateBuffer() from your backtrace > which then crashes. > > While debugging I got other segfaults caused by the same dangling Inset > pointer in Cursor, notably: 1) a trace identical to the second one from > <http://www.lyx.org/trac/ticket/10520>, and 2) a repeated segfault in > the critical path (a call to inMathed()) which closes LyX instantly > after the first segfault is caught (luckily after the emergency save is > complete). > > I am trying to find a proper fix for this issue, but the strategy for > managing these Inset pointers in CursorSlice escapes me. Does anyone > know which assumptions are made on when these pointers must be valid? > If there are such assumptions, how are they supposed to be enforced? > > The attached patch fixes this crash, but I find very unsatisfactory to > have to update the cursors by hand like this.
I don't know the answer to those questions, but I think that something like this solution is all right. The problem is that we would ordinarily suppose that, after reading the document, we have a brand new cursor in the open view. But that's not true in this case, because the document has been reloaded. It's probably fine to put the updateCursors() call in readDocument, as you have it. But it can also be put into Buffer::reload, right before the call to updateBuffer. That's where we actually need it, it seems to me. Richard
