792861 - Make HoldJSObjects/DropJSObjects infallible

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

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 1

[Andrew McCreight [:mccr8]]

Attachment: Make HoldJSObjects/DropJSObjects infallible

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Why should it? It can never happen.

Comment 5

[Andrew McCreight [:mccr8]]

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

Comment 6

[Andrew McCreight [:mccr8]]

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 7

[Bobby Holley (:bholley)]

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.

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: rework HOLD/DROP

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.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Make HoldJSObjects/DropJSObjects infallible

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 11

[Andrew McCreight [:mccr8]]

Attachment: Make HoldJSObjects/DropJSObjects infallible

Comment 12

[Andrew McCreight [:mccr8]]

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 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

[Andrew McCreight [:mccr8]]

I added a null check and assertion for nsXPConnect in HoldJSObjects.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecbc55bdcb05

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ecbc55bdcb05