404219 - Crash [@ nsFrame::Destroy] with -moz-column, float

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when closed)

Loading the testcase triggers:

###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 943

###!!! ASSERTION: next in flow should have been deleted: '!kidNextInFlow', file /Users/jruderman/trunk/mozilla/layout/generic/nsColumnSetFrame.cpp, line 549

###!!! ASSERTION: Leaking overflow placeholder frames: 'mOverflowPlaceholders.IsEmpty()', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockReflowState

Closing it triggers a crash [@ nsFrame::Destroy] dereferencing 0xdddddddd.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2L (assertions on Linux)

This somewhat-reduced testcase just triggers the first two assertions, with all page content disappearing, on Linux.  (No crash)

Those assertions are:
###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file mozilla/layout/generic/nsBlockFrame.cpp, line 943
###!!! ASSERTION: next in flow should have been deleted: '!kidNextInFlow', file mozilla/layout/generic/nsColumnSetFrame.cpp, line 549

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2M (assertions on Mac + Linux)

This testcase triggers the same behavior as the previous testcase, and it works on Linux *and* Mac.

(Due to fonts differences on Mac vs Linux, we need a little more content in the div with id="c" to make that content wrap enough to be hidden on Mac.)

Comment 3

[Mike Schroepfer]

Daniel can you take this one?

Comment 4

[Daniel Holbert [:dholbert]]

Sure.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (assertions, no JS)

This testcase doesn't use any JS tweaks.  It triggers two instances of this assertion on first pageload:

 ###!!! ASSERTION: next in flow should have been deleted: '!kidNextInFlow', file /scratch/work/builds/trunk.07-12-12.15-51/mozilla/layout/generic/nsColumnSetFrame.cpp, line 549

and it triggers this assertion on reload, at the end of the new pageload:

 ###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file /scratch/work/builds/trunk.07-12-12.15-51/mozilla/layout/base/nsPresShell.cpp, line 673

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (crashes Firefox when reloaded / closed)

This testcase crashes Firefox when the page is reloaded or when the browser is closed.  It triggers the same assertions as this bug's original testcase.

As noted in the HTML, lower values of c.height (less than 19px) will fix the blank page and the crash, but we trigger:
   ###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 4a (all platforms: crashes Firefox when reloaded / closed)

Tweaked a height in testcase 4 so that it crashes on *all* platforms. (previous version crashed on linux but not windows)

This one crashes on Win/Mac/Linux.

Comment 8

[Daniel Holbert [:dholbert]]

So, one of the first things to go wrong here is this:

 [A] nsBlockFrame::ReflowDirtyLines() fails for the ColumnSetFrame's second Block(div).    (This triggers the first assertion in Comment 0)  Right after we assert, we pass the failure rv upwards:
  938   if (NS_FAILED(rv)) return rv;

 [B] We pop up to nsContainerFrame.cpp:711 (with this == the nsColumnSetFrame) at which point we skip the "delete kidNextInFlow" chunk because Reflow() returned a failure code.  We return the same "result" failure code that we were given.

 [C] We pop up to nsColumnSetFrame.cpp:520.
    ***At this point, the return value from ReflowChild is discarded -- we don't store it, and so we can't (and don't) react to it!!  So, later on, we fail an assertion that "kidNextInFlow" is empty.  (This is the second assertion in Comment 0)   We fail this because, as I said above, we skipped the "delete kidNextInFlow" section just after nsContainerFrame.cpp:711, due to the failure return code.

=====

So, my next move is to find out why ReflowDirtyLines() is failing, as that seems to be the ultimate cause here.

However, we'd also ideally like nsColumnSetFrame to be able to react to the return value of ReflowChild.  (particularly, to be able to bail out somehow if it's a failure code)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> However, we'd also ideally like nsColumnSetFrame to be able to react to the
> return value of ReflowChild.  (particularly, to be able to bail out somehow if
> it's a failure code)

Sorry -- to be more precise:  It'd be nice for nsColumnSetFrame::ReflowChildren to react to the return-value of nsColumnSetFrame::ReflowChild, at line nsColumnSetFrame.cpp:517.

(not sure *how* we'd react to various return values... But for one thing, we'd know that kidNextInFlow isn't necessarily empty, and so we'd probably want to avoid the
   NS_ASSERTION(!kidNextInFlow, "next in flow should have been deleted");
assertion at line 549.)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This patch seems to fix the bug.  I haven't run it through reftests yet -- will report back on that shortly.

The problem was pretty simple -- when we fail at removing a frame from a container's overflow-list, we left the list orphaned, and never gave it back to the container.

In more detail, the situation was:
  - We've got both an OverflowContainers list and an ExcessOverflowContainers list
  - We call StealFrame for something in ExcessOverflowContainers list
  - First, StealFrame tries removing it from the OverflowContainers.  This fails, *** orphaning our OverflowContainers list in the process ***
  - Then StealFrame tries removing it from the ExcessOverflowContainers list and succeeds.

We're left with an orphaned OverflowContainers list, which causes issues down-the-line.

Comment 11

[Daniel Holbert [:dholbert]]

Patch is for code added in bug 379349 -- adding dependency.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 293061 [details] [diff] [review]
patch v1

Passes reftests.

Comment 13

[Daniel Holbert [:dholbert]]

Also: patch v1 fixes all assertions & crashes on Linux for all testcases posted on this bug.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #13)
> Also: patch v1 fixes all assertions & crashes on Linux for all testcases posted
> on this bug.

I just tested the patch on Mac as well, and it fixes all assertions & crashes there, too.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: reftests

Here are two reftests.  The only difference between them is that div#b has a specified height in the second testcase (but not in the first).

Added in reftest.list:
+!= 404219-1.html about:blank    # assertion / show-something test
+!= 404219-2.html about:blank    # assertion / crash / show-something test

In more detail: before patching, these testscases do these things wrong:
 (a) Both tests show up as blank pages due to errors
 (b) Both tests fail some assertions (see first comment on this bug)
 (c) The second testcase crashes on reload/exit (not sure how best to reftest for that? right now it makes the reftest process crash when it's done, in addition to failing the != about:blank check)

After patching, all three of those issues are fixed.

I'm not sure when it's considered "safe" to check in reftests for security-sensitive bugs, so I'm not planning on checking these in at the same time as the patch.  (but let me know if I should)

Comment 16

[fantasai]

Comment on attachment 293061 [details] [diff] [review]
patch v1

Looks good to me. Thanks for the fix.

Comment 17

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

These reftests should be checked in as crashtests, not reftests. Other than that they look fine.

Comment 18

[Daniel Holbert [:dholbert]]

Patch v1 checked in:

Checking in nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v  <--  nsContainerFrame.cpp
new revision: 1.298; previous revision: 1.29

Comment 19

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020819 Minefield/3.0b4pre and the testcases from this bug. 

No crash on testcases -> Verified fixed

Comment 20

[Jesse Ruderman]

None of the testcases cause assertions/crashes on branch, and Firefox 3 has had a few betas since this bug was fixed, so I'm making this bug public.

dholbert, it looks like you already decided which tests should be checked in.  Can you check them in now?

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: crashtests patch

(In reply to comment #17)
> These reftests should be checked in as crashtests, not reftests. Other than
> that they look fine.
(In reply to comment #20)
> dholbert, it looks like you already decided which tests should be checked in. 
> Can you check them in now?

Ok -- here's reftests patch converted into crashtests. I'll land this in a minute. (Sorry it took me a little while to get to this.)

Comment 22

[Daniel Holbert [:dholbert]]

Landed crashtests patch.

RCS file: /cvsroot/mozilla/layout/generic/crashtests/404219-1.html,v
done
Checking in 404219-1.html;
/cvsroot/mozilla/layout/generic/crashtests/404219-1.html,v  <--  404219-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/generic/crashtests/404219-2.html,v
done
Checking in 404219-2.html;
/cvsroot/mozilla/layout/generic/crashtests/404219-2.html,v  <--  404219-2.html
initial revision: 1.1
done
Checking in crashtests.list;
/cvsroot/mozilla/layout/generic/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.103; previous revision: 1.102
done