383129 - [FIX]Crash [@ nsContentUtils::ContentIsDescendantOf] with counters, mathml and treerow

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes Mozilla within 200ms after load.
It also crashes branch builds, so marking security sensitive for now.

Talkback ID: TB32819385Z
nsContentUtils::ContentIsDescendantOf  [mozilla/content/base/src/nscontentutils.cpp, line 1149]
nsCounterList::RecalcAll  [mozilla/layout/base/nscountermanager.cpp, line 176]
RecalcDirtyLists  [mozilla/layout/base/nscountermanager.cpp, line 274]
nsBaseHashtable<nsStringHashKey,nsCOMPtr<nsIVariant>,nsIVariant *>::EnumerateRead  [mozilla/dist/include/xpcom/nsbasehashtable.h, line 188]
nsCounterManager::RecalcAll  [mozilla/layout/base/nscountermanager.cpp, line 281]
mozAutoDocUpdate::~mozAutoDocUpdate  [mozilla/dist/include/content/nsidocument.h, line 986]
nsGenericElement::RemoveChildAt  [mozilla/content/base/src/nsgenericelement.cpp, line 2628]
nsGenericElement::doRemoveChild  [mozilla/content/base/src/nsgenericelement.cpp, line 3242]
nsGenericElement::RemoveChild  [mozilla/content/base/src/nsgenericelement.cpp, line 2812]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2229]

Comment 1

[Jesse Ruderman]

Does the treerow element have to be root for this crash, or does it still crash if you wrap it all in a window?

Comment 2

[Jesse Ruderman]

Bug 384728 has a similar stack trace, but involves <svg:use> instead of mathml and trees.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

We're failing to remove frames here when removing a content node.

I think we probably want this fix on branches too...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note that I doubt the mathml matters that much, by the way.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wouldn't it be more robust here to check the container's frame type? Or something like that?

Comment 6

[Boris Zbarsky [:bzbarsky]]

The point is that some of these containers just never have frames...  I guess we could check that too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And I'm a little worried about performance impact of GetPrimaryFrameFor() calls in this code if we do serious population of listboxes.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Given that we'd be getting the same primary frame over and over again, wouldn't that be reasonably fast?

Comment 9

[Daniel Veditz [:dveditz]]

This testcase looks like a null deref in 2.0.0.4 (debug), is this an exploitable situation?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm not sure whether we can tweak the testcase to be exploitable.  It's possible that this is always a null deref... But I can't guarantee that, given the invariants this ends up violating.

Comment 11

[Daniel Veditz [:dveditz]]

On trunk I see the following assertions with this testcase

###!!! ASSERTION: null check on startContent should be sufficient to null check nodeContent as well, since if nodeContent is for the root, startContent (which is before it) must be too: 'nodeContent || !startContent', file c:/dev/fftrunk/mo
zilla/layout/base/nsCounterManager.cpp, line 145
###!!! ASSERTION: The possible descendant is null!: 'aPossibleDescendant', file c:/dev/fftrunk/mozilla/content/base/src/nsContentUtils.cpp, line 1143

Comment 12

[Jesse Ruderman]

Those assertions also fire in bug 385866.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better fix?

Boris, I like this better ... what do you think? I think this is more clear about what's going on.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 270669 [details] [diff] [review]
better fix?

Sure.  That works too.

Let's also get this in on the branches.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 270669 [details] [diff] [review]
better fix?

approved for 1.8.1.5 and 1.8.0.13, a=dveditz

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 270669 [details] [diff] [review]
better fix?

tree closing early, non-blocker to next release.

Comment 18

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071905 Minefield/3.0a7pre

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 270669 [details] [diff] [review]
better fix?

moving approval to 1.8.0.14 to match 1.8.1 branch landing

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 270669 [details] [diff] [review]
better fix?

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in on branches.

Comment 22

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.7 using  Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/2007091204 BonEcho/2.0.0.7pre

no crash on testcase - adding verified keyword

Comment 23

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

No crash on the testcase in comment 0 with neither Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre

nor

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre

Replacing fixed1.8.0.14 keyword with verified1.8.0.14

Comment 24

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/92419627df02