The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9alpha8

Status

()

Core
Layout
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: roc)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.9alpha8
x86
Windows XP
crash, testcase, verified1.8.0.14, verified1.8.1.8
Points:
---
Bug Flags:
blocking1.8.1.8 +
wanted1.8.1.x +
blocking1.8.0.14 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low?] null-deref, possible memory manglement if it doesn't crash, crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 267142 [details]
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

10 years ago
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

10 years ago
Bug 384728 has a similar stack trace, but involves <svg:use> instead of mathml and trees.
Created attachment 268727 [details] [diff] [review]
Proposed fix

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

I think we probably want this fix on branches too...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #268727 - Flags: superreview?(roc)
Attachment #268727 - Flags: review?(roc)
Attachment #268727 - Flags: approval1.8.1.5?
Attachment #268727 - Flags: approval1.8.0.13?
Note that I doubt the mathml matters that much, by the way.
Flags: in-testsuite?
Summary: Crash [@ nsContentUtils::ContentIsDescendantOf] with counters, mathml and treerow → [FIX]Crash [@ nsContentUtils::ContentIsDescendantOf] with counters, mathml and treerow
Target Milestone: --- → mozilla1.9alpha6
Wouldn't it be more robust here to check the container's frame type? Or something like that?
The point is that some of these containers just never have frames...  I guess we could check that too.
And I'm a little worried about performance impact of GetPrimaryFrameFor() calls in this code if we do serious population of listboxes.
Given that we'd be getting the same primary frame over and over again, wouldn't that be reasonably fast?
Whiteboard: needs r/sr=roc
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
This testcase looks like a null deref in 2.0.0.4 (debug), is this an exploitable situation?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
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.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
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

10 years ago
Those assertions also fire in bug 385866.
Whiteboard: needs r/sr=roc → needs r/sr=roc .
Whiteboard: needs r/sr=roc . → needs r/sr=roc ..
Created attachment 270669 [details] [diff] [review]
better fix?

Boris, I like this better ... what do you think? I think this is more clear about what's going on.
Attachment #270669 - Flags: superreview?(bzbarsky)
Attachment #270669 - Flags: review?(bzbarsky)
Whiteboard: needs r/sr=roc .. → [sg:low?] null-deref, possible memory manglement if it doesn't crash
Comment on attachment 270669 [details] [diff] [review]
better fix?

Sure.  That works too.

Let's also get this in on the branches.
Attachment #270669 - Flags: superreview?(bzbarsky)
Attachment #270669 - Flags: superreview+
Attachment #270669 - Flags: review?(bzbarsky)
Attachment #270669 - Flags: review+
Attachment #268727 - Attachment is obsolete: true
Attachment #268727 - Flags: superreview?(roc)
Attachment #268727 - Flags: review?(roc)
Attachment #268727 - Flags: approval1.8.1.5?
Attachment #268727 - Flags: approval1.8.0.13?
Assignee: bzbarsky → roc
Status: ASSIGNED → NEW
Attachment #270669 - Flags: approval1.8.1.5?
Attachment #270669 - Flags: approval1.8.0.13?
checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 270669 [details] [diff] [review]
better fix?

approved for 1.8.1.5 and 1.8.0.13, a=dveditz
Attachment #270669 - Flags: approval1.8.1.5?
Attachment #270669 - Flags: approval1.8.1.5+
Attachment #270669 - Flags: approval1.8.0.13?
Attachment #270669 - Flags: approval1.8.0.13+
Comment on attachment 270669 [details] [diff] [review]
better fix?

tree closing early, non-blocker to next release.
Attachment #270669 - Flags: approval1.8.1.6?
Attachment #270669 - Flags: approval1.8.1.5-
Attachment #270669 - Flags: approval1.8.1.5+
(Reporter)

Comment 18

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071905 Minefield/3.0a7pre
Status: RESOLVED → VERIFIED
Comment on attachment 270669 [details] [diff] [review]
better fix?

moving approval to 1.8.0.14 to match 1.8.1 branch landing
Attachment #270669 - Flags: approval1.8.0.14?
Attachment #270669 - Flags: approval1.8.0.13-
Attachment #270669 - Flags: approval1.8.0.13+
Flags: blocking1.8.1.7+
Flags: blocking1.8.0.14?
Comment on attachment 270669 [details] [diff] [review]
better fix?

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #270669 - Flags: approval1.8.1.7?
Attachment #270669 - Flags: approval1.8.1.7+
Attachment #270669 - Flags: approval1.8.0.14?
Attachment #270669 - Flags: approval1.8.0.14+
checked in on branches.
Keywords: fixed1.8.0.14, fixed1.8.1.7
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
Keywords: fixed1.8.1.7 → verified1.8.1.7
Group: security
Flags: blocking1.8.0.14? → blocking1.8.0.14+
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
Keywords: fixed1.8.0.14 → verified1.8.0.14

Comment 24

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/92419627df02
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsContentUtils::ContentIsDescendantOf]
You need to log in before you can comment on or make changes to this bug.