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

VERIFIED FIXED

Status

()

Core
Layout: Tables
P3
critical
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: smontagu)

Tracking

(Blocks: 1 bug, 5 keywords)

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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?
(Reporter)

Comment 1

10 years ago
Created attachment 289190 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Updated

10 years ago
Whiteboard: [sg:critical]

Comment 2

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

Comment 3

10 years ago
2007-09-01 nightly: does not crash
2007-09-07 nightly: crashes
Keywords: regression

Comment 4

10 years ago
Just a  guess: bug 393923 would fit into that range and mentions first letter code
(Assignee)

Comment 5

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

10 years ago
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

10 years ago
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
Looks like Simon
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
(Assignee)

Updated

10 years ago
Assignee: nobody → smontagu
(Assignee)

Comment 10

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

Comment 11

10 years ago
Created attachment 290591 [details] [diff] [review]
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
Attachment #290591 - Flags: superreview?(roc)
Attachment #290591 - Flags: review?(roc)
(Assignee)

Comment 12

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

Comment 14

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

Comment 16

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

Comment 17

10 years ago
Created attachment 290746 [details] [diff] [review]
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.
Attachment #290591 - Attachment is obsolete: true
Attachment #290746 - Flags: superreview?(roc)
Attachment #290746 - Flags: review?(roc)
(Assignee)

Comment 18

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

Comment 20

10 years ago
Created attachment 291166 [details] [diff] [review]
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.
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+
(Assignee)

Comment 22

10 years ago
Checked in.
Flags: in-testsuite+
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 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
(Reporter)

Comment 24

10 years ago
No crash or assertions on the 1.8 branch.
Group: security
Flags: wanted1.8.1.x-

Comment 25

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