1268215 - Make CCGraphBuilder::AddNode fallible

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

+++ This bug was initially created as a clone of Bug #1144649 +++

Currently |CCGraphBuilder::AddNode| is half fallible. The |AddNodeToMap| call [1] is fallible, but further down the |mNodeBuilder.Add| [2] call is infallible. This ends up doing a rather large allocation (~160K on 32-bit) [3] if a new block is needed.

FWIW I saw this show up in a OOM crash.

[1] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/xpcom/base/nsCycleCollector.cpp#2210
[2] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/xpcom/base/nsCycleCollector.cpp#2218
[3] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/xpcom/base/nsCycleCollector.cpp#707

Comment 1

[Eric Rahm [:erahm]]

Attachment: Make CCGraphBuilder::AddNode fallible

Comment 2

[Andrew McCreight [:mccr8]]

It looks like that I removed this in bug 864344, because NS_Alloc is infallible.

I'm not sure this is a good idea. It looks like the CC will just keep running rather than aborting if this happens. Analyzing a partial CC graph can result in the CC thinking that objects that are being held alive by JS are actually dead, which can result in null pointer derefs in random code.

Comment 3

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> It looks like that I removed this in bug 864344, because NS_Alloc is
> infallible.
> 
> I'm not sure this is a good idea. It looks like the CC will just keep
> running rather than aborting if this happens. Analyzing a partial CC graph
> can result in the CC thinking that objects that are being held alive by JS
> are actually dead, which can result in null pointer derefs in random code.

Okay, but just to be clear you made it kind of fallible in bug 1144649 (so more recently). Should we just make that infallible again?

I suppose we could also reduce the block size instead...

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8746184 [details] [diff] [review]
Make CCGraphBuilder::AddNode fallible

Yeah, I guess it can't make things much worse. Reducing the size a bit might also be nice, but really I feel like the hash table is almost always going to be much bigger than one of these blocks.

Comment 5

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/984b9c1f5c1552bda0dd8fc77a568496c431a07c
Bug 1268215 - Make CCGraphBuilder::AddNode fallible. r=mccr8

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/984b9c1f5c15