502687 - GCGraphBuilder::AddNode crashes on OOM

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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) (inactive)]

Attachment: Proposed fix

I don't know how to test for this, but it does compile!

Comment 2

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

Comment on attachment 387035 [details] [diff] [review]
Proposed fix

r=dbaron

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/b0a681cd9df5

Comment 4

[Peter Van der Beken [:peterv]]

(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]]

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 7

[Peter Van der Beken [:peterv]]

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)]

Yeah, no reason not to block on this.

Comment 9

[Peter Van der Beken [:peterv]]

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

Cycle collector exists on the 1.9.0 branch, do we need this there too?

Comment 11

[Daniel Veditz [:dveditz]]

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) (inactive)]

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

https://bugzilla.mozilla.org/show_bug.cgi?id=502687

Comment 14

[Peter Van der Beken [:peterv]]

Sigh.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e4a97dfbb9b9

Comment 15

[Daniel Veditz [:dveditz]]

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

Checked in for 1.9.0.x.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.