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.

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

(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.

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

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?

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.

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)

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.

This passes all the same tests.

Checked in.

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

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