1477923 - CallSetup and nsFrameMessageManager use JSAutoRealm with CCWs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

See bug 1474541.

Here the callable might be a CCW:

https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/bindings/CallbackObject.cpp#254

Bug 1474541 will add a similar JSAutoRealm to nsFrameMessageManager::ReceiveMessage.

Boris volunteered to take this :)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make WebIDL callbacks store a global in addition to the object that's used as a callback

Please review the places that actually get the global a bit carefully.  I am especially worried about the one in getCallbackConversionInfo in Codegen.py, though I _think_ I have convinced myself that the global there will always match ${val}.

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> Please review the places that actually get the global a bit carefully.

What do you mean by that? The places that decide which global to store in the callbacks?

What do I need to look out for? That the global is actually the global for the callback object and not for something else?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> The places that decide which global to store in the callbacks?

Yes, as opposed to all the boilerplate just passing it around.

> That the global is actually the global for the callback object and not for something else?

Yep.

Comment 4

[Andrew McCreight [:mccr8]]

What is ${val}? I can't tell where it is defined.

Comment 5

[Boris Zbarsky [:bzbarsky]]

See documentation at https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/dom/bindings/Codegen.py#4338-4341

It's set in the various replacement dictionaries that then get used with this template.  Searching for "val" should find them....

(Yes, I know this is a pain to audit, sorry.  :( )

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8995616 [details] [diff] [review]
Make WebIDL callbacks store a global in addition to the object that's used as a callback

Review of attachment 8995616 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it took me so long to get through this. I was a little over my head with all of this, but I gave it my best.

::: dom/bindings/CallbackObject.h
@@ +72,5 @@
>  
>    // Instead of capturing the current stack to use as an async parent when the
>    // callback is invoked, the caller can use this overload to pass in a stack
>    // for that purpose.
> +  explicit CallbackObject(JSObject* aCallback,

Did you de-handle these args because it is hard to have a handle at some call site and we don't make use of it anyways?

@@ +214,5 @@
>                           nsIGlobalObject* aIncumbentGlobal)
>    {
>      MOZ_ASSERT(aCallback && !mCallback);
> +    MOZ_ASSERT(aCallbackGlobal);
> +    // Should we make this compartment assert a diagnostic assert?

Sure, why not? We have a bunch of diagnostic asserts like that.

@@ +293,5 @@
>    // its members, directly itself, this code won't call f(), or get its members,
>    // on the code's behalf.
>    JS::Heap<JSObject*> mCallback;
> +  // mCallbackGlobal is the global that we were in when we created the
> +  // callback.  In particular, it is guaranteed to be same-compartment with

micronit: one space after periods in this comment to be consistent with the rest of it.

::: dom/bindings/Codegen.py
@@ +4298,5 @@
>      # Optional<RootedCallback> thing, which is not transparent to consumers.
>      useFastCallback = (not isMember and not isCallbackReturnValue and
>                         not isOptional)
>      if useFastCallback:
>          name = "binding_detail::Fast%s" % name

For these two bits of code, I gave up on trying to understand CodeGen.py, and looked at some instances of this code in the output.

@@ +4300,5 @@
>                         not isOptional)
>      if useFastCallback:
>          name = "binding_detail::Fast%s" % name
> +        rootArgs = ""
> +        args = "&${val}.toObject(), JS::CurrentGlobalOrNull(cx)"

This one seems to only be called on args[n] things, which seems reasonable to assume are always in cx.

@@ +4305,5 @@
>      else:
> +        rootArgs = dedent(
> +            """
> +            JS::Rooted<JSObject*> tempRoot(cx, &${val}.toObject());
> +            JS::Rooted<JSObject*> tempGlobalRoot(cx, JS::CurrentGlobalOrNull(cx));

It is used in a number of ways besides args, but it looked like the non-args things were mostly objects returned from functions where we passed in cx, so it seems reasonable to believe that they are the same compartment as cx.

@@ +15074,5 @@
>            return nullptr;
>          }
> +        // We should be getting the implementation object for the relevant
> +        // contract here, which should never be a cross-compartment wrapper.
> +        MOZ_ASSERT(!js::IsWrapper(jsImplObj));

Is this assert redundant with the one in GetNonCCWObjectGlobal?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Sorry it took me so long to get through this. 

It wasn't a problem at all.  I expected this to be a pain to review; sorry to dump it on you...

> Did you de-handle these args because it is hard to have a handle at some call site and we don't make use of it anyways?

Right, de-handling was possible and allowed me to avoid an extra Rooted for the JSContext global.

> Sure, why not? We have a bunch of diagnostic asserts like that.

Mostly worried about performance of nightly being worse than it could be... but I think this one should be OK.

> micronit: one space after periods in this comment to be consistent with the rest of it.

Done.

> For these two bits of code, I gave up on trying to understand CodeGen.py

I don't blame you.  :(

> Is this assert redundant with the one in GetNonCCWObjectGlobal?

Yes, good point.  I'll take it out.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92fb1c3c4d93
Make WebIDL callbacks store a global in addition to the object that's used as a callback.  r=mccr8

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/92fb1c3c4d93