Closed
Bug 402380
Opened 17 years ago
Closed 17 years ago
"ASSERTION: unexpected flow" with :first-letter, :before, rtl, wrapping
Categories
(Core :: Layout, defect, P3)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: smontagu)
References
Details
(4 keywords, Whiteboard: [sg:critical])
Attachments
(4 files, 1 obsolete file)
265 bytes,
text/html
|
Details | |
369 bytes,
text/html
|
Details | |
5.52 KB,
patch
|
Details | Diff | Splinter Review | |
1.26 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
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•17 years ago
|
Severity: normal → critical
Comment 2•17 years ago
|
||
(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•17 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•17 years ago
|
||
This is a diff of the frame tree before and after bidi resolution.
Assignee | ||
Updated•17 years ago
|
Attachment #287597 -
Attachment is patch: true
Assignee | ||
Comment 5•17 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.
Comment 6•17 years ago
|
||
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•17 years ago
|
||
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)
Comment 9•17 years ago
|
||
fwiw, I doubt we have any first-letter reftests that seriously test punctuation....
Assignee | ||
Comment 10•17 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)
Assignee | ||
Comment 11•17 years ago
|
||
attachment 213727 [details] is from bug 329069
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•17 years ago
|
||
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•17 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Flags: in-testsuite? → in-testsuite+
Reporter | ||
Comment 15•17 years ago
|
||
The testcases don't trigger any assertions/crashes on the 1.8 branch.
Group: security
Flags: wanted1.8.1.x-
Comment 16•17 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.
Description
•