780758 - Nobody cares if NS_HOLD_JS_OBJECTS/NS_DROP_JS_OBJECTS fails

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

It turns out that NS_HOLD_JS_OBJECTS and NS_DROP_JS_OBJECTS can fail, basically only in OOM situations where we couldn't create mJSHolders, or can't add an entry to the hashtable.  There are around 28 places the former is called (including a bunch in IndexedDB), and only 4 of them check for error.

http://mxr.mozilla.org/mozilla-central/ident?i=NS_HOLD_JS_OBJECTS&filter=

This is bad because the object will think that the JS object it holds is rooted when it is not.

So we should either fix the other 24 callees or just make these functions infallible and crash the browser if these hash table operations fail. I'm inclined to do the latter, because that will avoid regressions in the future, and it will require less changes, but I'll defer to the opinions of others.

(NS_DROP_JS_OBJECTS isn't really checked either, but the only way it can fail is if there is no table, in which case the corresponding HOLD probably also failed, so who cares...)

I marked this security sensitive, though I'm not sure if potential GC hazards when we're running out of memory and likely to be in danger of imminent crashing anyways is a big deal.

This was found by Cindy Rubio González <[e-mail]>'s error propagation analysis.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Aren't hashtable allocations infallible now?

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Aren't hashtable allocations infallible now?

XPConnect uses JS_DHash, which uses js_malloc, which I think is fallible.  I could be wrong.

Comment 3

[Ben Liblit]

Would someone please add "[uwisc-analysis]" to the Whiteboard list for this bug?  We've been using that elsewhere to tag bugs discovered by Cindy Rubio González <[e-mail]>'s error propagation analysis.  I'd add it here myself, but I don't have that power.  Thanks!

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

> I'd add it here myself, but I don't have that power.  Thanks!

Refresh and you will!

Comment 5

[Andrew McCreight [:mccr8]]

I eliminated one of the ways NS_HOLD_JS_OBJECTS can fail in bug 782792, so this is mostly fixed now. Technically, it can still fail if there's no sXPConnect. That seems unlikely, though, so maybe it should just crash in that case...

Comment 6

[Andrew McCreight [:mccr8]]

Bug 792861 eliminates the last ways that these functions can fail, and adds checks to the chokepoints that will make the browser crash if it starts being able to fail, which will make this whole interface much safer. After that is done, we can close this.

Comment 7

[Andrew McCreight [:mccr8]]

I guess we don't care too much what happens when there's no XPConnect around, so I'll mark this fixed...