Jan de Mooij [:jandem] Assignee
	Description • 6 years ago

We'll soon be able to add a sameCompartmentAs option (similar to sameZoneAs) to newGlobal in the JS shell, so you can do this to create a same-compartment realm:

  newGlobal({sameCompartmentAs: this});

(This should probably be a no-op initially with --fuzzing-safe)

I think we should also:

* Add a testing function that calls JS::GetScriptedCallerGlobal. It's nice to expose that API to the shell anyway and it lets us easily test realm entering for scripted functions.

* Add a testing functions that returns the global for a (non-wrapper) JSObject. Then we can use this to check objects are allocated in the correct realm.

	Jan de Mooij [:jandem] Assignee
	Comment 1 • 6 years ago

ZoneSpecifier => CompartmentSpecifier, make it an enum class, rename the values.

Then we can do options.setExistingCompartment(..) in the JS shell for newGlobal({sameCompartmentAs: this}). This option is a no-op for now if --fuzzing-safe is used.

I also added a basic.js jit-test in a new jit-test/tests/realms/ directory. I verified with some logging we indeed have a compartment with 3 realms when I run it.

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 6 years ago

This also adds a simple test we can extend later to test realm switching for same-compartment scripted calls.

	Jan de Mooij [:jandem] Assignee
	Comment 3 • 6 years ago

objectGlobal(obj) returns null if the object is a wrapper.

This is sweet because now we can use this function in the realms/basic.js test and the test will now fail if same-compartment realms are disabled or broken (this happens with --fuzzing-safe, for instance).

	Luke Wagner [:luke]
	Comment 4 • 6 years ago

Comment on attachment 8983717 [details] [diff] [review]
Part 1 - Refactor ZoneSpecifier and add a sameCompartmentAs option to newGlobal in the shell

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

Nice and explicit.

::: js/src/gc/GC.cpp
@@ +8057,5 @@
>          return nullptr;
>      }
>  
> +    if (compHolder) {
> +        if (!zone->compartments().append(comp)) {

If the below gc.zones().append(zone) fails, I think this would leave a dangling JSCompartment* in the zone.  Really, the existing rt->gc.zones().append(zone) that this is copying was hazard-prone since simply adding a failure path between it and the release() calls below causes failure.  My preference would be to append(Move((comp|zone)Holder)) so there is a transactional transfer of ownership.

	Luke Wagner [:luke]
	Comment 5 • 6 years ago

Comment on attachment 8983732 [details] [diff] [review]
Part 2 - Add a scriptedCallerGlobal() testing function for JS::GetScriptedCallerGlobal

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

Nice!

	Luke Wagner [:luke]
	Comment 6 • 6 years ago

Comment on attachment 8983740 [details] [diff] [review]
Part 3 - Add an objectGlobal(obj) testing function

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

::: js/src/builtin/TestingFunctions.cpp
@@ +5122,5 @@
> +
> +    obj = ToWindowProxyIfWindow(&obj->global());
> +
> +    if (!cx->compartment()->wrap(cx, &obj))
> +        return false;

While harmless, is this wrap() needed?  I think it was necessary for GetScriptedCallerGlobal() b/c the caller can be in a different compartment, but here we're same-compartment with args[0] and we haven't unwrapped anything and I think the WindowProxy of a given GlobalObject is same-compartment as the GlobalObject.  Grepping around, other uses of ToWindowProxyIfWindow() (e.g., LexicalEnvironmentObject::thisValue()) don't wrap().

	Jan de Mooij [:jandem] Assignee
	Comment 7 • 6 years ago

(In reply to Luke Wagner [:luke] from comment #4)
> If the below gc.zones().append(zone) fails, I think this would leave a
> dangling JSCompartment* in the zone.

If we fail to append(zone), nobody will touch the JSCompartment* in the Zone's Vector anyway - the UniquePtrs will just delete both the compartment and zone.

> My preference would be to append(Move((comp|zone)Holder)) so there
> is a transactional transfer of ownership.

Yeah I agree it would be nice to use UniquePtr more for this code. I'll try something.

(In reply to Luke Wagner [:luke] from comment #6)
> While harmless, is this wrap() needed?

Oops, you're right, it's not needed here because we have this explicit wrapper check.

	Jan de Mooij [:jandem] Assignee
	Comment 8 • 6 years ago

I tried to use UniquePtr for the zone/compartment/realm Vectors, but it becomes pretty messy. I'm also not sure it fixes all potential issues here.

The attached patch changes NewRealm to first reserve() space in all the Vectors and then we can mutate them all infallibly. This way there should be no way to get into a weird/inconsistent state.

	Pulsebot
	Comment 9 • 6 years ago

Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bbae91a2eaf
part 1 - Refactor ZoneSpecifier and add a sameCompartmentAs option to newGlobal in the shell. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/22ea0700931c
part 2 - Add a scriptedCallerGlobal() testing function for JS::GetScriptedCallerGlobal. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6608413e08
part 3 - Add an objectGlobal(obj) testing function. r=luke

	Andrei Ciure[:aciure]
	Comment 10 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6bbae91a2eaf
https://hg.mozilla.org/mozilla-central/rev/22ea0700931c
https://hg.mozilla.org/mozilla-central/rev/4b6608413e08

	Jan de Mooij [:jandem] Assignee
	Comment 11 • 6 years ago

NI myself to update the newGlobal documentation when I have time.

	Jan de Mooij [:jandem] Assignee
	Comment 12 • 6 years ago

Nvm, this is being fixed in bug 1481858.