Closed Bug 404209 Opened 14 years ago Closed 14 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

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

Crash Data

Attachments

(2 files, 2 obsolete files)

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.
Flags: blocking1.9?
Whiteboard: [sg:critical]
The testcase looks rather ordinary, there should be a regression range for this, the fuzzers didn't change that much over the past months.
2007-09-01 nightly: does not crash
2007-09-07 nightly: crashes
Keywords: regression
Just a  guess: bug 393923 would fit into that range and mentions first letter code
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.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Assignee: nobody → 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.' 
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #290591 - Flags: superreview?(roc)
Attachment #290591 - Flags: review?(roc)
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.
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?
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.
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 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.
Attachment #290591 - Flags: superreview?(roc)
Attachment #290591 - Flags: review?(roc)
Attached patch New patch (obsolete) — Splinter Review
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.
Attachment #290591 - Attachment is obsolete: true
Attachment #290746 - Flags: superreview?(roc)
Attachment #290746 - Flags: review?(roc)
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 ;-)
+ * 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.
Attached patch Patch v.3Splinter Review
(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.
Attachment #290746 - Attachment is obsolete: true
Attachment #291166 - Flags: superreview?(roc)
Attachment #291166 - Flags: review?(roc)
Attachment #290746 - Flags: superreview?(roc)
Attachment #290746 - Flags: review?(roc)
Comment on attachment 291166 [details] [diff] [review]
Patch v.3

cool
Attachment #291166 - Flags: superreview?(roc)
Attachment #291166 - Flags: superreview+
Attachment #291166 - Flags: review?(roc)
Attachment #291166 - Flags: review+
Checked in.
Flags: in-testsuite+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
No crash or assertions 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
Crash Signature: [@ nsLineLayout::ReflowFrame]
You need to log in before you can comment on or make changes to this bug.