Closed Bug 586806 Opened 9 years ago Closed 9 years ago

Crash [@ nsFrameList::RemoveFrame] with position: absolute and -moz-column-count

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- final+

People

(Reporter: martijn.martijn, Assigned: dbaron)

References

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(5 files)

Attached file testcase
See testcase, which crashes current trunk build after 100ms.

This appears to have regressed between 2010-08-04 and 2010-08-08. I guess a regression from bug 563584.

http://crash-stats.mozilla.com/report/index/768cd7ae-773e-4fe8-a6a7-453432100812
0  	xul.dll  	nsFrameList::RemoveFrame  	 layout/generic/nsFrameList.cpp:133
1 	xul.dll 	nsBlockFrame::CollectFloats 	layout/generic/nsBlockFrame.cpp:6688
2 	xul.dll 	nsBlockFrame::ReparentFloats 	layout/generic/nsBlockFrame.cpp:1703
3 	xul.dll 	nsBlockFrame::ReflowDirtyLines
blocking2.0: --- → final+
Assignee: nobody → dbaron
In a debug build, the crash is preceded by two assertion failures.

###!!! ASSERTION: Broken frame linkage: 'prevSibling && prevSibling->GetNextSibling() == aFrame', file layout/generic/nsFrameList.cpp, line 132

###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aNextSibling', file nsIFrame.h, line 964
valgrind doesn't show anything unusual prior to the asserts, so I think the asserts are the thing to investigate
DEBUG_FRAME_LIST, however, gives an earlier assert
The DEBUG_FRAME_LIST problem I reduced the testcase to is different from the actual bug; it's a bogus assertion:
    NS_ASSERTION(nsFrameList(aFrame, nsLayoutUtils::GetLastSibling(aFrame))
                   .GetLength() >= pushCount,
                 "Not enough frames to push");
which constructs a frame list whose first child has a previous sibling, which is not allowed.
The above testcase (and the original one) trigger a real DEBUG_FRAME_LIST assertion ("wrong list") pointing to what is likely the problem, which I think is a bug in CollectFloats.
This avoids construction of an nsFrameList that triggers the DEBUG_FRAME_LIST assertions (because its "first child" has a previous sibling)
Attachment #467297 - Flags: review?(roc)
We need to consider the aFromOverflow parameter for pushed floats just like we do for regular ones.
Attachment #467298 - Flags: review?(roc)
Note that what the fix is doing is basically making the two sides of the:
  if (outOfFlowFrame->GetStateBits() & NS_FRAME_IS_PUSHED_FLOAT)
a little more similar to each other, because in this case we had a pushed float that was not currently pushed, but had been pulled back and was now on the overflow floats list.

Perhaps I should be thinking about managing the NS_FRAME_IS_PUSHED_FLOAT bit more strictly, though, so we don't have this problem.
http://hg.mozilla.org/mozilla-central/rev/15ba1f799202
http://hg.mozilla.org/mozilla-central/rev/0cdee3bfea66
Status: NEW → RESOLVED
Closed: 9 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Verified fixed, using:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100822 Minefield/4.0b5pre
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsFrameList::RemoveFrame]
You need to log in before you can comment on or make changes to this bug.