359371 - Float in column disappears; crash [@ nsBlockFrame::IsFloatContainingBlock] [@ nsBlockFrame::ClearLineCursor]

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect)

Comment 1

[Jesse Ruderman]

Attachment: smallish testcase

Comment 4

[Jesse Ruderman]

The larger testcase causes two assertions before the scary crash:

###!!! ASSERTION: bad geometric parent: 'IsChild(aNextInFlow)', file /Users/admin/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 6090

###!!! ASSERTION: can't find deleted frame in lines: 'Error', file /Users/admin/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 5914


The smaller testcases causes those assertions plus several more:

###!!! ASSERTION: frame should be in parent's lists: 'NS_SUCCEEDED(rv)', file /Users/admin/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 4989

###!!! ASSERTION: same old and new parent frame: 'aOldParentFrame != aNewParentFrame', file /Users/admin/trunk/mozilla/layout/generic/nsHTMLContainerFrame.cpp, line 419

###!!! ASSERTION: Null out-of-flow for placeholder?: 'outOfFlow', file /Users/admin/trunk/mozilla/layout/base/../generic/nsPlaceholderFrame.h, line 133

###!!! ASSERTION: null ptr: 'nsnull != aFrame', file /Users/admin/trunk/mozilla/layout/generic/nsFrameList.cpp, line 114

###!!! ASSERTION: Must have the out of flow in some child list: 'found', file /Users/admin/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 5008

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase showing weird frame tree

So I can reproduce the crash on Linux... and if I just create the DOM that we should end up with statically, I see all sorts of stuff hanging out on overflow lists.  That doesn't seem right to me...

roc, any idea what's happening during reflow here?

Comment 6

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

I only see one text frame hanging in the overflow-list of the float, contianing "W W Waaaaa", but that may be because of font size differences. Anyway that's definitely a bad bug, something is going wrong with the combination of column balancing and floats ... somehow the last column is saying it's complete when the float actually isn't.

I have to check out of the hotel now, but I'll look into this more later.

Comment 7

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

OK, the problem is that the float is a below-current-line float and when nsBlockReflowState::PlaceBelowCurrentLineFloats places an incomplete float, it doesn't propagate incompleteness up the call chain to signal the parent is incomplete.

Comment 8

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

Attachment: Simple fix

This fixes the leftover overflow and the crashes. There's an error in the computation of the available height when we reflow a float in a block which has border or padding, because we look at the wrong box edge.

Comment 9

[Jesse Ruderman]

It scares me that memory safety depends on width/height arithmetic being correct here.  Can you file a followup bug on fixing that?

Comment 10

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

The crashes happen when a frame gets left in an overflow-list and then gets removed from the frame tree altogether (although it's still linked as the next-in-flow for some other frame). We should be able to prevent this, so that leaving frames in an overflow-list means stuff is not displayed but otherwise doesn't cause problems.

Comment 11

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

Comment on attachment 245989 [details] [diff] [review]
Simple fix

r+sr=dbaron, but please make sure there's a bug filed on the other issue

Comment 12

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

Attachment: defense in depth

Here's a fix that prevents crashes even without fixing the geometry here.

Here's the situation:
-- Float placeholder continuation gets pushed to an overflow list when there isn't enough room in the columns to fit all of the float given a height constraint on all columns
-- We reflow the columns again, reflowing the last column with unconstrained height this time (happens as part of the column balancing strategy)
-- In DrainOverflowLines for the last column, we can't pull back the placeholder continuation because we already a placeholder for that float in this block frame. So it goes to our mOverflowPlaceholders

Then there are two things that can go wrong. #1:
-- Some other bug causes the float to not reflow completely, so the placeholder continuation is not destroyed but stays in mOverflowPlaceholders
-- In nsBlockFrame::Reflow, because this is an unconstrained height reflow we don't look at mOverflowPlaceholders to see if there are frames to be dealt with, we assume it's empty. So the frames just drop out of the frame tree. Bad.

#2:
-- The float continuation was pushed up from some descendant block because the descendant block completed but there was still float content that didn't fit in the height constraint
-- Now the original float completes and we try to delete the float continuation. nsBlockFrame::DoRemoveFrame looks up a frame property to find the currently active mOverflowPlaceholders list where the frame is living temporarily. But because the reflow is unconstrained-height, nsBlockReflowState's constructor didn't set up the frame property that poitns to mOverflowPlaceholders.
-- Frame is not removed, hilarity ensues.

The patch fixes #1 by ensuring that we always deal with mOverflowPlaceholders frames even in unconstrained-height reflows. It fixed #2 by setting up the property for mOverflowPlaceholders if that list is non-empty or it might become nonempty because height is constrained.

Comment 13

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

Just to be clear: we should take both patches.

Comment 14

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

Comment on attachment 246106 [details] [diff] [review]
defense in depth

r+sr=dbaron (without looking too closely)

Comment 15

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

Checked in attachment 246106 [details] [diff] [review]. I will wait for a while before checking in attachment 245989 [details] [diff] [review] so that QA can see the effects of the patches independently.

Comment 16

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

Checked in attachment 245989 [details] [diff] [review].

Comment 17

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

Comment on attachment 246106 [details] [diff] [review]
defense in depth

this should fix a branch crash

Comment 18

[Daniel Veditz [:dveditz]]

You say we should take both patches, does that mean "simple fix" needs branch approvals as well?

Comment 19

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

"Simple fix" is not needed to fix the crash, so I think we should not take it on branch.

Comment 20

[Jay Patel [:jay]]

Comment on attachment 246106 [details] [diff] [review]
defense in depth

Approved for both branches, a=jay for drivers.  Let's just land what we need to fix the crash, as roc mentioned.

Comment 21

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

fixed on branches

Comment 22

[juan becerra [:juanb]]

Verified using smallish and partially reduced test cases. Application no longer crashes.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.10pre) Gecko/20070112 Firefox/1.5.0.10pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2pre) Gecko/20070116 BonEcho/2.0.0.2pre

Comment 23

[Daniel Veditz [:dveditz]]

Jesse: can I remove the security-sensitive flag on this bug now?

Comment 24

[Jesse Ruderman]

Crashtests checked in.