Closed Bug 1465002 Opened 5 years ago Closed 5 years ago

Make GetScriptedCallerGlobal work with same-compartment realms


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox62 --- fixed


(Reporter: jandem, Assigned: jandem)




(2 files)

This one is not entirely trivial because it assumes we can do activation->compartment()->global() but activations will contain cross-realm calls.
Attached patch PatchSplinter Review
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.
Attachment #8981351 - Flags: review?(luke)
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 on attachment 8981351 [details] [diff] [review]

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()?
Attachment #8981351 - Flags: review?(luke) → review+
Note to self: post a follow-up patch to handle wasm frames too on the fast path (instead of pretending they don't exist).
Pushed by
Make GetScriptedCallerGlobal work with same-compartment realms. r=luke
Keeping this open for the wasm part.
Flags: needinfo?(jdemooij)
Keywords: leave-open
Flags: needinfo?(jdemooij)
Attachment #8982172 - Flags: review?(luke)
Comment on attachment 8982172 [details] [diff] [review]
Part 2 - Don't skip wasm frames in GetScriptedCallerActivationRealmFast

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


::: 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()?
Attachment #8982172 - Flags: review?(luke) → review+
Pushed by
part 2 - Don't skip wasm frames in GetScriptedCallerActivationRealmFast. r=luke
(In reply to Luke Wagner [:luke] from comment #9)
> How about naming it: isSelfHostedIgnoringInlining()?

Good idea, done.
Keywords: leave-open
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.