Closed Bug 359371 Opened 13 years ago Closed 13 years ago

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

Categories

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

PowerPC
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: roc)

References

(Blocks 1 open bug)

Details

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

Crash Data

Attachments

(4 files)

Attached file smallish testcase
Whiteboard: [sg:critical] stirdom testcases
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
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?
Flags: blocking1.9?
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.
Assignee: nobody → roc
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.
Attached patch Simple fixSplinter Review
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.
Attachment #245989 - Flags: superreview?(dbaron)
Attachment #245989 - Flags: review?(dbaron)
It scares me that memory safety depends on width/height arithmetic being correct here.  Can you file a followup bug on fixing that?
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 on attachment 245989 [details] [diff] [review]
Simple fix

r+sr=dbaron, but please make sure there's a bug filed on the other issue
Attachment #245989 - Flags: superreview?(dbaron)
Attachment #245989 - Flags: superreview+
Attachment #245989 - Flags: review?(dbaron)
Attachment #245989 - Flags: review+
Attached patch defense in depthSplinter Review
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.
Attachment #246106 - Flags: superreview?(dbaron)
Attachment #246106 - Flags: review?(dbaron)
Just to be clear: we should take both patches.
Comment on attachment 246106 [details] [diff] [review]
defense in depth

r+sr=dbaron (without looking too closely)
Attachment #246106 - Flags: superreview?(dbaron)
Attachment #246106 - Flags: superreview+
Attachment #246106 - Flags: review?(dbaron)
Attachment #246106 - Flags: review+
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.
Checked in attachment 245989 [details] [diff] [review].
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 246106 [details] [diff] [review]
defense in depth

this should fix a branch crash
Attachment #246106 - Flags: approval1.8.1.2?
Attachment #246106 - Flags: approval1.8.0.10?
You say we should take both patches, does that mean "simple fix" needs branch approvals as well?
"Simple fix" is not needed to fix the crash, so I think we should not take it on branch.
Component: Layout: Floats → IPC
Component: IPC → Layout: Floats
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.
Attachment #246106 - Flags: approval1.8.1.2?
Attachment #246106 - Flags: approval1.8.1.2+
Attachment #246106 - Flags: approval1.8.0.10?
Attachment #246106 - Flags: approval1.8.0.10+
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
Status: RESOLVED → VERIFIED
Jesse: can I remove the security-sensitive flag on this bug now?
Whiteboard: [sg:critical] stirdom testcases → [sg:critical]
Group: security
Flags: blocking1.9? → in-testsuite?
Crashtests checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsBlockFrame::IsFloatContainingBlock] [@ nsBlockFrame::ClearLineCursor]
You need to log in before you can comment on or make changes to this bug.