1465002 - Make GetScriptedCallerGlobal work with same-compartment realms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

This one is not entirely trivial because it assumes we can do activation->compartment()->global() but activations will contain cross-realm calls.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

For the most part pretty easy if we make GetScriptedCallerActivationFast also return the Realm* in addition to the activation.

One wrinkle is that we have to skip self-hosted scripts. That's easy to do for interpreter and baseline, but Ion self-hosted frames *could* have inlined a same-realm non-self-hosted script, so in that case we have to give up now. I think that should be fine, though.

Comment 2

[Jan de Mooij [:jandem]]

Also, once wasm can make direct DOM calls without going through JS, I think it will no longer be okay to just skip wasm frames here?

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8981351 [details] [diff] [review]
Patch

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

Nice job with the case analysis.

::: js/src/jsapi.cpp
@@ +7546,5 @@
> +//   all have the same realm/global anyway.
> +//
> +// * When we see a self-hosted outer script, it's possible we inlined
> +//   non-self-hosted scripts into it, so in this case we have to give up
> +//   (Baseline and the interpreter don't inline so this only affects Ion).

Maybe append "because in this case, whether or not to skip the self-hosted frame (to the possibly different-realm caller) requires the slow path to handle inlining"?

@@ +7551,2 @@
>  static bool
> +GetScriptedCallerActivationFast(JSContext* cx, Activation** activation, Realm** realm)

nit: maybe rename "GetScriptedCallerActivationRealmFast()?

Comment 4

[Jan de Mooij [:jandem]]

Note to self: post a follow-up patch to handle wasm frames too on the fast path (instead of pretending they don't exist).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fddfc4fff54
Make GetScriptedCallerGlobal work with same-compartment realms. r=luke

Comment 6

[Jan de Mooij [:jandem]]

Keeping this open for the wasm part.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Don't skip wasm frames in GetScriptedCallerActivationRealmFast

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2fddfc4fff54

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8982172 [details] [diff] [review]
Part 2 - Don't skip wasm frames in GetScriptedCallerActivationRealmFast

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

Thanks!

::: js/src/vm/Stack.h
@@ +1956,5 @@
>      void skipNonScriptedJSFrames();
> +
> +    // Returns true iff this is a JIT frame with a self-hosted script. Note: be
> +    // careful, JitFrameIter does not consider functions inlined by Ion.
> +    bool isSelfHosted() const;

How about naming it: isSelfHostedIgnoringInlining()?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8733db9b64b0
part 2 - Don't skip wasm frames in GetScriptedCallerActivationRealmFast. r=luke

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #9)
> How about naming it: isSelfHostedIgnoringInlining()?

Good idea, done.

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8733db9b64b0