343293 - [FIX]Crash [@ nsLayoutUtils::GetFloatFromPlaceholder] using ::first-line, floats, caption and generated content

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P1)

Description

[Martijn Wargers (dead)]

See upcoming testcase which crashes Mozilla upon load.
It doesn't crash Mozilla1.7, but it crashes in Firefox1.5.0.4 (that's why I'm marking it security sensitive for now).
I haven't looked for a regression range for now, I don't have the necessary builds on this computer, I could look for the regression range when I am at home.

Talkback ID: TB20463441M
nsLayoutUtils::GetFloatFromPlaceholder

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: backtrace

Backtrace from debug build.
Some assertions:
###!!! ASSERTION: Reparenting something that has no usable parent? Shouldn't hap
pen!: 'Not Reached', file c:/mozilla/mozilla/layout/base/nsFrameManager.cpp, lin
e 916
This is bug 334602.

###!!! ASSERTION: Can only remove a singleton element: '!aElement->mNext', file
c:/mozilla/mozilla/layout/generic/nsLineBox.cpp, line 941
This is bug 342322 (there is a patch which removes the assertion).

###!!! ASSERTION: Null out-of-flow for placeholder?: 'outOfFlow', file c:/mozill
a/mozilla/layout/base/../generic/nsPlaceholderFrame.h, line 127
I've added the stack for that assertion.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal-ish testcase that still crashes trunk

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch -w for ease of review

This one was fun!  What happened was that we detected an {ib} situation and decided to blow away the containing block.  So in WipeContainingBlock we called CleanupFrameReferences on aFrameList.  But one of the frames in that list was a pseudo outer table (since the caption wants one), and its mContent was of course the root content of the document.  So we blew away the primary frame mapping for the root content.

Then we go to actually reframe, realize we're looking at the root content, and end up in ReconstructDocElementHierarchy(Internal).  We look up the primary frame for the root, get nothing (since the mapping is gone and one can't find that frame by walking the frame tree starting at the parent, since there's no parent), so we blow away the primary frame and placeholder maps, null out the out-of-flow pointers in all placeholders, and then bail out(!).  The result is that we have a placeholder with a null out of flow pointer in the tree, so when we reflow we crash.

This patch fixes things so we don't incorrectly blow away the root's primary frame mapping.  But maybe we should also add an early return in ReconstructDocElementHierarchy when there's no frame for the root content?

Comment 7

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

Instead of bailing out early, shouldn't ReconstructDocElementHierarchy actually create the frame tree?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, the problem is that there _is_ a frame tree there... ReconstructDocElementHierarchy just can't find it.  Without this patch, that is.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch that actually applies

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 11

[Daniel Veditz [:dveditz]]

So is this a security bug? comment 6 makes it sound like a null-deref crash.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I _think_ this is always a null-deref crash, but we're violating all sorts of frame tree invariants in this case, so if we don't hit the null-deref we could easily crash in truly nasty ways...  I can't guarantee that that won't happen.  :(

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 247205 [details] [diff] [review]
Branch patch

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 14

[Jay Patel [:jay]]

Comment on attachment 247205 [details] [diff] [review]
Branch patch

Approved for both branches, a=jay for drivers.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.1.2 and 1.8.0.10.

Comment 16

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

(In reply to comment #6)
> This patch fixes things so we don't incorrectly blow away the root's primary
> frame mapping.  But maybe we should also add an early return in
> ReconstructDocElementHierarchy when there's no frame for the root content?

Is there a bug filed on this?

Comment 17

[Marcia Knous [:marcia]]

verified fixed on the 1.8 branch using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.2pre) Gecko/20070111 BonEcho/2.0.0.2pre. Neither of Martijn's testcases crash. Adding verified keyword.

Also verified on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.10pre) Gecko/20070111 Firefox/1.5.0.10pre. Same results as with the 1.8 branch, no crash. Adding verified keyword.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Is there a bug filed on this?

Filed bug 377850.

Comment 19

[Bob Clary [:bc] (inactive)]

crash tests landed
http://hg.mozilla.org/mozilla-central/rev/5c9e24b8e99c