Closed Bug 404219 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect, P2, critical)

x86
All
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: dholbert)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(7 files, 2 obsolete files)

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.
Flags: blocking1.9?
Group: security
Whiteboard: [sg
Whiteboard: [sg → [sg:critical?]
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
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
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.)
Daniel can you take this one?
Assignee: nobody → dholbert
Priority: P3 → P2
Sure.
Status: NEW → ASSIGNED
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
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
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.
Attachment #292889 - Attachment is obsolete: true
OS: Mac OS X → All
Summary: Crash [@ nsFrame::Destroy] with -moz-column, float, Chinese characters → Crash [@ nsFrame::Destroy] with -moz-column, float
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)
(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.)
Attached patch patch v1Splinter Review
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.
Patch is for code added in bug 379349 -- adding dependency.
Blocks: 379349
Comment on attachment 293061 [details] [diff] [review]
patch v1

Passes reftests.
Attachment #293061 - Flags: review?(roc)
Also: patch v1 fixes all assertions & crashes on Linux for all testcases posted on this bug.
Attachment #293061 - Flags: superreview?(roc)
(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.
Attached patch reftests (obsolete) — Splinter Review
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)
Attachment #293193 - Flags: review?(roc)
Comment on attachment 293061 [details] [diff] [review]
patch v1

Looks good to me. Thanks for the fix.
Attachment #293061 - Flags: review?(roc) → review+
Attachment #293061 - Flags: superreview?(roc) → superreview+
These reftests should be checked in as crashtests, not reftests. Other than that they look fine.
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
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?
Group: security
Flags: wanted1.8.1.x-
Attached patch crashtests patchSplinter Review
(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.)
Attachment #293193 - Attachment is obsolete: true
Attachment #293193 - Flags: review?(roc)
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
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsFrame::Destroy]
You need to log in before you can comment on or make changes to this bug.