413048 - Crash [@ nsFrameList::SortByContentOrder] with -moz-column, float

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading this testcase in a Mac trunk debug build triggers:

###!!! ASSERTION: unexpected flow: 'mFrames.ContainsFrame(nextInFlow)', file /Users/jruderman/trunk/mozilla/layout/generic/nsInlineFrame.cpp, line 470

###!!! ASSERTION: StealFrame failure: 'NS_SUCCEEDED(rv)', file /Users/jruderman/trunk/mozilla/layout/generic/nsContainerFrame.cpp, line 1092

Crash in nsIFrame::GetNextSibling, called by nsFrameList::SortByContentOrder, dereferencing 0xddddddfd.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

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

This is very nasty. Basically, inline frames have no protection against pulling a placeholder continuation into the same container as its prev-in-flow. Blocks have that protection in nsBlockFrame::HandleOverflowPlaceholdersForPulledFrame ... but I just now realized that that's broken for the case where aFrame is a span; we descend into the span and traverse its children, but later we assume that the frames we're looking at have a block frame as parent...

I wonder if it would be easier to just remove float continuations as we're planning to do.

Comment 3

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

I think we should just disable float breaking in columns. It's too fragile to fix up with any reasonable amount of effort. We should reform layout (at least by making placeholders not have continuation frames) and then revisit the issue.

Comment 4

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

Attachment: fix

This disables float breaking in columns by reflowing them with unrestricted height.

The change to nsBlockReflowState::AddFloat is the scary bit. We need this change because we don't want to mark the placeholder for a below-current-line float as incomplete, that triggers this crash. As far as I can tell, this code is only needed to stop the teardown of next-in-flows, so we only need the GetNextInFlow check --- which, conveniently, will now never be true for floats in columns.

The test 386147-1.html depends on floats breaking across columns so I just disabled it.

Comment 5

[Jesse Ruderman]

>-== 386147-1.html 386147-1-ref.html
>+fails == 386147-1.html 386147-1-ref.html # bug 413048

Please reference a new bug instead of this one.  I like being able to grep reftest.list for bug references and make sure they are all still open bugs.

Comment 6

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

Sounds reasonable. I filed bug 447660 for that.

Comment 7

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

Attachment: fix v2

Updated with that

Comment 8

[David Baron :dbaron:]

Comment on attachment 330973 [details] [diff] [review]
fix v2

r+sr=dbaron

Comment 9

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

Pushed 5d09fb3015d1.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080816113032 Minefield/3.1a2pre

Comment 11

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

Comment on attachment 330973 [details] [diff] [review]
fix v2

This should fix a number of bugs on branch.

Comment 12

[Samuel Sidler (old account; do not CC)]

Comment on attachment 330973 [details] [diff] [review]
fix v2

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 13

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

Checked into 1.9.0.

Comment 14

[Alexander Sack]

doesnt look like 1.8.0 is affected. if i am mistaken, please flip wanted or blocking flag.