Last Comment Bug 359371 - Float in column disappears; crash [@ nsBlockFrame::IsFloatContainingBlock] [@ nsBlockFrame::ClearLineCursor]
: Float in column disappears; crash [@ nsBlockFrame::IsFloatContainingBlock] [@...
Status: VERIFIED FIXED
[sg:critical]
: crash, testcase, verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
Mentors:
Depends on:
Blocks: stirdom
  Show dependency treegraph
 
Reported: 2006-11-03 03:59 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
3 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
smallish testcase (746 bytes, text/html)
2006-11-03 04:07 PST, Jesse Ruderman
no flags Details
Another testcase showing weird frame tree (755 bytes, text/html)
2006-11-10 21:37 PST, Boris Zbarsky [:bz]
no flags Details
Simple fix (1.92 KB, patch)
2006-11-19 16:34 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
defense in depth (11.72 KB, patch)
2006-11-20 18:32 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
dbaron: superreview+
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review

Comment 1 Jesse Ruderman 2006-11-03 04:07:45 PST
Created attachment 244561 [details]
smallish testcase
Comment 4 Jesse Ruderman 2006-11-03 04:47:37 PST
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 [:bz] 2006-11-10 21:37:20 PST
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?
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-11-18 11:54:42 PST
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) 2006-11-18 14:54:49 PST
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) 2006-11-19 16:34:47 PST
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.
Comment 9 Jesse Ruderman 2006-11-19 17:15:18 PST
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) 2006-11-19 17:23:20 PST
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-7 (review requests must explain patch) 2006-11-20 15:58:23 PST
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) 2006-11-20 18:32:43 PST
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.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-11-20 18:37:06 PST
Just to be clear: we should take both patches.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-12-11 16:07:46 PST
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) 2006-12-11 17:07:40 PST
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) 2006-12-14 17:58:49 PST
Checked in attachment 245989 [details] [diff] [review].
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-12-14 18:00:06 PST
Comment on attachment 246106 [details] [diff] [review]
defense in depth

this should fix a branch crash
Comment 18 Daniel Veditz [:dveditz] 2006-12-18 15:14:02 PST
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) 2006-12-18 16:35:44 PST
"Simple fix" is not needed to fix the crash, so I think we should not take it on branch.
Comment 20 Jay Patel [:jay] 2006-12-20 15:19:15 PST
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) 2006-12-21 13:51:01 PST
fixed on branches
Comment 22 juan becerra [:juanb] 2007-01-16 11:30:51 PST
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] 2007-02-23 14:02:34 PST
Jesse: can I remove the security-sensitive flag on this bug now?
Comment 24 Jesse Ruderman 2007-12-15 21:40:23 PST
Crashtests checked in.

Note You need to log in before you can comment on or make changes to this bug.