1474522 - PrepareScriptEnvironmentAndInvoke should always take a global object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

For bug 1474272 it would help a lot if PrepareScriptEnvironmentAndInvoke and the |invoke| callback called there would have a |global| argument instead of an arbitrary |scope|.

Most callers already pass a global object. The one exception is js-ctypes passing the function it's calling:

https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/js/src/ctypes/CTypes.cpp#7516

Given how the Gecko callback uses this value (it calls NativeGlobal which calls js::GetGlobalForObjectCrossCompartment), I think it's okay to use JS::CurrentGlobalOrNull(cx) instead.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Change PrepareScriptEnvironmentAndInvoke to always take a global object instead of an arbitrary scope

See comment 0. Bobby, according to hg blame you added this mechanism.

Comment 2

[Jan de Mooij [:jandem]]

Green on Try btw.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Change PrepareScriptEnvironmentAndInvoke to always take a global object instead of an arbitrary scope

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8990916 [details] [diff] [review]
Change PrepareScriptEnvironmentAndInvoke to always take a global object instead of an arbitrary scope

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

::: js/src/ctypes/CTypes.cpp
@@ +7512,5 @@
>    ArgClosure argClosure(cif, result, args, static_cast<ClosureInfo*>(userData));
>    JSContext* cx = argClosure.cinfo->cx;
> +
> +  RootedObject global(cx, JS::CurrentGlobalOrNull(cx));
> +  MOZ_ASSERT(global);

Can we assert that jsFnObj belongs to |global|?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #4)
> > +  RootedObject global(cx, JS::CurrentGlobalOrNull(cx));
> > +  MOZ_ASSERT(global);
> 
> Can we assert that jsFnObj belongs to |global|?

It's complicated because (1) jsFnObj could be a (callable) CCW and then we can't get its global (this is the issue I'm trying to fix) and (2) with same-compartment-realms it could be a plain cross-global JSFunction.

So our options are either using the cx's global or unwrapping jsFnObj and using the unwrapped object's global.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > > +  RootedObject global(cx, JS::CurrentGlobalOrNull(cx));
> > > +  MOZ_ASSERT(global);
> > 
> > Can we assert that jsFnObj belongs to |global|?
> 
> It's complicated because (1) jsFnObj could be a (callable) CCW and then we
> can't get its global (this is the issue I'm trying to fix) and (2) with
> same-compartment-realms it could be a plain cross-global JSFunction.
> 
> So our options are either using the cx's global or unwrapping jsFnObj and
> using the unwrapped object's global.

Ah right. Let's just assert they're same-compartment then?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #6)
> Ah right. Let's just assert they're same-compartment then?

Sure, will add js::AssertSameCompartment(cx, argClosure.cinfo->jsfnObj);

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/397856154e99
Change PrepareScriptEnvironmentAndInvoke to always take a global object instead of an arbitrary scope. r=bholley

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/397856154e99