Followup from bug 782792. Now that mJSHolders is infallible, we should be able to get rid of a bunch of nsresult error propagation. I probably should have done this at the same time, but oh well.

Comment on attachment 673914 [details] [diff] [review] Make HoldJSObjects/DropJSObjects infallible Review of attachment 673914 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsContentUtils.cpp @@ +4561,5 @@ > + NS_LOG_ADDREF(sXPConnect, sJSGCThingRootCount, "HoldJSObjects", > + sizeof(void*)); > + > + if (NS_FAILED(sXPConnect->AddJSHolder(aScriptObjectHolder, aTracer))) { > + MOZ_CRASH(); Just use MOZ_ALWAYS_TRUE.

(In reply to :Ms2ger from comment #2) > Just use MOZ_ALWAYS_TRUE. That won't crash in release builds.

Why should it? It can never happen.

It is just defense-in-depth against future changes. Failures here tend to be dangerous and hard to find in testing.

Comment on attachment 673914 [details] [diff] [review] Make HoldJSObjects/DropJSObjects infallible Feel free to bounce this over to Olli or somebody else if you don't think you are the right person to review this. Nothing very complex here, just clearing out a pipeline...

Comment on attachment 673914 [details] [diff] [review] Make HoldJSObjects/DropJSObjects infallible If they're infallible, can we just mark them as [notxpcom] in nsIXPConnect and have them be bonafide void methods? Feel free to reflag if there's a good reason not to do that.

I wonder if it would be a nice idea to make the XPC hold/drop functions return failure if the object is/isn't present. Right now, we could end up in a situation where sJSGCThingRootCount is wrong if a class does HOLD or DROP too much. I think that would require an extra hashtable lookup.

Reworked version that cleans up the HOLD/DROP a bit. We check for HOLD of things we already hold and DROP of things we don't hold, and don't adjust ref counts in case we did it badly. As an experiment, I threw in assertions that we never fail these checks: https://tbpl.mozilla.org/?tree=Try&rev=9aa3ce78d219 In practice, we may want to just deal with the root count separately from the infallibillization, which is mostly a side show here.

This is a little cleaner after Olli's fixup of the root counting mess. Per Bobby's suggestion, I made the rooting functions [notxpcom]. I'm not sure why I didn't try that before, it seems to work just fine. I guess

Comment on attachment 686790 [details] [diff] [review] Make HoldJSObjects/DropJSObjects infallible L64 try run looks good. Olli is an expert on this code now, so I'll make him review. :)

Comment on attachment 686790 [details] [diff] [review] Make HoldJSObjects/DropJSObjects infallible > nsContentUtils::HoldJSObjects(void* aScriptObjectHolder, > nsScriptObjectTracer* aTracer) > { >- NS_ENSURE_TRUE(sXPConnect, NS_ERROR_UNEXPECTED); >- >- return sXPConnect->AddJSHolder(aScriptObjectHolder, aTracer); >+ sXPConnect->AddJSHolder(aScriptObjectHolder, aTracer); > } Please don't remove the null check. perhaps if (!sXPConnect) { MOZ_ASSERT(false, "Unexpected time to call HoldJSObjects"); return; } I know this is a bit ugly, but I'm worried about extra crashes during shutdown. Could we at least have the null check for some time. >- [noscript] void addJSHolder(in voidPtr aHolder, >- in nsScriptObjectTracerPtr aTracer); >+ [noscript,notxpcom] void addJSHolder(in voidPtr aHolder, >+ in nsScriptObjectTracerPtr aTracer); > > /** > * Stop rooting the JS objects held by aHolder. > * @param aHolder The object that hold the rooted JS objects. > */ >- [noscript] void removeJSHolder(in voidPtr aHolder); >+ [noscript,notxpcom] void removeJSHolder(in voidPtr aHolder); This shouldn't affect whether binary addons can call these methods, so should be ok.

> I know this is a bit ugly, but I'm worried about extra crashes during shutdown. > Could we at least have the null check for some time. Sure, I'll do something like you suggest. I guess if we try to root something after XPConnect has gone away, we hopefully don't actually have any JS things any more so it shouldn't matter, and there's probably not a lot going on that is attacker-controllable anyways.

I added a null check and assertion for nsXPConnect in HoldJSObjects. https://hg.mozilla.org/integration/mozilla-inbound/rev/ecbc55bdcb05