391894 - "ASSERTION: Out of flow frame doesn't have the expected parent" and more and hang with -moz-column, float, :first-line

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (might hang Firefox when loaded)

Loading the testcase triggers:

###!!! ASSERTION: Out of flow frame doesn't have the expected parent: 'outOfFlowFrame->GetParent() == this', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 6404

###!!! ASSERTION: Parent not consistent with exepectations: 'aOldParent == aFrame->GetParent()', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 592

###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aNextSibling', file /Users/jruderman/trunk/mozilla/layout/base/../generic/nsIFrame.h, line 800


Variations on the testcase trigger:

###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockReflowState.cpp

###!!! ASSERTION: loop in frame list.  This will probably hang soon.: 'Error', file /Users/jruderman/trunk/mozilla/layout/generic/nsFrameList.cpp

Comment 1

[Jesse Ruderman]

A related testcase crashes dereferencing 0xddddde01.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch v1 (call CollectFloats on overflowList)

This patch fixes the "Out of flow frame doesn't have the expected parent" assertion and the hang.

The issue was this:

  When we're breaking the "Body" frame to make the second column, we call CollectFloats, which recursively looks for placeholders, and then moves their corresponding floats onto the OutOfFlowFrame list.  That list is important because it lets these floats follow their placeholders into the the second column.  (this list is called "OverflowOutOfFlow-list" in frame dumps)
  
  This process was failing in our testcase, though, because when we call CollectFloats in preparation for making the second column, the Placeholder is in an Overflow-list, as a result of the "first-line" CSS property.  This means that CollectFloats misses the placeholder entirely, and so its float doesn't get moved to an OutOfFlowFrame list.

  Then we end up with the second column having a placeholder whose float is back in the first column, which is bad.

  The actual "Out of flow frame doesn't have the expected parent" assertion was getting hit when we called CollectFloats on a frame in the second column, to prepare for making the third column.  At that point, we *do* run across the placeholder in the second column, but its float is back in the first column, and that triggers the assertion.

This patch fixes this whole process by making CollectFloats make recursive calls not just on its children, but also on its overflow-list.

Comment 3

[Daniel Holbert [:dholbert]]

btw: patch v1 passes layout reftests without causing any new assertion-failures

Comment 4

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

Comment on attachment 280783 [details] [diff] [review]
patch v1 (call CollectFloats on overflowList)

You should add a comment explaining that even though you're calling CollectFloats on aFrame's overflow-list, you're passing down aFromOverflow unchanged because we haven't moved out of this frame's regular-children subtree.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch v1a (added comment)

Added the comment below (and fixed s/aHead/aList in the function comment for CollectFloats)

roc, how's this sound?
>      // Note: Even though we're calling CollectFloats on aFrame's overflow
>      // list, we'll pass down aFromOverflow unchanged because we're still
>      // traversing the normal-child subtree of the 'this' frame.

Comment 6

[Daniel Holbert [:dholbert]]

oops, meant "regular-children", not normal-child.

New version (will change when I commit):
     // Note: Even though we're calling CollectFloats on aFrame's
     // overflow-list, we'll pass down aFromOverflow unchanged because we're
     // still traversing the regular-children subtree of the 'this' frame.

Comment 7

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

Yeah, that's great.

Comment 8

[Daniel Holbert [:dholbert]]

Patch v1a checked in at 1:18

Checking in nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.868; previous revision: 3.867
done

Comment 9

[Jesse Ruderman]

Crashtest checked in.