389428 - crash [@ gfxTextRun::CompressedGlyph::IsClusterStart] in nsTextFrame::Reflow

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm crashing 100% of the time while loading https://bugzilla.mozilla.org/show_bug.cgi?id=288839 .  This is in a debug build built from a pull a few hours ago (Tue Jul 24 07:58:23 PDT 2007).  The stack is:

#5  <signal handler called>
#6  0x011cb1e7 in gfxTextRun::CompressedGlyph::IsClusterStart (this=0x1a92a4e0) 
at ../../../dist/include/thebes/gfxFont.h:920
#7  0x082474c0 in nsTextFrame::Reflow (this=0x1dc58870, aPresContext=0x1acd4e30,
 aMetrics=@0xbfacab44, aReflowState=@0xbfacaa98, 
    aStatus=@0xbfacac34) at /builds/trunk/mozilla/layout/generic/nsTextFrameTheb
es.cpp:5373
#8  0x0820faff in nsLineLayout::ReflowFrame (this=0xbfacad4c, aFrame=0x1dc58870,
 aReflowStatus=@0xbfacac34, aMetrics=0x0, 
    aPushedFrame=@0xbfacac30) at /builds/trunk/mozilla/layout/generic/nsLineLayo
ut.cpp:891
#9  0x081b7dbe in nsBlockFrame::ReflowInlineFrame (this=0x1d18d274, aState=@0xbf
acb300, aLineLayout=@0xbfacad4c, aLine=
      {mCurrent = 0x1dc588b4, mListLink = 0x1d18d2b4}, aFrame=0x1dc58870, aLineR
eflowStatus=0xbfacace4)
    at /builds/trunk/mozilla/layout/generic/nsBlockFrame.cpp:3449
...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The first assertion leading to the crash is here (note offsets in frames 2 and 4):

#1  0x0069febb in NS_DebugBreak_P (aSeverity=1, 
    aStr=0x124a96f "Invalid offset", 
    aExpr=0x124a8d0 "aOffset <= mSkipChars->mCharCount", 
    aFile=0x124a804 "/builds/trunk/mozilla/gfx/thebes/src/gfxSkipChars.cpp", 
    aLine=92) at /builds/trunk/mozilla/xpcom/base/nsDebugImpl.cpp:350
#2  0x011e8a51 in gfxSkipCharsIterator::SetOffsets (this=0xbfad6218, 
    aOffset=4294966723, aInOriginalString=1)
    at /builds/trunk/mozilla/gfx/thebes/src/gfxSkipChars.cpp:91
#3  0x0644e4bf in gfxSkipCharsIterator::SetOriginalOffset (this=0xbfad6218, 
    aOriginalStringOffset=76) at ../../dist/include/thebes/gfxSkipChars.h:265
#4  0x0644e584 in gfxSkipCharsIterator (this=0xbfad6218, 
    aSkipChars=@0x895361c, aOriginalStringToSkipCharsOffset=-649, 
    aOriginalStringOffset=76) at ../../dist/include/thebes/gfxSkipChars.h:228
#5  0x06448a98 in nsTextFrame::EnsureTextRun (this=0x93e68b4, aRC=0x93572c0, 
    aLineContainer=0x932f7d8, aLine=0xbfad685c, aFlowEndInTextRun=0xbfad64d4)
    at /builds/trunk/mozilla/layout/generic/nsTextFrameThebes.cpp:1986
#6  0x064491ec in nsTextFrame::Reflow (this=0x93e68b4, aPresContext=0x9084438, 
    aMetrics=@0xbfad6604, aReflowState=@0xbfad6558, aStatus=@0xbfad66f4)
    at /builds/trunk/mozilla/layout/generic/nsTextFrameThebes.cpp:5264

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like the TextRunMappedFlow represents a 0-length segment at the very end of the text frame (which has mContentOffset==76 and mContentLength==573), but we're trying to initialize the iterator (I think that's what the aOriginalStringOffset parameter to the construtor means) at the position of the beginning of the text frame (mContentLength), which is not in the text run.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and to be clear, this TextRunMappedFlow is at the very beginning of the text run -- it has 5 mapped flows.

I'm surprised that a single text frame would have more than one text run corresponding to it -- maybe the underlying problem is that we're creating a text run that covers only a part of the text frame, and thus it has two text runs?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To be more specific, the text frame in question is the one representing the text of the <pre id="comment_text_0"> up to the start of the first a element inside it.  Its text run represents the text from the beginning of that a element through the end of the pre (which includes 2 a elements in total).

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So what I see (with printfs in BuildTextRunsScanner::BuildTextRunForFrames) is:

Creating flows:
Creating flow 0xaf011ac for frame 0xae4417c xoffset 0 length 649

Creating flows:
Creating flow 0xaf1b7e4 for frame 0xaf1c644 xoffset -649 length 0
Creating flow 0xaf1b7f0 for frame 0xae44350 xoffset 0 length 85
Creating flow 0xaf1b7fc for frame 0xae44310 xoffset 85 length 110
Creating flow 0xaf1b808 for frame 0xae444b4 xoffset 195 length 39
Creating flow 0xaf1b814 for frame 0xae44474 xoffset 234 length 285

0xaf1c644 is the next-in-flow of 0xae4417c, and its GetContentOffset gets changed from 649 when we create the flows to 76 when the assert happens.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the problem seems to be happening the second time we reflow the frames in question.  The first time, there's only one frame for the text node, and things are ok.  But the second time, we have a first-in-flow with mContentLength = 649 and then a series of next-in-flows with mContentLength = 0 and mContentOffset = 649.  Presumably one for each line, but they've had their offsets reset to the end at some point.

So BuildTextRunsScanner::ContinueTextRunAcrossFrames is returning false for whether to continue the text run after this 649-character frame because the first text node in the pre happens to end in a newline.  Since it has a continuation at this point, we then start the next text run with the 0-length continuation, whose offset gets adjusted when the reflow finishes.

I'm not sure why we have this chain of 0-length continuations, though.

But to work around this, I'll just temporarily disable that piece of ContinueTextRunAcrossFrames in my tree.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This will probably be fixed by bug 385270, or at least, any fix will depend on that.

Comment 8

[Jesse Ruderman]

Long bug reports causing crashes kinda breaks dogfooding...

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Okay. The immediate problem here is that bidi resolution is reassigning all the content to the first frame. The first thing we need to do is have AdjustOffsetsForBidi clear the textrun on the primary frame so it can be reconstructed (we were already doing that for next-in-flows that we're emptying out but that's not enough).

That's not enough though; we still reach the situation David described where next-in-flows with a zero-length textrun assigned to them are being remapped to content in the first frame's textrun, without updating the textruns, and havoc ensues. The best thing to do here is to simply ensure that when the next-in-flow's content offsets change (because we're either moving content from this frame to a next-in-flow, or from a next-in-flow to this frame), if the textruns for the frames are different, wipe out the next-in-flow's textrun to ensure a correct textrun is recreated. (Note that it's not normally necessary to wipe out this frame's textrun because the current frame can't *grow* across a textrun boundary during Reflow except for the first-letter/first-line case, which is handled explicitly. In particular the current frame can't grow beyond a textrun boundary induced by a preformatted newline.)

Comment 10

[Caio Tiago Oliveira (asrail)]

I got the same warnings and almost the same stack trace at:
 http://en.wikipedia.org/wiki/Ray_tracing

viewed at 14px or less (press control+- once on a new profile).

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixed by backout of 385270