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

RESOLVED FIXED

Status

()

P3
critical
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: jruderman, Assigned: smontagu)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Mac OS X
assertion, crash, rtl, testcase
Points:
---
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Created attachment 287257 [details]
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?
(Reporter)

Comment 1

11 years ago
Created attachment 287258 [details]
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
(Reporter)

Updated

11 years ago
Keywords: crash
Whiteboard: [sg:critical]
(Reporter)

Updated

11 years ago
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...
(Assignee)

Comment 3

11 years ago
I assume this is a bidi resolver thing. It's not fixed by the patch in bug 397961.
Assignee: nobody → smontagu
(Assignee)

Comment 4

11 years ago
Created attachment 287597 [details] [diff] [review]
Frametree diff

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

Updated

11 years ago
Attachment #287597 - Attachment is patch: true
(Assignee)

Comment 5

11 years ago
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
(Assignee)

Comment 8

11 years ago
Created attachment 287867 [details] [diff] [review]
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.
Attachment #287867 - Flags: review?(roc)
fwiw, I doubt we have any first-letter reftests that seriously test punctuation....
(Assignee)

Comment 10

11 years ago
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.
(Assignee)

Comment 13

11 years ago
Created attachment 287886 [details] [diff] [review]
Do it in FirstLetterFrame

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+
(Assignee)

Comment 14

11 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Flags: in-testsuite? → in-testsuite+
(Reporter)

Comment 15

11 years ago
The testcases don't trigger any assertions/crashes on the 1.8 branch.
Group: security
Flags: wanted1.8.1.x-

Comment 16

11 years ago
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.