Last Comment Bug 502687 - GCGraphBuilder::AddNode crashes on OOM
: GCGraphBuilder::AddNode crashes on OOM
Status: VERIFIED FIXED
: verified1.9.0.18, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9.2
Assigned To: Blake Kaplan (:mrbkap)
:
:
Mentors:
: 481108 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-06 12:46 PDT by Blake Kaplan (:mrbkap)
Modified: 2010-02-28 15:22 PST (History)
11 users (show)
jst: blocking1.9.2+
dveditz: blocking1.9.0.18+
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.8+
.8-fixed


Attachments
Proposed fix (895 bytes, patch)
2009-07-06 12:57 PDT, Blake Kaplan (:mrbkap)
dbaron: review+
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2009-07-06 12:46:46 PDT
The code looks like:

    PtrToNodeEntry *e = static_cast<PtrToNodeEntry*>(PL_DHashTableOperate(&mPtrToNodeMap, s, PL_DHASH_ADD));
    PtrInfo *result;
    if (!e->mNode) {

but PL_DHASH_ADD returns NULL on OOM. The function already returns null in some cases, so callers should be mostly null safe. dbaron thinks that failure to add a node should result in leaks (as opposed to overreleasing).
Comment 1 Blake Kaplan (:mrbkap) 2009-07-06 12:57:41 PDT
Created attachment 387035 [details] [diff] [review]
Proposed fix

I don't know how to test for this, but it does compile!
Comment 2 David Baron :dbaron: ⌚️UTC-7 2009-07-06 13:07:49 PDT
Comment on attachment 387035 [details] [diff] [review]
Proposed fix

r=dbaron
Comment 3 Blake Kaplan (:mrbkap) 2009-07-16 13:08:39 PDT
http://hg.mozilla.org/mozilla-central/rev/b0a681cd9df5
Comment 4 Peter Van der Beken [:peterv] 2009-07-17 04:18:43 PDT
(In reply to comment #0)
> dbaron thinks that failure to add
> a node should result in leaks (as opposed to overreleasing).

I'm not convinced that that's true, XPConnect relies on the cycle collector traversing all of the XPConnect objects holding JS things. If we fail to add any of those, they will not get marked (whether they're live or not) and will be finalized. Something needs to signal this failure to XPConnect, so that it just marks all the JS things held by XPConnect objects. See also bug 460916.
Comment 5 Peter Van der Beken [:peterv] 2009-07-17 04:23:54 PDT
Actually, this might be fine. The key is that we won't try to unlink the XPConnect objects, so they will all mark their JS object after CC.
Comment 6 Jesse Ruderman 2009-08-19 21:39:11 PDT
*** Bug 481108 has been marked as a duplicate of this bug. ***
Comment 7 Peter Van der Beken [:peterv] 2009-12-16 10:48:32 PST
We should take this on branches, we've sometimes seen this cause crashes with the testcase in bug 533000 (the testcase probably triggers it faster because it manages to use up much more memory quickly).
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2009-12-16 11:44:02 PST
Yeah, no reason not to block on this.
Comment 9 Peter Van der Beken [:peterv] 2009-12-16 12:27:57 PST
Turns out this was fixed on trunk before branching for 1.9.2, still need it on 1.9.1 though.
Comment 10 Daniel Veditz [:dveditz] 2009-12-16 16:40:29 PST
Cycle collector exists on the 1.9.0 branch, do we need this there too?
Comment 11 Daniel Veditz [:dveditz] 2009-12-16 17:05:00 PST
Comment on attachment 387035 [details] [diff] [review]
Proposed fix

Approved for 1.9.1.7, a=dveditz for release-drivers
Comment 12 Blake Kaplan (:mrbkap) 2009-12-16 17:24:37 PST
Comment on attachment 387035 [details] [diff] [review]
Proposed fix

Yeah, we should just take this on 1.9.0.
Comment 13 Peter Van der Beken [:peterv] 2009-12-17 16:47:17 PST
https://bugzilla.mozilla.org/show_bug.cgi?id=502687
Comment 14 Peter Van der Beken [:peterv] 2009-12-17 16:47:37 PST
Sigh.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e4a97dfbb9b9
Comment 15 Daniel Veditz [:dveditz] 2009-12-18 11:11:01 PST
Comment on attachment 387035 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.18, a=dveditz for release-drivers
Comment 16 Peter Van der Beken [:peterv] 2010-01-13 07:04:20 PST
Checked in for 1.9.0.x.
Comment 17 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2010-01-27 17:37:41 PST
Given by Blake's comment 1 we do not have a test and do not know how to test it. So QA can only verify this fix based on the check-ins on all branches. I have checked mozilla1.9.2, mozilla1.9.1, and mozilla1.9.0 for existence of this null pointer check. All look fine. Means I will mark all branches as verified fixed.

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