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

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

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)

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

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

[Felix Miata]

Crashes Linux trunk too.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 5

[Martijn Wargers (dead)]

Oops, forgot about that patch. Yes, could very well be a bug in the frame display lists patch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 7

[Mats Palmgren (inactive)]

Attachment: stack

Comment 8

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Yep, this fixes all the asserts and the crash

I think it's worth taking this on branches and working on something better on trunk....

Comment 10

[Martijn Wargers (dead)]

Attachment: testcase2

This is something similar as the first testcase, only this one is not fixed by the patch from bug 310638.

Comment 11

[Martijn Wargers (dead)]

Ok, the crash in testcase2 also seems to be fixed with the "Yep, this fixes all the asserts and the crash" patch.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, I did test this on the CSS1 and CSS Selectors :first-line tests.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But I'd say give up the #ifdef DEBUG nightmare in nsInlineFrame.cpp and just pass the extra parameter all the time.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch patch

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk and 1.8 branch.

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 215198 [details] [diff] [review]
1.8 branch patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 24

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.0 branch.

Comment 25

[juan becerra [:juanb]]

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

Comment 26

[Frank Wein [:mcsmurf]]

Verified with a 1.8 FF branch build.