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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[Jesse Ruderman]

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

valgrind doesn't show anything unusual prior to the asserts, so I think the asserts are the thing to investigate

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

DEBUG_FRAME_LIST, however, gives an earlier assert

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: minimal testcase for DEBUG_FRAME_LIST assertions

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: partly simplified testcase for original bug (I think)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: fix bogus frame list assertions

This avoids construction of an nsFrameList that triggers the DEBUG_FRAME_LIST assertions (because its "first child" has a previous sibling)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: fix the crash

We need to consider the aFromOverflow parameter for pushed floats just like we do for regular ones.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/15ba1f799202
http://hg.mozilla.org/mozilla-central/rev/0cdee3bfea66

Comment 12

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100822 Minefield/4.0b5pre