Last Comment Bug 383129 - [FIX]Crash [@ nsContentUtils::ContentIsDescendantOf] with counters, mathml and treerow
: [FIX]Crash [@ nsContentUtils::ContentIsDescendantOf] with counters, mathml an...
Status: VERIFIED FIXED
[sg:low?] null-deref, possible memory...
: crash, testcase, verified1.8.0.14, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla1.9alpha8
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks: randomclasses
  Show dependency treegraph
 
Reported: 2007-06-04 06:39 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
11 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.14+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (775 bytes, application/xhtml+xml)
2007-06-04 06:39 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Proposed fix (1.42 KB, patch)
2007-06-17 17:41 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
better fix? (1.83 KB, patch)
2007-07-02 19:11 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.5-
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.13-
dveditz: approval1.8.0.14+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-06-04 06:39:08 PDT
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 Jesse Ruderman 2007-06-04 10:38:33 PDT
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 2007-06-16 18:03:33 PDT
Bug 384728 has a similar stack trace, but involves <svg:use> instead of mathml and trees.
Comment 3 Boris Zbarsky [:bz] 2007-06-17 17:41:26 PDT
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...
Comment 4 Boris Zbarsky [:bz] 2007-06-17 17:42:34 PDT
Note that I doubt the mathml matters that much, by the way.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-06-17 21:53:18 PDT
Wouldn't it be more robust here to check the container's frame type? Or something like that?
Comment 6 Boris Zbarsky [:bz] 2007-06-17 22:03:36 PDT
The point is that some of these containers just never have frames...  I guess we could check that too.
Comment 7 Boris Zbarsky [:bz] 2007-06-18 21:22:45 PDT
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) (Exited; email my personal email if necessary) 2007-06-20 17:06:08 PDT
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] 2007-06-25 10:14:41 PDT
This testcase looks like a null deref in 2.0.0.4 (debug), is this an exploitable situation?
Comment 10 Boris Zbarsky [:bz] 2007-06-28 09:58:50 PDT
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] 2007-06-28 13:04:28 PDT
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 2007-06-28 13:52:03 PDT
Those assertions also fire in bug 385866.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-02 19:11:05 PDT
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.
Comment 14 Boris Zbarsky [:bz] 2007-07-03 12:38:43 PDT
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) (Exited; email my personal email if necessary) 2007-07-03 20:22:22 PDT
checked in.
Comment 16 Daniel Veditz [:dveditz] 2007-07-06 15:28:08 PDT
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] 2007-07-11 17:35:03 PDT
Comment on attachment 270669 [details] [diff] [review]
better fix?

tree closing early, non-blocker to next release.
Comment 18 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-07-19 14:29:00 PDT
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] 2007-08-07 11:35:29 PDT
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] 2007-08-29 15:35:16 PDT
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) (Exited; email my personal email if necessary) 2007-09-11 18:53:50 PDT
checked in on branches.
Comment 22 Carsten Book [:Tomcat] - PTO-back Sept 4th 2007-09-13 07:20:20 PDT
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] 2007-12-10 16:49:11 PST
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:] 2009-04-24 10:45:40 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/92419627df02

Note You need to log in before you can comment on or make changes to this bug.