Last Comment Bug 343293 - [FIX]Crash [@ nsLayoutUtils::GetFloatFromPlaceholder] using ::first-line, floats, caption and generated content
: [FIX]Crash [@ nsLayoutUtils::GetFloatFromPlaceholder] using ::first-line, flo...
Status: RESOLVED FIXED
[sg:critical?] null deref but maybe w...
: crash, regression, testcase, verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-30 17:48 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
4 users (show)
dveditz: blocking1.8.1.2+
dveditz: blocking1.8.0.10+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (617 bytes, application/xhtml+xml)
2006-06-30 17:49 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
backtrace (28.27 KB, text/plain)
2006-06-30 18:02 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Minimal-ish testcase that still crashes trunk (297 bytes, application/xhtml+xml)
2006-12-01 10:29 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
Trunk patch (8.06 KB, patch)
2006-12-01 11:51 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Branch patch (8.10 KB, patch)
2006-12-01 11:51 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
dveditz: approval1.8.1.2+
dveditz: approval1.8.0.10+
Details | Diff | Review
Trunk patch -w for ease of review (7.11 KB, patch)
2006-12-01 11:57 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: review+
roc: superreview+
Details | Diff | Review
Trunk patch that actually applies (8.08 KB, patch)
2006-12-04 12:33 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-06-30 17:48:07 PDT
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 [:mwargers] (not working for Mozilla) 2006-06-30 17:49:26 PDT
Created attachment 227776 [details]
Testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-06-30 18:02:16 PDT
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.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-01 10:29:24 PST
Created attachment 247197 [details]
Minimal-ish testcase that still crashes trunk
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-01 11:51:21 PST
Created attachment 247204 [details] [diff] [review]
Trunk patch
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-01 11:51:49 PST
Created attachment 247205 [details] [diff] [review]
Branch patch
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-01 11:57:45 PST
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?
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-12-03 23:18:26 PST
Instead of bailing out early, shouldn't ReconstructDocElementHierarchy actually create the frame tree?
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-03 23:19:42 PST
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 [:bz] (Out June 25-July 6) 2006-12-04 12:33:57 PST
Created attachment 247458 [details] [diff] [review]
Trunk patch that actually applies
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-04 12:36:39 PST
Fixed on trunk.
Comment 11 Daniel Veditz [:dveditz] 2006-12-14 15:33:12 PST
So is this a security bug? comment 6 makes it sound like a null-deref crash.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-14 21:37:02 PST
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] 2006-12-18 14:51:18 PST
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] 2006-12-18 14:51:23 PST
Comment on attachment 247205 [details] [diff] [review]
Branch patch

Approved for both branches, a=jay for drivers.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-18 20:41:50 PST
Fixed for 1.8.1.2 and 1.8.0.10.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-12-18 22:30:44 PST
(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 - use ni] 2007-01-11 17:20:44 PST
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 [:bz] (Out June 25-July 6) 2007-04-18 00:05:34 PDT
> Is there a bug filed on this?

Filed bug 377850.
Comment 19 Bob Clary [:bc:] 2009-04-24 11:08:03 PDT
crash tests landed
http://hg.mozilla.org/mozilla-central/rev/5c9e24b8e99c

Note You need to log in before you can comment on or make changes to this bug.