Closed
Bug 401042
Opened 17 years ago
Closed 16 years ago
Crash [@ nsInlineFrame::ReflowFrames] with ::first-letter float, binding, pre and //
Categories
(Core :: Layout, defect, P3)
Tracking
()
VERIFIED
FIXED
People
(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)
References
Details
(5 keywords, Whiteboard: [sg:critical?] using freed frame)
Crash Data
Attachments
(5 files, 2 obsolete files)
397 bytes,
application/xhtml+xml
|
Details | |
2.12 KB,
text/html
|
Details | |
2.42 KB,
patch
|
Details | Diff | Splinter Review | |
2.62 KB,
patch
|
roc
:
review+
roc
:
superreview+
dveditz
:
approval1.9.0.6+
|
Details | Diff | Splinter Review |
2.63 KB,
patch
|
dveditz
:
approval1.8.1.next+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
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..
Flags: blocking1.9?
I get "StealFrame failure!"
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
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.
Assignee | ||
Comment 3•17 years ago
|
||
Also crashes Firefox 2.0.0.11 on Linux
Flags: in-testsuite?
OS: Windows XP → All
Whiteboard: [sg:critical?] using freed frame
Version: Trunk → unspecified
Assignee | ||
Comment 4•17 years ago
|
||
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!
Assignee | ||
Comment 5•17 years ago
|
||
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)
Assignee: nobody → mats.palmgren
Status: NEW → ASSIGNED
Killing that optimization would be very unfortunate...
Comment 7•17 years ago
|
||
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.
Flags: blocking1.9- → blocking1.9?
Priority: -- → P2
Comment 8•17 years ago
|
||
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.
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Flags: wanted1.9.0.x+
Flags: wanted1.9+
Flags: blocking1.9-
Updated•17 years ago
|
Flags: tracking1.9+
Comment 10•17 years ago
|
||
Do we need to make this blocking or not? The comments seem to suggest this should block.
Comment 11•16 years ago
|
||
Still crashes [@ nsInlineFrame::ReflowFrames] on trunk.
Mats, are you still working on this?
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: P2 → P3
Comment 12•16 years ago
|
||
I am adding this to our "Top Security Bugs" list. Please treat this as a top priority.
Assignee | ||
Comment 13•16 years ago
|
||
Assignee | ||
Comment 14•16 years ago
|
||
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).
Attachment #291135 -
Attachment is obsolete: true
Attachment #347433 -
Flags: superreview?(roc)
Attachment #347433 -
Flags: review?(roc)
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.
Assignee | ||
Comment 16•16 years ago
|
||
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?
Attachment #347433 -
Attachment is obsolete: true
Attachment #347804 -
Flags: superreview?(roc)
Attachment #347804 -
Flags: review?(roc)
Attachment #347433 -
Flags: superreview?(roc)
Attachment #347433 -
Flags: review?(roc)
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.
Attachment #347804 -
Flags: superreview?(roc)
Attachment #347804 -
Flags: superreview+
Attachment #347804 -
Flags: review?(roc)
Attachment #347804 -
Flags: review+
Whiteboard: [sg:critical?] using freed frame → [sg:critical?] using freed frame [needs addressing review comments and landing]
Comment 18•16 years ago
|
||
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"?
Assignee | ||
Comment 19•16 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?] using freed frame [needs addressing review comments and landing] → [sg:critical?] using freed frame
Assignee | ||
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Updated•16 years ago
|
Flags: blocking1.9.0.6?
Reporter | ||
Comment 20•16 years ago
|
||
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081208 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Target Milestone: mozilla1.9.2a1 → ---
Updated•16 years ago
|
Flags: blocking1.9.0.6? → blocking1.9.0.6+
Assignee | ||
Comment 21•16 years ago
|
||
Fixed for 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/35ba7840d002
Keywords: fixed1.9.1
Assignee | ||
Comment 22•16 years ago
|
||
Comment on attachment 347804 [details] [diff] [review]
Patch rev. 3
Medium risk. No performance impact.
Assignee | ||
Updated•16 years ago
|
Attachment #347804 -
Flags: approval1.9.0.6?
Comment 23•16 years ago
|
||
Comment on attachment 347804 [details] [diff] [review]
Patch rev. 3
Approved for 1.9.0.6, a=dveditz for release-drivers.
Attachment #347804 -
Flags: approval1.9.0.6? → approval1.9.0.6+
Assignee | ||
Comment 24•16 years ago
|
||
Checked in on CVS trunk for 1.9.0.6:
mozilla/layout/generic/nsInlineFrame.cpp 3.303
Keywords: fixed1.9.0.6
Comment 25•16 years ago
|
||
1.8 and 1.8.0 branches crash on this.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next+
Comment 26•16 years ago
|
||
backport of attachment 347804 [details] [diff] [review] (Patch rev. 3) with just s/nsGkAtoms::/nsLayoutAtoms::/
Updated•16 years ago
|
Attachment #355345 -
Flags: approval1.8.1.next?
Attachment #355345 -
Flags: approval1.8.0.next?
Comment 27•16 years ago
|
||
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•16 years ago
|
||
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.
Keywords: fixed1.9.0.6 → verified1.9.0.6
Updated•16 years ago
|
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Comment 29•16 years ago
|
||
Comment on attachment 355345 [details] [diff] [review]
for 1.8
Approved for 1.8.1.21, a=dveditz for release-drivers.
Attachment #355345 -
Flags: approval1.8.1.next? → approval1.8.1.next+
Comment 30•16 years ago
|
||
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
Keywords: fixed1.8.1.21
Updated•16 years ago
|
Attachment #355345 -
Flags: approval1.8.0.next? → approval1.8.0.next+
Comment 31•16 years ago
|
||
Comment on attachment 355345 [details] [diff] [review]
for 1.8
a=asac for 1.8.0 branch
Comment 32•16 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
Updated•16 years ago
|
Group: core-security
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
Updated•14 years ago
|
Crash Signature: [@ nsInlineFrame::ReflowFrames]
You need to log in
before you can comment on or make changes to this bug.
Description
•