GCGraphBuilder::AddNode crashes on OOM

VERIFIED FIXED in mozilla1.9.2

Status

()

Core
XPCOM
P2
normal
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

({verified1.9.0.18, verified1.9.1, verified1.9.2})

Trunk
mozilla1.9.2
verified1.9.0.18, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.0.18 +
wanted1.9.0.x +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .8+, status1.9.1 .8-fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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).
(Assignee)

Comment 1

8 years ago
Created attachment 387035 [details] [diff] [review]
Proposed fix

I don't know how to test for this, but it does compile!
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #387035 - Flags: superreview?(dbaron)
Attachment #387035 - Flags: review?(dbaron)
Comment on attachment 387035 [details] [diff] [review]
Proposed fix

r=dbaron
Attachment #387035 - Flags: superreview?(dbaron)
Attachment #387035 - Flags: review?(dbaron)
Attachment #387035 - Flags: review+
(Assignee)

Comment 3

8 years ago
http://hg.mozilla.org/mozilla-central/rev/b0a681cd9df5
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(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.
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.

Updated

8 years ago
Duplicate of this bug: 481108
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).
blocking1.9.1: --- → ?
Flags: blocking1.9.2?
Attachment #387035 - Flags: approval1.9.1.7?
Yeah, no reason not to block on this.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
Turns out this was fixed on trunk before branching for 1.9.2, still need it on 1.9.1 though.
status1.9.2: --- → beta1-fixed
Cycle collector exists on the 1.9.0 branch, do we need this there too?
blocking1.9.1: ? → .7+
status1.9.1: --- → wanted
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.17?
Comment on attachment 387035 [details] [diff] [review]
Proposed fix

Approved for 1.9.1.7, a=dveditz for release-drivers
Attachment #387035 - Flags: approval1.9.1.7? → approval1.9.1.7+
(Assignee)

Comment 12

8 years ago
Comment on attachment 387035 [details] [diff] [review]
Proposed fix

Yeah, we should just take this on 1.9.0.
Attachment #387035 - Flags: approval1.9.0.17?
https://bugzilla.mozilla.org/show_bug.cgi?id=502687
status1.9.1: wanted → .7-fixed
Sigh.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e4a97dfbb9b9
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.18?
Flags: blocking1.9.0.18+
Comment on attachment 387035 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.18, a=dveditz for release-drivers
Attachment #387035 - Flags: approval1.9.0.18? → approval1.9.0.18+
Whiteboard: [needs 1.9.0 landing]
Checked in for 1.9.0.x.
Keywords: fixed1.9.0.18
Whiteboard: [needs 1.9.0 landing]
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.18 → verified1.9.0.18, verified1.9.1, verified1.9.2
OS: Linux → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.