Make GetScriptedCallerGlobal work with same-compartment realms

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
11 months ago
22 days ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

11 months ago
Posted 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)
(Assignee)

Comment 2

11 months ago
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

11 months ago
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()?
Attachment #8981351 - Flags: review?(luke) → review+
(Assignee)

Comment 4

11 months ago
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

11 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fddfc4fff54
Make GetScriptedCallerGlobal work with same-compartment realms. r=luke
(Assignee)

Comment 6

11 months ago
Keeping this open for the wasm part.
Flags: needinfo?(jdemooij)
Keywords: leave-open
(Assignee)

Comment 7

11 months ago
Flags: needinfo?(jdemooij)
Attachment #8982172 - Flags: review?(luke)

Comment 9

11 months ago
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()?
Attachment #8982172 - Flags: review?(luke) → review+

Comment 10

11 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8733db9b64b0
part 2 - Don't skip wasm frames in GetScriptedCallerActivationRealmFast. r=luke
(Assignee)

Comment 11

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

Good idea, done.
Keywords: leave-open

Comment 12

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8733db9b64b0
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.