373533 - Crash [@ nsLineBox::CachedIsEmpty] [@ IsContinuationPlaceholder] with XUL, MathML, HTML

Status: VERIFIED WORKSFORME

(Core :: Layout, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Steps to reproduce:
1. Load the testcase

Result: one or more of:

###!!! ASSERTION: frame not in line: 'line->Contains(aDeletedFrame)', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 5025

Crash, null deref [@ nsLineBox::CachedIsEmpty]

Crash, random address "called by" IsContinuationPlaceholder -- sg:critical

Comment 1

[Jesse Ruderman]

This bug can also trigger:

###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 4667

Comment 2

[Martijn Wargers (dead)]

This regressed between 2006-12-07 and 2006-12-08, so probably a regression from the reflow branch landing.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This doesn't crash my Linux debug build.

Comment 4

[Martijn Wargers (dead)]

This became worksforme between 2007-03-04 and 2007-03-05:
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-03-04+05&maxdate=2007-03-05+09&cvsroot=%2Fcvsroot

Comment 5

[Jesse Ruderman]

Still crashes for me on Mac (debug).

Comment 6

[Martijn Wargers (dead)]

Ah wait, when reloading it quickly a couple of times, I still crash with a 2007-03-22 build, talkback ID: TB30541118K

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack from TB30541118

It crashes for me on the first try in a Linux nightly, but it doesn't crash my debug build (with patches).

Comment 8

[Jesse Ruderman]

Do you guys still get a crash with this testcase?  If not, or if you need a new one that crashes in debug, I can try to make a new one.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A testcase that crashes more reliably would be useful.

Comment 10

[Jesse Ruderman]

Attachment: testcase 2

This crashes reliably on trunk [@ nsLineBox::CachedIsEmpty].

Comment 11

[Damon Sicore (:damons)]

Poke.  


Any news on this?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I can't reproduce any sort of exploitable crash on Linux here, but I do see a null-ptr crash in layout. We seem to manage to remove frames without managing to notify their lines...

Comment 13

[Jeff Walden [:Waldo]]

If you tweak the test slightly to move the appendChild and removeChild calls into two timeouts (boom ends in one which calls appendChild, that one ends in one which calls removeChild), you get these two assertions and a hang on a loop over that circular list:

###!!! ASSERTION: frame not in line: 'line->Contains(aDeletedFrame)'
###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aNextSibling'

Still working on this yet...

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Modification from comment 13

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Another modification, definitely accessing freed frames

So, if you actually read the previous modification, you'll see that it's not what I said it was -- I somehow managed to convert the last call, a removeChild, into an appendChild.  If you do what I *said* I did (posted as this attachment), and make the last call to removeChild, the crash occurs while accessing presshell-deallocated memory.

Comment 16

[Jeff Walden [:Waldo]]

So it seems at least part of the problem is as follows: the outer XUL box is collapsed, so its bounding box is (0,0,0,0).  Within a box, layout of children is done as though the children were themselves boxes, using an nsBoxReflowState to pass state from parent to child.  When we reach the xul:box and try to layout its children, we notice it's collapsed and short-circuit immediately, passing in (0,0,0,0) as the rect for each child.  When we layout a non-XUL child, we don't have an nsHTMLReflowState to use for passing information, so we create a root reflow state, and we pass in our rect as the size.  This height is 0, so things start wrapping, creating continuations, etc. and Eli says we don't deal with that at all.

Comment 17

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

We pass in zero for availableHeight somewhere? That would be wrong.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yes, we do.  nsFrame::BoxReflow uses the passed-in aWidth and aHeight as the available space.  When called from nsFrame::DoLayout, those are mRect.width and mRect.height.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For parentReflowState?  Yeah, that should probably be fixed, but does it matter?

Comment 20

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

Hmm, I think I've seen a bug relating to this somewhere.

It obviously does matter if it's managing to trigger continuation placeholders outside a columns/pagination situation.

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Pass a rect of (0,0,0,UC) instead of (0,0,0,0)

Passing in a rect with height NS_UNCONSTRAINEDSIZE does the right thing for me; it prevents the testcases from asserting, and it seems to be the right thing to do anyway, since in any other non-paginated context we wouldn't know the child's available height.

I added all the testcases here (except the first, which is just the second minimized, basically) to the patch.  The third crashes dereferencing freed memory and absolutely can't be immediately committed to CVS.  The other two, I believe, are null derefs, but I need to double-check that; given how little they differ from the third, I suspect none of them can be committed for awhile.  :-(

Comment 22

[Boris Zbarsky [:bzbarsky]]

Uh.... That diff only has tests in it.

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Include the fix this time

Blah; I'd reverted the fix to ensure the tests crashed in a pre-fix build and hadn't un-reverted.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277980 [details] [diff] [review]
Include the fix this time

This could really use review from Neil.

Comment 25

[[e-mail]]

Comment on attachment 277980 [details] [diff] [review]
Include the fix this time

Something doesn't like this. I got several of these opening DOMi:

###!!! ASSERTION: non-root frame's desired size changed during an incremental reflow: '(target == rootFrame && size.height == NS_UNCONSTRAINEDSIZE) || (desiredSize.width == size.width && desiredSize.height == size.height)', file C:/mozilla/layout/base/nsPresShell.cpp, line 6069

Sadly this was in a debug build without symbols.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Is size.height just unconstrained for a non-root frame here?

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Explicitly resize collapsed children to 0,0 so no frames in the final tree are unconstrained

(In reply to comment #26)
> Is size.height just unconstrained for a non-root frame here?

Yes; if I force the frame to (0,0) after laying it out, that satisfies the assertion, in what seems to be a reasonably correct manner.  I don't get the assertion opening the DOM inspector any more, and I don't see anything else showing up that could have been caused by this patch.

Comment 28

[[e-mail]]

Comment on attachment 278460 [details] [diff] [review]
Explicitly resize collapsed children to 0,0 so no frames in the final tree are unconstrained

>+      nsBoxFrame::LayoutChildAt(aState, child,
>+                                nsRect(0, 0, 0, NS_UNCONSTRAINEDSIZE));
>+
>+      // Fix expectations: a fully reflowed frame tree contains no frames
>+      // with unconstrained size, but since we just passed an unconstrained
>+      // height, we need to fix that.
>+      child->SetSize(nsSize(0, 0));
If we set the size like this is it actually necessary to lay out the child?
(A quick test suggests that it is not and I saw no errors or crashes either.)

Comment 29

[Jeff Walden [:Waldo]]

Attachment: SetSize only

Yeah, I saw the same thing in my testing after you mentioned it on IRC; I think we should be okay doing that, or at least trying it to see if it breaks anything.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry for taking so long to get to this...

So I'm a little uncomfortable about skipping the layout of the children.  Why does their position never matter?

And, looking at the original patch, I'd think a non-unconstrained height ought to be OK within the box code, but we shouldn't propagate that to non-box.  Do you know what's responsible for doing that propagation?

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 278634 [details] [diff] [review]
SetSize only

Marking sr- given previous comment.

Comment 32

[Jesse Ruderman]

WFM with a more recent trunk build.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Jesse, is the underlying problem gone too?  That is, did dbaron's "wrap in block" patch fix this?  Or is something just hiding the crash?

Comment 34

[Jesse Ruderman]

I don't know.  Want me to test nightlies to find out when the crash went away?

Comment 35

[Boris Zbarsky [:bzbarsky]]

If it's easy, please?

Comment 36

[Jesse Ruderman]

2007-11-08 Mac nightly: crashes
2007-11-09 Mac nightly: does not crash

Comment 37

[Boris Zbarsky [:bzbarsky]]

Odd.  Nothing in there obviously looks like it should fix this...

Comment 38

[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 39

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76f18b2b35a5

Comment 40

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

https://hg.mozilla.org/mozilla-central/rev/76f18b2b35a5