Closed Bug 402380 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sg:critical])

Attachments

(4 files, 1 obsolete file)

Attached file 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.
Flags: blocking1.9?
Attached file 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
Keywords: crash
Whiteboard: [sg:critical]
Severity: normal → critical
(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...
I assume this is a bidi resolver thing. It's not fixed by the patch in bug 397961.
Assignee: nobody → smontagu
Attached patch Frametree diffSplinter Review
This is a diff of the frame tree before and after bidi resolution.
Attachment #287597 - Attachment is patch: true
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.
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.
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?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Attached patch Possible patch (obsolete) — Splinter Review
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.
Attachment #287867 - Flags: review?(roc)
fwiw, I doubt we have any first-letter reftests that seriously test punctuation....
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)
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.
This passes all the same tests.
Attachment #287867 - Attachment is obsolete: true
Attachment #287886 - Flags: superreview?(roc)
Attachment #287886 - Flags: review?(roc)
Attachment #287867 - Flags: review?(roc)
Attachment #287886 - Flags: superreview?(roc)
Attachment #287886 - Flags: superreview+
Attachment #287886 - Flags: review?(roc)
Attachment #287886 - Flags: review+
Checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Flags: in-testsuite? → in-testsuite+
The testcases don't trigger any assertions/crashes on the 1.8 branch.
Group: security
Flags: wanted1.8.1.x-
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in before you can comment on or make changes to this bug.