397 bytes, application/xhtml+xml
2.12 KB, text/html
2.42 KB, patch
|Details | Diff | Splinter Review|
2.62 KB, patch
|Details | Diff | Splinter Review|
2.63 KB, patch
Alexander Sack: approval1.8.0.next+
|Details | Diff | Splinter Review|
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..
I get "StealFrame failure!"
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.
Also crashes Firefox 184.108.40.206 on Linux
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!
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)
Killing that optimization would be very unfortunate...
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.
Roc - why did you minus this?
Because I minused it in October and Mats identified it as sg:critical? in December. It should block now.
Do we need to make this blocking or not? The comments seem to suggest this should block.
Still crashes [@ nsInlineFrame::ReflowFrames] on trunk. Mats, are you still working on this?
I am adding this to our "Top Security Bugs" list. Please treat this as a top priority.
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).
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.
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 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.
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"?
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
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081208 Minefield/3.2a1pre
Fixed for 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/35ba7840d002
Comment on attachment 347804 [details] [diff] [review] Patch rev. 3 Medium risk. No performance impact.
Comment on attachment 347804 [details] [diff] [review] Patch rev. 3 Approved for 220.127.116.11, a=dveditz for release-drivers.
Checked in on CVS trunk for 18.104.22.168: mozilla/layout/generic/nsInlineFrame.cpp 3.303
1.8 and 1.8.0 branches crash on this.
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 on attachment 355345 [details] [diff] [review] for 1.8 verified that it fixes crash on 1.8.0 and 1.8 branches
Verified for 22.214.171.124 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:126.96.36.199pre) Gecko/2009010504 GranParadiso/3.0.6pre.
Comment on attachment 355345 [details] [diff] [review] for 1.8 Approved for 188.8.131.52, a=dveditz for release-drivers.
Checked in on CVS 1.8 branch for 184.108.40.206 ... /cvsroot/mozilla/layout/generic/nsInlineFrame.cpp,v <-- nsInlineFrame.cpp new revision: 220.127.116.11; previous revision: 18.104.22.168 done
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