402380 - "ASSERTION: unexpected flow" with :first-letter, :before, rtl, wrapping

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

###!!! ASSERTION: unexpected flow: 'mFrames.ContainsFrame(nextInFlow)', file /Users/jruderman/trunk/mozilla/layout/generic/nsInlineFrame.cpp, line 471

The testcase doesn't crash, but I think there's an sg:critical crash in here somewhere.

Comment 1

[Jesse Ruderman]

Attachment: crashing testcase

In addition to the assertion in comment 0, this testcase triggers another assertion and a crash.

###!!! ASSERTION: StealFrame failure: 'NS_SUCCEEDED(rv)', file /Users/jruderman/trunk/mozilla/layout/generic/nsContainerFrame.cpp, line 1059

[@ nsFrameList::InsertFrame] - dereferencing null
[@ nsInlineFrame::PullOneFrame] - calling a random address
[@ nsLineLayout::ReflowFrame] - calling 0xdddddddd

Comment 2

[Boris Zbarsky [:bzbarsky]]

(gdb) frame
#0  0xb67c67df in nsInlineFrame::ReflowFrames (this=0xb4020cc4, aPresContext=0x89b5610, 
    aReflowState=@0xbfffc130, irs=@0xbfffc020, aMetrics=@0xbfffc0e0, aStatus=@0xbfffc280)
    at ../../../mozilla/layout/generic/nsInlineFrame.cpp:471
471             NS_ASSERTION(mFrames.ContainsFrame(nextInFlow), "unexpected flow");
(gdb) p this
$4 = (nsInlineFrame *) 0xb4020cc4
(gdb) p nextInFlow
$5 = (nsContinuingTextFrame *) 0xb40202b8

The relevant part of the frame tree looks like:

 line 0xb4020cfc: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4000] {0,0,0,0} <
   Inline(span)(0)@0xb4020cc4 next=0xb4020224 prev-continuation=0xb4013af4 next-continuation=0xb4020224 {0,0,0,0} [state=00000407] [content=0xb4004ed0] [sc=0xb400e100]<
     Text(0)@0xb4013d14[0,0,F]  next-continuation=0xb40202b8 {0,0,0,1380} [state=01020000] [content=0xb4019948] sc=0xb4013c34 pst=:-moz-non-element<
       ""
     >
   >
 >
 line 0xb402025c: count=1 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x4121] {0,1380,600,1380} <
   Inline(span)(0)@0xb4020224 next=0xb40202fc prev-continuation=0xb4020cc4 next-continuation=0xb40202fc {0,1380,600,1380} [state=00000004] [content=0xb4004ed0] [sc=0xb400e100]<>
 >
 line 0xb4020334: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,2760,1380,1380} <
   Inline(span)(0)@0xb40202fc prev-continuation=0xb4020224 {0,2760,1380,1380} [state=00000004] [content=0xb4004ed0] [sc=0xb400e100]<
     Text(0)@0xb40202b8[0,7,T]  prev-continuation=0xb4013d14 {0,0,1380,1380} [state=10620004] [content=0xb4019948] sc=0xb4013c34 pst=:-moz-non-element<
       "is text"
     >
   >
 >

So the issue is that our kid's next-in-flow is a child of our next-in-flow's next-in-flow, not of our next-in-flow.  I'm not quite sure how we get into that state...

Comment 3

[Simon Montagu :smontagu]

I assume this is a bidi resolver thing. It's not fixed by the patch in bug 397961.

Comment 4

[Simon Montagu :smontagu]

Attachment: Frametree diff

This is a diff of the frame tree before and after bidi resolution.

Comment 5

[Simon Montagu :smontagu]

Part of the frame tree before bidi resolution:

Inline(span)(0)@05B56A0C {0,0,2400,960} [state=00000048] [content=0695B0A0] [overflow=0,0,2880,960] [sc=05B57E00] pst=:before<

  Letter(span)(0)@05B56E64 next=05B56AC0 {0,240,480,480} [state=00000040] [content=0695B0A0] [sc=05B57E50] pst=:first-letter<

    Text(-1)@05B56E24[0,1,T]  {0,0,480,480} [state=00300040] [content=046996D0] sc=05B57EFC pst=:-moz-non-element<

      "\020034"

    >

  >

  Text(-1)@05B56AC0[0,1,F]  next=05B57464 next-continuation=05B57464 {480,240,480,480} [state=00100040] [content=046997C0] sc=05B56DD4 pst=:-moz-non-element<

    "T"

  >

  Text(-1)@05B57464[1,4,T]  prev-continuation=05B56AC0 {960,0,1440,960} [state=0040004c] [content=046997C0] [overflow=0,0,1920,960] sc=05B56DD4 pst=:-moz-non-element<

    "his "

  >

>

I think that the bidi resolver is getting confused by the "T" and "his" in separate frames. I've tried to work round this in RemoveBidiContinuation, but it gets complicated and it's probably better to solve the problem earlier -- I assume that this is a manifestation of bug 45091.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Uh... so why exactly do we think there is a break opportunity after that "T"?  Or rather, why do we go ahead and create a continuing frame there?  Before we reflow, the first-letter frame just wraps the textframe for the '"'....  But during reflow the "This " text decides that the right content length for it is 1.

Comment 7

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

My guess is that the textframe for the quote character didn't clear first-letter state in nsLineLayout (because it's leading punctuation, so following text is allowed to also be first-letter), so the next textframe treated itself as first-letter too, even though it isn't wrapped in the first-letter frame. I guess for consistency we need to clear first-letter state when we leave the first-letter frame inserted by the frame constructor. Simon, can you handle that?

Comment 8

[Simon Montagu :smontagu]

Attachment: Possible patch

Like this, maybe? I can't say I really understand the code that I'm patching, but this stops the assertion and crash in the testcases here, and doesn't cause any regressions in the first-letter reftests.

Comment 9

[Boris Zbarsky [:bzbarsky]]

fwiw, I doubt we have any first-letter reftests that seriously test punctuation....

Comment 10

[Simon Montagu :smontagu]

Some other testcases that I checked:
attachment 4119 [details] from bug 23605
attachment 213727 [details] from bug 32906
attachment 285035 [details] from bug 399941
http://www.hixie.ch/tests/evil/mixed/pseudoelements1.html

All these display the same as trunk (which in some cases means they are broken the same way)

Comment 11

[Simon Montagu :smontagu]

attachment 213727 [details] is from bug 329069

Comment 12

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

Seems to me a better place to do this would be here:
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsFirstLetterFrame.cpp#310
when an nsFirstLetterFrame is complete, turn first-letter status off.

Comment 13

[Simon Montagu :smontagu]

Attachment: Do it in FirstLetterFrame

This passes all the same tests.

Comment 14

[Simon Montagu :smontagu]

Checked in.

Comment 15

[Jesse Ruderman]

The testcases don't trigger any assertions/crashes on the 1.8 branch.

Comment 16

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).