Closed Bug 1470250 Opened Last year Closed Last year

Implement realm switching for native calls

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(7 files, 1 obsolete file)

And some fixes for things I ran into while writing tests for this.
ObjectGroupRealm::makeGroup allocates a new ObjectGroup but it set group->realm to cx->realm() and that's incorrect if we're allocating a new group for an existing object (for instance because the original group is a lazy group or whatever).

This fixes some weird issues I saw when writing tests.
Attachment #8986851 - Flags: review?(luke)
Not strictly necessary, but we'll have some more AutoRealms and it's nice to have that code be just two pointer writes in opt builds (cx->realm_ and cx->zone_).
Attachment #8986855 - Flags: review?(jcoppeard)
In JSContext::setRealm we can't assert realm->hasBeenEnteredIgnoringJit() for the "old" realm, because the JIT could have entered it (and we're now entering a *different* realm in C++).

We also can't assert this for the *new* realm, because when we leave a realm we also call setRealm and it's basically the same issue.

We *can* assert this in leaveRealm for the realm we're leaving, because that must have been entered from C++ (we'll probably need to add a new method for exception unwinding).
Attachment #8986857 - Flags: review?(luke)
Note that CallJSNative and CallJSNativeConstructor are also used for call/construct Class hooks, so I added the AutoRealm outside these functions for now so we don't switch realms in that case.

I need to look at this more, but I think for proxies at least we don't want to switch realms when calling call/construct traps.
Attachment #8986862 - Flags: review?(luke)
For native calls we know the function statically in all cases, so this should have no perf overhead for same-realm calls.
Attachment #8986865 - Flags: review?(luke)
Comment on attachment 8986865 [details] [diff] [review]
Part 6 - Switch realms if needed before/after native calls in JIT code

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

::: js/src/jit/CodeGenerator.cpp
@@ +4264,5 @@
>      masm.Push(argUintNReg);
>  
> +    if (call->mir()->maybeCrossRealm()) {
> +        masm.movePtr(ImmGCPtr(target->rawJSFunction()), tempReg);
> +        masm.switchToObjectRealm(tempReg, tempReg);

Looking at this now, we could optimize this by baking in the Realm* instead of the JSFunction*, but we only emit this for definitely-cross-realm calls so a few loads is still orders of magnitude faster than what we're doing now.
This adds an assertCorrectRealm testing function that just MOZ_RELEASE_ASSERT's the context's realm matches callee->realm.

This will be useful once we start fuzzing because it will find places where we call a native without entering its realm.
Attachment #8986866 - Flags: review?(luke)
Attachment #8986851 - Flags: review?(luke) → review+
Attachment #8986852 - Flags: review?(luke) → review+
Attachment #8986857 - Flags: review?(luke) → review+
Comment on attachment 8986862 [details] [diff] [review]
Part 5 - Use AutoRealm when calling natives or resolve hooks

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

Maybe assert cx->realm = callee->realm in CallJSNative()?
Attachment #8986862 - Flags: review?(luke) → review+
Attachment #8986865 - Flags: review?(luke) → review+
Comment on attachment 8986866 [details] [diff] [review]
Part 7 - Add a testing function + tests

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

Nice
Attachment #8986866 - Flags: review?(luke) → review+
Comment on attachment 8986855 [details] [diff] [review]
Part 3 - Remove JSContext::arenas_

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

Nothing wrong with the patch, but I'd like us to keep this for now because I'm planning to make use of it in bug 1434598.
Attachment #8986855 - Flags: review?(jcoppeard)
Is this one OK or will that also break your use case?
Attachment #8986855 - Attachment is obsolete: true
Attachment #8987055 - Flags: review?(jcoppeard)
Comment on attachment 8987055 [details] [diff] [review]
Part 3 - Get rid of a null check in JSContext::setRealm

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

That's fine.
Attachment #8987055 - Flags: review?(jcoppeard) → review+
Blocks: 1470904
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f70bd2a3a9b
part 1 - Use correct realm in ObjectGroupRealm::makeGroup. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d21705dace
part 2 - Move CallJSNative and CallJSNativeConstructor to Interpreter.cpp. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e81f5917390
part 4 - Fix some realm assertions that are invalid when JIT code switches realms. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/0616cd7fc7ad
part 3 - Get rid of a null check in JSContext::setRealm. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6490693cad
part 5 - Use AutoRealm when calling natives or resolve hooks. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/95df215e1636
part 6 - Switch realms if needed before/after native calls in JIT code. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cef7604e477
part 7 - Add a testing function + tests. r=luke
(In reply to Luke Wagner [:luke] from comment #9)
> Maybe assert cx->realm = callee->realm in CallJSNative()?

We can't because Class call/construct hooks also go through there, see comment 5.
Depends on: 1479430
You need to log in before you can comment on or make changes to this bug.