369547 - Crash [@ nsSubDocumentFrame::Reflow] with testcase, using first-letter, first-line, inline-block and iframes

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P3)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes my current trunk Mozilla builds within 2 seconds.
It doesn't crash in a 2007-01-26 build, but it does crash with 2007-01-28 build.
This is when the fix for bug 9458 went in. I guess that makes sense, since the testcase also makes use of inline-block.
The testcase isn't completely minimised, I can try and minimise it further if wanted.

Talkback ID: TB29084255Y
nsSubDocumentFrame::Reflow  [mozilla\layout\generic\nsframeframe.cpp, line 426]
nsLineLayout::ReflowFrame  [mozilla\layout\generic\nslinelayout.cpp, line 890]

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I see all sorts of asserts, then:

###!!! ASSERTION: What's going on?: 'mInnerView', file ../../../mozilla/layout/generic/nsFrameFrame.cpp, line 716

Then in Reflow() we try to use mInnerView and crash...

If this _could_ be minimized, that would be nice.  Then we could try to figure out why the docshell has a presshell when it shouldn't.

Comment 2

[Mats Palmgren (inactive)]

Attachment: frame dump + stack

I get the following assertion, which is the root cause of the crash:
###!!! ASSERTION: inserting after sibling frame with different parent

Here's the stack and frame dump when that occurs.
In ContentInserted() we have aContainer=<fieldset> and aChild=<legend>.
First we calculate prevSibling=0x1110f28 (blue) at line 8924:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1308&root=/cvsroot&mark=8924-8926#8917
and set 'parentFrame' to its parent, a first-line frame, 0x1089a00 (green).

The reason we get this rather odd result is that <legend>'s are not
allowed to be siblings to anything except other legend frames:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1308&root=/cvsroot&mark=8066-8074#8031

This is correct AFAICT. Next we find that we 'haveFirstLetterStyle' and
remove those frames and recalculate 'prevSibling':
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1308&root=/cvsroot&mark=9049-9053#9041
Since 'parentFrame' is now the first-line frame IsValidSibling() doesn't
object to inserting it at 'aIndexInContainer' and we set 'prevSibling'
to be the frame at that point, a text frame 0x11e7970 (yellow).

(Note that we don't do "parentFrame = prevSibling->GetParent()" like we did
 on line 8946 when we first calculated 'prevSibling'. Why?)

Then we construct the new frame as a child of 'parentFrame'
(the first-line frame) and insert it using 'prevSibling' which has a
different parent frame.
Oops.


I guess we could add "parentFrame = prevSibling->GetParent()" which would
make us create the new frame with the correct parent but then the special
nsFieldSetFrame logic that picks up legend frames would be bypassed...
(actually, I don't think we have a choice, we need to do this)

Another solution might be to remove the fieldset stuff in IsValidSibling()
and instead test for the special fieldset/legend situation just before
InsertFrames()... (we probably need to do both)

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip

Fixes the crash, but makes nsFieldSetFrame miss the legend frame...

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Note that we don't do "parentFrame = prevSibling->GetParent()"

Uh...  Didn't we fix that once?  I _swear_ I recall writing that.

In any case, we should absolutely do it.

As for IsValidSibling(), I'd really like to get rid of that method, so whatever we can do to do that...

Comment 5

[Mike Schroepfer]

Mats did this fall off your radar?

Comment 6

[Mats Palmgren (inactive)]

It did.  Putting it on my list, but feel free to take it if you have a patch.

Comment 7

[Martijn Wargers (dead)]

This seems to have been fixed somehow between 2007-04-25 and 2007-04-26:
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-04-25+04&maxdate=2007-04-26+09&cvsroot=%2Fcvsroot

Comment 8

[Boris Zbarsky [:bzbarsky]]

The underlying problem is still there.  I have no idea why this particular testcase stopped hitting that problem, but it really should't be hard to custom-write one that hits it.

Comment 9

[Martijn Wargers (dead)]

Ok, a windows resize after the testcase has done its thing still crashes in current trunk builds.

Comment 10

[Martijn Wargers (dead)]

The testcase doesn't crash at all for me anymore with current trunk build.
I don't get the assertion in a debug build that Mats saw in comment 2.
I get this assertion instead:
###!!! ASSERTION: NS_BLOCK_HAS_FIRST_LETTER_STYLE state out of sync: 'haveFirstL
etterStyle == ((mState & NS_BLOCK_HAS_FIRST_LETTER_STYLE) != 0)', file c:/mozill
a/mozilla/layout/generic/nsBlockFrame.cpp, line 6143
However, that might just be a normal assertion.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That's not a "normal" assertion; in this case it's catching that we don't support first-letter on <legend>.  See also bug 369758.

Comment 12

[Martijn Wargers (dead)]

Attachment: testcase2

Ok, this is a testcase that actually crashes again in curren trunk build.

Comment 13

[Jesse Ruderman]

nsSubDocumentFrame::Reflow is the #14 topcrash for Firefox 3 Beta 5.  Is the topcrash this bug?

Comment 14

[Jesse Ruderman]

I filed bug 426987 with a testcase reduced from a URL mentioned in a Breakpad report.

Comment 15

[Jesse Ruderman]

WFM.  No crashes, and the only assertion I see is the one in bug 399262.

Martijn, do your testcases still crash for you now that the topcrashes have been fixed?

Comment 16

[Martijn Wargers (dead)]

Indeed, this is worksforme in current trunk build.
I don't know if the wip patch is still useful, probably not.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Please check in the tests as regression tests!

Comment 18

[Boris Zbarsky [:bzbarsky]]

Added testcase2 as a crashtest.  Can't quite make sense of how to do that with testcase1, so leaving the "in-testsuite?" flag.

Comment 19

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/867c20b932f4

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/867c20b932f4