Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED

Status

()

Core
Layout: Floats
--
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: roc)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(4 attachments)

(Reporter)

Comment 1

11 years ago
Created attachment 244561 [details]
smallish testcase
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical] stirdom testcases
(Reporter)

Comment 4

11 years ago
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

11 years ago
Created attachment 245315 [details]
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?

Updated

11 years ago
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.
Created attachment 245989 [details] [diff] [review]
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.
Attachment #245989 - Flags: superreview?(dbaron)
Attachment #245989 - Flags: review?(dbaron)
(Reporter)

Comment 9

11 years ago
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+
Created attachment 246106 [details] [diff] [review]
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.
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
Last Resolved: 11 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 20

11 years ago
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+
fixed on branches
Keywords: fixed1.8.0.10, fixed1.8.1.2
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
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
Jesse: can I remove the security-sensitive flag on this bug now?
Whiteboard: [sg:critical] stirdom testcases → [sg:critical]
Group: security

Updated

10 years ago
Flags: blocking1.9? → in-testsuite?
(Reporter)

Comment 24

10 years ago
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.