The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Layout: Floats
P1
critical
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

(5 keywords)

Trunk
crash, regression, testcase, verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +
blocking1.8.0.10 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] null deref but maybe worse, crash signature)

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 227776 [details]
Testcase
(Reporter)

Comment 2

11 years ago
Created attachment 227778 [details]
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.
(Reporter)

Updated

11 years ago
Group: security
Created attachment 247197 [details]
Minimal-ish testcase that still crashes trunk
Created attachment 247204 [details] [diff] [review]
Trunk patch
Created attachment 247205 [details] [diff] [review]
Branch patch
Created attachment 247207 [details] [diff] [review]
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?
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #247207 - Flags: superreview?(roc)
Attachment #247207 - Flags: review?(roc)
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: Crash [@ nsLayoutUtils::GetFloatFromPlaceholder] using ::first-line, floats, caption and generated content → [FIX]Crash [@ nsLayoutUtils::GetFloatFromPlaceholder] using ::first-line, floats, caption and generated content
Attachment #247207 - Flags: superreview?(roc)
Attachment #247207 - Flags: superreview+
Attachment #247207 - Flags: review?(roc)
Attachment #247207 - Flags: review+
Instead of bailing out early, shouldn't ReconstructDocElementHierarchy actually create the frame tree?
Well, the problem is that there _is_ a frame tree there... ReconstructDocElementHierarchy just can't find it.  Without this patch, that is.
Created attachment 247458 [details] [diff] [review]
Trunk patch that actually applies
Attachment #247204 - Attachment is obsolete: true
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Attachment #247205 - Flags: approval1.8.1.2?
Attachment #247205 - Flags: approval1.8.0.10?
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.2?
So is this a security bug? comment 6 makes it sound like a null-deref crash.
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
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 on attachment 247205 [details] [diff] [review]
Branch patch

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #247205 - Flags: approval1.8.1.2?
Attachment #247205 - Flags: approval1.8.1.2+
Attachment #247205 - Flags: approval1.8.0.10?
Attachment #247205 - Flags: approval1.8.0.10+

Comment 14

10 years ago
Comment on attachment 247205 [details] [diff] [review]
Branch patch

Approved for both branches, a=jay for drivers.
Fixed for 1.8.1.2 and 1.8.0.10.
Flags: in-testsuite?
Keywords: fixed1.8.0.10, fixed1.8.1.2
(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?
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.
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
Whiteboard: [sg:critical?]
Group: security
Whiteboard: [sg:critical?] → [sg:critical?] null deref but maybe worse
> Is there a bug filed on this?

Filed bug 377850.

Comment 19

8 years ago
crash tests landed
http://hg.mozilla.org/mozilla-central/rev/5c9e24b8e99c
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsLayoutUtils::GetFloatFromPlaceholder]
You need to log in before you can comment on or make changes to this bug.