On 12/30/2016 12:15 PM, Guillaume Munch wrote: > Le 30/12/2016 à 17:11, Richard Heck a écrit : >> 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. >> > > Thanks for the patch. It is the same as calling fixIfBroken in > GuiView::structureChanged (as attached) which I agree becomes even > simpler. But, provided one accepts the idea that DocIterators hold > perishable pointers that need to be fixed (I do not see any way out of > it), I find it better to fix them the moment we know that they are > broken.
This seems fine to me, though maybe a bit of overkill. > However, I still miss how and when all the cursors not connected > to the GuiView are being fixed, though. Any idea? I worried about this, too, and I'm not sure they are fixed. One case to worry about would be if Scott's file were open in two windows. But my patch still seems to work. You might want to try this one, too. Richard
