1471924 - Implement realm switching for Class call/construct hooks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

These hooks are just JSNatives and it's nice to have the invariant that all JSNatives are called with cx->realm == the callee's realm.

Now, we don't want to switch realms for proxies (bug 1297179) and proxies are currently special-cased in JSObject::{callHook,constructHook}. We can do some nice refactoring though to pull the proxy code out of callHook/constructHook and just have InternalCallOrConstruct call Proxy::{call,construct} directly if we have a proxy. After that we can always enter the callee's realm in CallJSNative.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Stop pretending proxies have a JSNative call/construct hook

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Enter callee's realm when calling Class call/construct hooks

Now CallJSNative is never called for proxy callees so we can simply enter the callee's realm there.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8988517 [details] [diff] [review]
Part 1 - Stop pretending proxies have a JSNative call/construct hook

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

Wow, nice.

Comment 5

[André Bargull [:anba]]

Thanks for fixing bug 1331410, too. :-D

Comment 6

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #5)
> Thanks for fixing bug 1331410, too. :-D

\o/

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Add a proxy testcase

It just occurred to me we can actually add a proxy test too. Bug 1297179 should be partially fixed with same-compartment realms (the proxy part, not the bound function part) so I had to see if that test passes now and it does :)

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Add a proxy testcase

Comment 9

[André Bargull [:anba]]

Comment on attachment 8988604 [details] [diff] [review]
Part 3 - Add a proxy testcase

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

::: js/src/jit-test/tests/realms/proxy-realm.js
@@ +2,5 @@
> +
> +load(libdir + "asserts.js");
> +
> +var g = newGlobal({sameCompartmentAs: this});
> +var {proxy, revoke} = g.eval(`Proxy.revocable(() => {}, {})`);

The |new proxy()| case was actually already passing when using an arrow function as the underlying proxy target. Only when the proxy target is actually a constructor, which then makes the proxy a constructor, too, |new proxy()| doesn't throw a TypeError from the correct realm.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #9)
> The |new proxy()| case was actually already passing when using an arrow
> function as the underlying proxy target.

D'oh, good catch. I'll change it to a non-arrow function and verify adding an AutoRealm before the Proxy::construct call breaks the test (I checked this for Proxy::call but not Proxy::construct..).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da138f7c70ef
part 1 - Stop pretending proxies have a JSNative call/construct hook. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/0807cc4090d9
part 2 - Enter callee's realm when calling Class call/construct hooks. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a4c5bb5efb
part 3 - Add a proxy testcase. r=anba

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/da138f7c70ef
https://hg.mozilla.org/mozilla-central/rev/0807cc4090d9
https://hg.mozilla.org/mozilla-central/rev/a7a4c5bb5efb