349185 - Crash [@ ReparentFrame] with moz-column-count, floats and display: table-row

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes Mozilla on load.
It also crashes 1.8 branch builds, so it doesn't seem to be a regression.

Talkback ID: TB22233772X
ReparentFrame   nsBlockFrame::HandleOverflowPlaceholdersOnPulledLine   nsBlockFrame::Reflow

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: original testcase

Comment 3

[Mats Palmgren (inactive)]

Attachment: trace + frame dump

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

When the nsBlockReflowContext destroys float continuations due to the
reflow being COMPLETE:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockReflowContext.cpp&rev=1.143&root=/cvsroot&mark=725,735#703

in some cases [the placeholders for] these continuations does not live in
the frame tree (including the overflowOutOfFlowsProperty list) but in
nsBlockReflowState::mOverflowPlaceholders which will then contain stale
placeholders and the crash occurs if the rest of the reflow looks at this
data, e.g. nsBlockFrame::HandleOverflowPlaceholdersOnPulledLine.

There is this comment:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&root=/cvsroot&mark=5601-5604#5568
which I guess is correct in a way, provided that no-one is using that
part of the block reflow state after that point.
After reading through the code and trying various ways to fix this, I get
the impression that there are several ways this can happen though.

This patch fixes the crash for the attached testcases, but I'm unsure
if it's the correct fix. I think there is at least one path through
nsLineLayout::ReflowFrame where a similar crash might happen if we're
unlucky.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineLayout.cpp&rev=3.238&root=/cvsroot&mark=1155,1163#1126

Comment 5

[Bernd]

I just asked Robert about a similiar thing in tables, isn't it wrong to mark a frame complete if nsBlockReflowState::mOverflowPlaceholders is not empty?

Comment 6

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

Blocks are more complicated, unfortunately. We mark the block complete when the regular in-flow content has all been reflowed. But there could still be float content that needs to be reflowed, and hence overflowing placeholders.

Comment 7

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

I think the basic idea of removing these destroyed floats' placeholders from blockReflowState's mOverflowPlaceholders, when a float completes and destroys its next-in-flows, is the right idea. I don't see how this patch does that though

Comment 8

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

Mats, are you still on this or do you want me to take it?

Comment 9

[Mats Palmgren (inactive)]

You're most welcome to :-)

Comment 10

[Johnny Stenback (:jst)]

FWIW, I don't see this crahs on the trunk (linux) any more, both testcases load w/o crashing here.

Comment 11

[Martijn Wargers (dead)]

In a 2006-12-07 build, it still crashes, not anymore in a 2006-12-08 build (it hangs there, in a 2006-12-30 build, it doesn't even hang anymore).
Marking worksforme?

Comment 12

[Johnny Stenback (:jst)]

Roc, you still have work to do here, even if this doesn't appear to crash any more?

Comment 13

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

No, I think I fixed this in bug 359371. Perhaps the reflow branch stopped the crash and bug 359371 stopped the hang.

Comment 14

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

This still happens on branch (since there's no reflow branch). Is it worth fixing there?

Comment 15

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

Did you check? bug 359371 is fixed on branch.

Comment 16

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

Attachment 234437 [details] crashes Firefox 2.0.0.2 for me.

Comment 17

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

s/2.0.0.2/2.0.0.3

Comment 18

[Daniel Veditz [:dveditz]]

The branch crash appears to be a null dereference. Is there an exploitable flaw here?