Last Comment Bug 401042 - Crash [@ nsInlineFrame::ReflowFrames] with ::first-letter float, binding, pre and //
: Crash [@ nsInlineFrame::ReflowFrames] with ::first-letter float, binding, pre...
Status: VERIFIED FIXED
[sg:critical?] using freed frame
: crash, fixed1.8.1.21, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 All
P3 critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 5588
  Show dependency treegraph
 
Reported: 2007-10-24 17:49 PDT by Martijn Wargers [:mwargers]
Modified: 2015-04-16 14:14 PDT (History)
15 users (show)
roc: blocking1.9.1+
roc: blocking1.9-
dveditz: blocking1.9.0.6+
roc: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
asac: wanted1.8.1.x+
asac: blocking1.8.0.next+
asac: wanted1.8.0.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (397 bytes, application/xhtml+xml)
2007-10-24 17:49 PDT, Martijn Wargers [:mwargers]
no flags Details
Frame dump (2.12 KB, text/html)
2007-12-02 17:14 PST, Mats Palmgren (:mats)
no flags Details
WIP (4.02 KB, patch)
2007-12-02 17:15 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
crashtest.diff (2.42 KB, patch)
2008-11-10 18:00 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 2 (4.36 KB, patch)
2008-11-10 18:46 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 3 (2.62 KB, patch)
2008-11-12 10:53 PST, Mats Palmgren (:mats)
roc: review+
roc: superreview+
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review
for 1.8 (2.63 KB, patch)
2009-01-04 18:10 PST, Alexander Sack
dveditz: approval1.8.1.next+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description User image Martijn Wargers [:mwargers] 2007-10-24 17:49:01 PDT
Created attachment 286096 [details]
testcase

See testcase which crashes current trunk and branch builds on load.
Since this also crashes on branch, I'm marking it security sensitive for now.

http://crash-stats.mozilla.com/report/index/4bf28766-828f-11dc-af09-001a4bd43ed6
0  	nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&)  	 mozilla/layout/generic/nsInlineFrame.cpp:460
1 	nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	mozilla/layout/generic/nsInlineFrame.cpp:394
2 	nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, int&) 	mozilla/layout/generic/nsLineLayout.cpp:883
3 	nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) 	mozilla/layout/generic/nsBlockFrame.cpp:3573
4 	nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, int*, LineReflowStatus*, int) 	mozilla/layout/generic/nsBlockFrame.cpp:3393
5 	nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, int*) 	mozilla/layout/generic/nsBlockFrame.cpp:3237
6 	nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, int*) 	mozilla/layout/generic/nsBlockFrame.cpp:2269
7 	nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) 	mozilla/layout/generic/nsBlockFrame.cpp:1854
8 	nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	mozilla/layout/generic/nsBlockFrame.cpp:940
9 	nsBlockReflowContext::ReflowBlock(nsRect const&, int, nsCollapsingMargin&, int, int, nsMargin&, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) 	mozilla/layout/generic/nsBlockReflowContext.cpp:339
10 	nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, int*) 	mozilla/layout/generic/nsBlockFrame.cpp:2973
etc..
Comment 1 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2007-10-25 20:54:39 PDT
I get "StealFrame failure!"
Comment 2 User image fantasai 2007-11-16 21:39:04 PST
I get  "ASSERTION: bad parent frame pointer: 'kid->GetParent() == (nsIFrame*)this', file /home/fantasai/moz/mozilla/layout/generic/nsContainerFrame.cpp, line 1497" when I try to print a frametree right before StealFrame. So I don't think the failure is StealFrame's fault: somebody's screwed up the frame tree.
Comment 3 User image Mats Palmgren (:mats) 2007-12-02 17:12:03 PST
Also crashes Firefox 2.0.0.11 on Linux
Comment 4 User image Mats Palmgren (:mats) 2007-12-02 17:14:34 PST
Created attachment 291133 [details]
Frame dump

We are reflowing a frame tree that looks like this.
Note that the parent pointers for the float child next-in-flows
haven't been setup yet due the "lazy parent pointer" optimization.

What happens is this (roughly):
reflow the second line (0x1467b18)
  which reflows the placeholder
  and then reflows the float, which decides it's complete and calls
    DeleteNextInFlowChild() on the kid's next-in-flow (0x1459bb0).

But DeleteNextInFlowChild() requires correct parent pointers!
Comment 5 User image Mats Palmgren (:mats) 2007-12-02 17:15:50 PST
Created attachment 291135 [details] [diff] [review]
WIP

This fixes the crash, but also kills this optimization I guess.
Is there a way to avoid that, at least in most cases?
(I'll dig some more tomorrow)
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2007-12-02 17:40:08 PST
Killing that optimization would be very unfortunate...
Comment 7 User image Daniel Veditz [:dveditz] 2008-01-10 15:38:20 PST
Why wouldn't an sg:critical bug block the release? If it's not actually a security bug please let us know, otherwise please block on this one.
Comment 8 User image Mike Schroepfer 2008-01-18 12:45:42 PST
Roc - why did you minus this?
Comment 9 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-01-18 21:17:33 PST
Because I minused it in October and Mats identified it as sg:critical? in December. It should block now.
Comment 10 User image Damon Sicore (:damons) 2008-03-11 14:26:28 PDT
Do we need to make this blocking or not?  The comments seem to suggest this should block.
Comment 11 User image Jesse Ruderman 2008-09-07 12:43:26 PDT
Still crashes [@ nsInlineFrame::ReflowFrames] on trunk.

Mats, are you still working on this?
Comment 12 User image Brandon Sterne (:bsterne) 2008-11-06 11:39:58 PST
I am adding this to our "Top Security Bugs" list.  Please treat this as a top priority.
Comment 13 User image Mats Palmgren (:mats) 2008-11-10 18:00:35 PST
Created attachment 347427 [details] [diff] [review]
crashtest.diff
Comment 14 User image Mats Palmgren (:mats) 2008-11-10 18:46:44 PST
Created attachment 347433 [details] [diff] [review]
Patch rev. 2

The crash only occurs when we delete a next-in-flow that is a child of
a ns(Positioned)InlineFrame in reflow with "lazilySetParentPointer" set.
I think such next-in-flow frames can only occur for ::first-letter
(or possibly ::first-line ?) child frames.  I don't think ::first-line
is a problem since it seems to do all re-parenting up front and it
leaves InlineReflowState::mSetParentPointer false:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsInlineFrame.cpp?mark=876-890#865

At one point I also did ReparentFloatsForInlineChild() for the
next-in-flow but it seems overkill since they are always text frames
(we may want to revisit this in the future when we allow first-letter
to dig into child blocks -- which I think would lead to nested
first-letter frames).
Comment 15 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-11 13:39:22 PST
This looks good but I think it would be best to move this "fix lazy parent pointer" code out to nsLayoutUtils and give it a real spec and documentation.
Comment 16 User image Mats Palmgren (:mats) 2008-11-12 10:53:38 PST
Created attachment 347804 [details] [diff] [review]
Patch rev. 3

This is a different approach;  fix the parent for letter frame child
next-in-flows before reflowing the letter frame.  I think I prefer this
patch - it brings a little bit of first-letter knowledge into nsInlineFrame
rather than bringing nsInlineFrame internals into nsFirstLetterFrame.
(and it would be easier to add ReparentFloatsForInlineChild here too
if we need it in the future)

What do you think?
Comment 17 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-12 13:23:53 PST
Comment on attachment 347804 [details] [diff] [review]
Patch rev. 3

I like it. Probably should add an assertion that 'child' and its next-in-flows are text frames.
Comment 18 User image Brandon Sterne (:bsterne) 2008-12-04 10:35:26 PST
This has a reviewed patch, though I'm not sure roc's review comment was addressed.  Can we get this landed please, as it is one of our "Top Security Bugs"?
Comment 19 User image Mats Palmgren (:mats) 2008-12-07 13:07:03 PST
Added the suggested assertions:
http://hg.mozilla.org/mozilla-central/rev/18ae5e6af525

I'm holding the crashtest until Firefox 3.0.x is fixed.

-> FIXED
Comment 20 User image Martijn Wargers [:mwargers] 2008-12-08 09:08:06 PST
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081208 Minefield/3.2a1pre
Comment 21 User image Mats Palmgren (:mats) 2008-12-10 15:20:56 PST
Fixed for 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/35ba7840d002
Comment 22 User image Mats Palmgren (:mats) 2008-12-10 15:24:59 PST
Comment on attachment 347804 [details] [diff] [review]
Patch rev. 3

Medium risk. No performance impact.
Comment 23 User image Daniel Veditz [:dveditz] 2008-12-15 11:48:59 PST
Comment on attachment 347804 [details] [diff] [review]
Patch rev. 3

Approved for 1.9.0.6, a=dveditz for release-drivers.
Comment 24 User image Mats Palmgren (:mats) 2008-12-18 02:29:47 PST
Checked in on CVS trunk for 1.9.0.6:
mozilla/layout/generic/nsInlineFrame.cpp 	3.303
Comment 25 User image Alexander Sack 2009-01-04 17:22:14 PST
1.8 and 1.8.0 branches crash on this.
Comment 26 User image Alexander Sack 2009-01-04 18:10:21 PST
Created attachment 355345 [details] [diff] [review]
for 1.8

backport of attachment 347804 [details] [diff] [review] (Patch rev. 3) with just s/nsGkAtoms::/nsLayoutAtoms::/
Comment 27 User image Alexander Sack 2009-01-04 18:32:08 PST
Comment on attachment 355345 [details] [diff] [review]
for 1.8

verified that it fixes crash on 1.8.0 and 1.8 branches
Comment 28 User image Al Billings [:abillings] 2009-01-05 17:01:06 PST
Verified for 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009010504 GranParadiso/3.0.6pre.
Comment 29 User image Daniel Veditz [:dveditz] 2009-01-16 11:31:20 PST
Comment on attachment 355345 [details] [diff] [review]
for 1.8

Approved for 1.8.1.21, a=dveditz for release-drivers.
Comment 30 User image Alexander Sack 2009-01-19 08:33:18 PST
Checked in on CVS 1.8 branch for 1.8.1.21 ...

/cvsroot/mozilla/layout/generic/nsInlineFrame.cpp,v  <--  nsInlineFrame.cpp
new revision: 3.241.4.7; previous revision: 3.241.4.6
done
Comment 31 User image Alexander Sack 2009-01-19 08:34:55 PST
Comment on attachment 355345 [details] [diff] [review]
for 1.8

a=asac for 1.8.0 branch
Comment 32 User image Tony Chung [:tchung] 2009-01-22 20:38:25 PST
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090122 Shiretoko/3.1b3pre

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