404209 - Crash [@ nsLineLayout::ReflowFrame] with table, rtl, :first-letter

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[Jesse Ruderman]

Loading the testcase triggers two assertions and a crash.

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

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

Crash [@ nsLineLayout::ReflowFrame], *calling* 0xdddddddd or 0x00000000 or another address.

Comment 1

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 2

[Bernd]

The testcase looks rather ordinary, there should be a regression range for this, the fuzzers didn't change that much over the past months.

Comment 3

[Jesse Ruderman]

2007-09-01 nightly: does not crash
2007-09-07 nightly: crashes

Comment 4

[Bernd]

Just a  guess: bug 393923 would fit into that range and mentions first letter code

Comment 5

[Simon Montagu :smontagu]

I can reproduce the crash in my "clean" tree but not in my "work" tree. Trying now to isolate which patch or w-i-p is fixing it.

Comment 6

[Martijn Wargers (dead)]

It regressed between 2007-09-01 and 2007-09-03 for me:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-01+04&maxdate=2007-09-03+06&cvsroot=%2Fcvsroot

Comment 7

[Bernd]

more layout centric bonsai query
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-01+04&maxdate=2007-09-03+06&cvsroot=%2Fcvsroot

Comment 8

[Bernd]

argh

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Flayout&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-01+04&maxdate=2007-09-03+06&cvsroot=%2Fcvsroot

Comment 9

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

Looks like Simon

Comment 10

[Simon Montagu :smontagu]

This is rather similar to bug 402380, in that we enter Bidi resolution with the text "abcd" split into two frames, one with the "a" and one with "bcd", but in this case there is no first-letter frame in the frame tree at any stage.

I don't know why there is this inconsistency (presumably first-letter state is set in nsLineLayout as in the other bug), but it seems that the frame tree is correct: the nearest thing I can find in http://www.w3.org/TR/CSS21/selector.html#first-letter is:
'The first letter of a table-cell or inline-block cannot be the first letter of an ancestor element. Thus, in <DIV><P STYLE="display: inline-block">Hello<BR>Goodbye</P> etcetera</DIV> the first letter of the DIV is not the letter "H". In fact, the DIV doesn't have a first letter.' 

Comment 11

[Simon Montagu :smontagu]

Attachment: Patch

The issue here is that we set FirstLetterStyleOK in nsLineLayout according to NS_BLOCK_HAS_FIRST_LETTER_STYLE, but we always set NS_BLOCK_HAS_FIRST_LETTER_STYLE in nsCSSFrameConstructor::WrapFramesInFirstLetterFrame even if we don't end up creating the first letter frame. I asked bz on IRC if we should change that, and he suggested just adding a new style bit to indicate that there is an actual first letter frame

Comment 12

[Simon Montagu :smontagu]

I wasn't sure what to do about the assertion at http://mxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#6328. It would be nice to have an assertion that fires in the situation in the testcase here.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Simon, does the FIRST_LETTER_CHILD bit mean that the block has _some_ first-letter frame?  Or in particular an inline one?

Put another way, what should the boolean in line layout be if there is a floating first-letter frame?

Comment 14

[Simon Montagu :smontagu]

At the moment it means any first-letter frame, but maybe it should only be set with a inline first-letter. I understand from http://mxr.mozilla.org/seamonkey/source/layout/generic/nsFirstLetterFrame.cpp#274 that floating first-letters aren't associated with a linelayout at all.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I guess the real question is why a floating first-letter won't screw things up the same way that the bit we currently use screws them up.

Comment 16

[Simon Montagu :smontagu]

Comment on attachment 290591 [details] [diff] [review]
Patch

Who says it won't? We have had all sorts of issues lately with floating first letter frames, and the fixes have tended to be duct-tape in the bidi resolver. Postponing review requests while I investigate this.

Comment 17

[Simon Montagu :smontagu]

Attachment: New patch

In this patch the HAS_FIRST_LETTER_CHILD bit is only set on inline frames.

However, the known issues with floating first-letter frames must have some other cause: floating first-letter frames currently remove the first-letter style flag from LineLayout at http://mxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#921. I tried removing that and ringing the changes on the testcase, but didn't find any new crashes.

Comment 18

[Simon Montagu :smontagu]

Comment on attachment 290746 [details] [diff] [review]
New patch

>Index: layout/generic/nsBlockFrame.cpp

>+      //(NS_BLOCK_HAS_FIRST_LETTER_CHILD & mState) &&

Oops, that line isn't really commented out ;-)

Comment 19

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

+ * NS_BLOCK_HAS_FIRST_LETTER_CHILD means that there is a first-letter inline 
+ *  frame among the block's descendants.

But it's not set if the inline frame is floating, right? You'd better mention that.

+#ifdef DEBUG
         if (outOfFlowFrame->GetType() == nsGkAtoms::letterFrame) {
-          SetFlag(LL_FIRSTLETTERSTYLEOK, PR_FALSE);
+          NS_ASSERTION(!GetFirstLetterStyleOK(),
+                "FirstLetterStyle set on line with floating first letter");
         }
+#endif

This can all just be one big NS_ASSERTION. But why are we asserting that here? Shouldn't LL_FIRSTLETTERSTYLEOK be true here since the placeholder will be the first thing on the first line?

     PRBool        pushedFrame;

Get rid of the space alignment here.

Comment 20

[Simon Montagu :smontagu]

Attachment: Patch v.3

(In reply to comment #19)
> + * NS_BLOCK_HAS_FIRST_LETTER_CHILD means that there is a first-letter inline 
> + *  frame among the block's descendants.
> 
> But it's not set if the inline frame is floating, right? You'd better mention
> that.

"in-flow" would probably have been a better choice of words than "inline" here and in my previous comments in the bug. I've made the code comment clearer, I hope.

> This can all just be one big NS_ASSERTION. But why are we asserting that here?
> Shouldn't LL_FIRSTLETTERSTYLEOK be true here since the placeholder will be the
> first thing on the first line?

My understanding, based on http://mxr.mozilla.org/seamonkey/source/layout/generic/nsFirstLetterFrame.cpp#274, is that LL_FIRSTLETTERSTYLEOK doesn't need to be set on the line when there is a placeholder, since the floating first-letter-frame creates its own line-layout.

I removed the "FirstLetterStyle not set on line with inline first-letter" assertion, because it fires sometimes when we reflow the first-letter frame again after we have already reflowed its text-frame child and reset the bit.

Comment 21

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

Comment on attachment 291166 [details] [diff] [review]
Patch v.3

cool

Comment 22

[Simon Montagu :smontagu]

Checked in.

Comment 23

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007123104 Minefield/3.0b3pre and the testcase from this bug - no crash on testcase

-> Verified fixed

Comment 24

[Jesse Ruderman]

No crash or assertions on the 1.8 branch.

Comment 25

[(no longer active)]

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