[FIX]Crash [@ nsFrameList::DestroyFrames] with evil testcase using position:fixed/absolute; and ::first-line

VERIFIED FIXED in mozilla1.9alpha1

Status

()

defect
P2
critical
VERIFIED FIXED
14 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

Tracking

(Depends on 1 bug, 4 keywords)

Trunk
mozilla1.9alpha1
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
See upcoming testcase, when hovering over the text, Mozilla crashes.
Also crashes Mozilla1.7, so no recent regression.

There is a table involved, so might be a table bug.

Talkback ID: 
nsFrameList::DestroyFrames  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsFrameList.cpp, line 138]
nsFrameList::DestroyFrame  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsFrameList.cpp, line 234]
DeletingFrameSubtree  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9747]
nsCSSFrameConstructor::ContentRemoved  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9897]
nsCSSFrameConstructor::RecreateFramesForContent  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 11481]
nsCSSFrameConstructor::RestyleElement  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10391]
nsCSSFrameConstructor::ProcessOneRestyle  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13203]
nsCSSFrameConstructor::ProcessPendingRestyles  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13255]
nsCSSFrameConstructor::RestyleEvent::HandleEvent  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13322]
SHELL32.dll + 0x520c24 (0x778b0c24)
(Reporter)

Comment 1

14 years ago
Posted file testcase
(Reporter)

Comment 2

14 years ago
Hmm, with a debug build (with frames diplay lists patch) I get this:
frame: Table(span)(1) (13BF181C) style: 13BF1728 {}
Wrong parent style context:  style: 13C52154 :-moz-table-outer {}
should be using:  style: 13C520BC {}

frame: TableOuter(span)(1) (13BF1680) style: 13C52154 :-moz-table-outer {}
Wrong parent style context:  style: 13C520BC {}
should be using:  style: 13BF1728 {}

frame: Table(span)(1) (13BF181C) style: 13BF1728 {}
Wrong parent style context:  style: 13C52154 :-moz-table-outer {}
should be using:  style: 13C520BC {}

frame: Placeholder(span)(1) (13BF18C0) style: 13C521F8 :-moz-non-element {}
Wrong parent style context:  style: 13C52154 :-moz-table-outer {}
should be using:  style: 13C520BC {}

frame: TableOuter(span)(1) (13BF1680) style: 13C52154 :-moz-table-outer {}
Wrong parent style context:  style: 13C520BC {}
should be using:  style: 13BF1728 {}

frame: Table(span)(1) (13BF181C) style: 13BF1728 {}
Wrong parent style context:  style: 13C52154 :-moz-table-outer {}
should be using:  style: 13C520BC {}

frame: Placeholder(span)(1) (13BF18C0) style: 13C521F8 :-moz-non-element {}
Wrong parent style context:  style: 13C52154 :-moz-table-outer {}
should be using:  style: 13C520BC {}

###!!! ASSERTION: unknown out of flow frame type: 'NS_STYLE_FLOAT_NONE != disp->
mFloats', file c:/mozilla/mozilla/layout/generic/nsHTMLReflowState.cpp, line 485

###!!! ASSERTION: Expected to solve for left: 'NS_AUTOOFFSET == kidReflowState.m
ComputedOffsets.left', file c:/mozilla/mozilla/layout/generic/nsAbsoluteContaini
ngBlock.cpp, line 580
###!!! ASSERTION: unknown out of flow frame type: 'NS_STYLE_FLOAT_NONE != disp->
mFloats', file c:/mozilla/mozilla/layout/generic/nsHTMLReflowState.cpp, line 485

###!!! ASSERTION: Expected to solve for left: 'NS_AUTOOFFSET == kidReflowState.m
ComputedOffsets.left', file c:/mozilla/mozilla/layout/generic/nsAbsoluteContaini
ngBlock.cpp, line 580

But the text doesn't show up (and so you can't crash).

Comment 3

14 years ago
Crashes Linux trunk too.
OS: Windows XP → All
> But the text doesn't show up (and so you can't crash).

Shows up fine in a trunk debug build.  Sounds like a bug in the frame display list patch?
(Reporter)

Comment 5

14 years ago
Oops, forgot about that patch. Yes, could very well be a bug in the frame display lists patch.
OK, so the first issue I run into when loading that testcase is that the style context parentage for the table outer frame is screwed up due to bug 141265.  I strongly suspect all the rest is just corollaries.

David, do you think it's worth trying to hack ReParentStyleContext to deal with table outer frames?  Or should we just try to fix bug 141265?  At least in part, this is a matter of whether we want this on branches; we _are_ ending up with virtual calls on deleted objects here.  :(
Depends on: 141265
Posted file stack
Posted file Frame tree
The crash is caused by DoDeletingFrameSubtree().
The patch for bug 310638 fixes the crash.

Still, this frame tree is broken, we have a table frame
on an Absolute-list that has the OUT_OF_FLOW bit,
but isn't float/abs/fixed.
Depends on: 310638
I think it's worth taking this on branches and working on something better on trunk....
Attachment #207659 - Flags: superreview?(dbaron)
Attachment #207659 - Flags: review?(dbaron)
(Reporter)

Comment 10

14 years ago
Posted file testcase2
This is something similar as the first testcase, only this one is not fixed by the patch from bug 310638.
(Reporter)

Comment 11

14 years ago
Ok, the crash in testcase2 also seems to be fixed with the "Yep, this fixes all the asserts and the crash" patch.
(Assignee)

Updated

14 years ago
Blocks: 322652
(Assignee)

Updated

14 years ago
Blocks: 316026
(Assignee)

Updated

14 years ago
Blocks: 322689
(Assignee)

Updated

14 years ago
No longer blocks: 322689
Comment on attachment 207659 [details] [diff] [review]
Yep, this fixes all the asserts and the crash

r+sr=dbaron, if you:
 * rename ReParentStyleContext to DoReparentStyleContext or something like that
 * make ReParentStyleContext a wrapper for DoReparentStyleContext that asserts that the initial caller to ReParentStyleContext doesn't have a non-child provider.  (Could be inlined.)
since otherwise this would make a class of cases where ReParentStyleContext would be a no-op with no assertions.

More seriously, though, it seems like this patch causes the aNewParentContext argument to be ignored!  If it really can be removed, it should be.

And you should probably also assert in DoReparentStyleContext in the case where the new context and old context are the same?  Either that, or handle that case in ReparentStyleContext (the wrapper).  Such an assertion is likely to fire in most cases where providerFrame ends up null, but I think it should.

Have you tested the CSS1 test suite tetscases for :first-line?
Attachment #207659 - Flags: superreview?(dbaron)
Attachment #207659 - Flags: superreview+
Attachment #207659 - Flags: review?(dbaron)
Attachment #207659 - Flags: review+
Posted patch Updated to comments (obsolete) — Splinter Review
Attachment #207659 - Attachment is obsolete: true
Posted patch Same as diff -w (obsolete) — Splinter Review
David, do you think this is safe for 1.8.0.x?  And do you think we should take this for 1.7.x branches?
Attachment #214279 - Flags: superreview?(dbaron)
Attachment #214279 - Flags: review?(dbaron)
Attachment #214279 - Flags: approval-branch-1.8.1?(dbaron)
Oh, I did test this on the CSS1 and CSS Selectors :first-line tests.
Assignee: nobody → bzbarsky
Priority: -- → P2
Summary: Crash [@ nsFrameList::DestroyFrames] with evil testcase using position:fixed/absolute; and ::first-line → [FIX]Crash [@ nsFrameList::DestroyFrames] with evil testcase using position:fixed/absolute; and ::first-line
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 214279 [details] [diff] [review]
Same as diff -w

This is probably pretty safe since ReParentStyleContext is used so rarely (right?), and the current incarnation is so broken.
Attachment #214279 - Flags: superreview?(dbaron)
Attachment #214279 - Flags: superreview+
Attachment #214279 - Flags: review?(dbaron)
Attachment #214279 - Flags: review+
Attachment #214279 - Flags: approval-branch-1.8.1?(dbaron)
Attachment #214279 - Flags: approval-branch-1.8.1+
But I'd say give up the #ifdef DEBUG nightmare in nsInlineFrame.cpp and just pass the extra parameter all the time.
> is used so rarely (right?)

It's only used for first-line, first-letter, and fieldset on trunk and those plus <button> and col that's a child of an anon colgroup on branch..... This last is bogus and got removed on trunk.

I'll post updated patches for trunk and branch.
Posted patch Trunk patchSplinter Review
Attachment #214278 - Attachment is obsolete: true
Attachment #214279 - Attachment is obsolete: true
I think this is worth considering for the next security release.  Risk is "some", but I've done a decent amount of testing of the things this patch touches, I think.  This does prevent a deleted-frame-deref crash.
Attachment #215198 - Flags: approval1.8.0.3?
Fixed on trunk and 1.8 branch.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Verified FIXED using trunk SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060316 SeaMonkey/1.5a with:

https://bugzilla.mozilla.org/attachment.cgi?id=207493
--and--
https://bugzilla.mozilla.org/attachment.cgi?id=207806

as testcases.  No crashes.
Status: RESOLVED → VERIFIED
Flags: blocking1.8.0.3?

Updated

13 years ago
Flags: blocking1.8.0.3? → blocking1.8.0.3+
(Reporter)

Updated

13 years ago
Depends on: 334602
Comment on attachment 215198 [details] [diff] [review]
1.8 branch patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #215198 - Flags: approval1.8.0.3? → approval1.8.0.3+
Fixed on 1.8.0 branch.
Keywords: fixed1.8.0.3
Also verified on:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Verified with a 1.8 FF branch build.
(Assignee)

Updated

13 years ago
Depends on: 350444
Crash Signature: [@ nsFrameList::DestroyFrames]
You need to log in before you can comment on or make changes to this bug.