Jason Orendorff [:jorendorff] Assignee
	Description • 7 years ago

The first step in supporting same-compartment realms.

Without any change in behavior, we'll make the following superficial
JSAPI changes:

*   Define JS::Realm as a public typedef of JSCompartment

*   Move several APIs from jsapi.h to public/Realm.h and rename them

*   Rename JSAutoCompartment -> JS::AutoRealm.
    (Look for cases where the object passed to the ctor might be a CCW,
    since we want to ban that later.)

*   Finally, change JS::Realm so that it is not the same type as JSCompartment,
    to ensure that API users outside js/src do not conflate the two.
    (JS::Realm can be an opaque struct type, for now, and inside
    the JSAPI we can just cast it to JSCompartment immediately
    on entry.)

The idea is to make sure that outside js/src, everyone's ready for a world with multiple realms per compartment, and no one's conflating the two (as much as we can manage). It seems good to do this before we actually start making changes.

	Jason Orendorff [:jorendorff] Assignee
	Comment 1 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72b00bd642f286af4ebb9492f9b7cac9f275ee0e

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bff917cef6965a64f8e5b42e1f6795b4943868a2

	Jason Orendorff [:jorendorff] Assignee
	Comment 3 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ae6976d4a734003a449da022f44aabc9749b814

	Jason Orendorff [:jorendorff] Assignee
	Comment 4 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14386a3632953bb9fe8de7ba3b73f71290f6d824

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc5237d9dee6a5192525de1be03d9d2248d7a536

	Jason Orendorff [:jorendorff] Assignee
	Comment 6 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ab13420bbe102efd4d9cd05952ba0ce8622f2f1

	Jason Orendorff [:jorendorff] Assignee
	Comment 7 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3751c95046d98117fd7a688d83685877204ff4a

	Jason Orendorff [:jorendorff] Assignee
	Comment 8 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3a8f616964382a3ab132b97dc52bb685a897b19

	Jason Orendorff [:jorendorff] Assignee
	Comment 9 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac9c7e04c174c8b2e4ca0e533a8a2838d293f700

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cacc36f8f2dde3a0031a8aff4df22c2d344ded9c

	Jason Orendorff [:jorendorff] Assignee
	Comment 11 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8448cb19ea582f16b88ffbbebafff7a7e5194e4

	Steve Fink [:sfink] [:s:]
	Comment 14 • 7 years ago

Comment on attachment 8890331 [details] [diff] [review]
JSAPI for realms: Add JS::Realm opaque type and GC rooting policy for it

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

::: js/public/GCPolicyAPI.h
@@ +176,5 @@
>          return false;
>      }
>  };
>  
> +template <> struct GCPolicy<JS::Realm*>;  // declared in TypeDecls.h

TypeDecls.h only has a forward-declaration of Realm. Shouldn't this say Realm.h?

::: js/public/Realm.h
@@ +25,5 @@
>  
>  namespace JS {
>  
> +template <>
> +struct GCPolicy<Realm*>

It seems a little unusual to have a GCPolicy for a non-GC thing. (Hardly the first, but still.) Can you repeat some form of what you say in Realm.cpp? As in, there is a strong edge both ways between a Realm and its global.

@@ +50,5 @@
> +    // compartments as being one-to-one, but they are actually identical.
> +    return reinterpret_cast<JSCompartment*>(realm);
> +}
> +
> +// Return the realm in a given compartment

missing period

::: js/public/TraceKind.h
@@ +132,5 @@
>  JS_FOR_EACH_TRACEKIND(JS_EXPAND_DEF)
>  #undef JS_EXPAND_DEF
>  
>  // Specify the RootKind for all types. Value and jsid map to special cases;
> +// cell pointer types we can derive directly from the TraceKind; everything else

I think I'd go with "Cell".

::: js/src/jsgc.cpp
@@ +3508,5 @@
>   * compartment. If we know we're deleting the entire zone, then
>   * SweepCompartments is allowed to delete all compartments. In this case,
> + * |keepAtleastOne| is false. If any cells remain alive in the zone, it
> + * cannot be deleted, and we then set |keepAtleastOne| true to prohibit
> + * sweepCompartments from deleting every compartment. Instead, it preserves an

the s/objects/cells/ change is good, but I find the rest of this confusing now. How about:

    If any cells remain alive in the zone, set |keepAtLeastOne| to true to prevent sweepCompartments from deleting every compartment.

or at least s/cannot/should not/.

::: js/src/vm/Realm.cpp
@@ +25,5 @@
> +    // Here we simply trace our side of that edge. During GC,
> +    // GCRuntime::traceRuntimeCommon() marks all other compartment roots, for
> +    // all compartments.
> +    JSCompartment* comp = JS::GetCompartmentForRealm(realm);
> +    comp->traceGlobal(trc);

Why the temporary?

@@ +32,5 @@
> +JS_PUBLIC_API(bool)
> +gc::RealmNeedsSweep(JS::Realm* realm)
> +{
> +    JSCompartment* comp = JS::GetCompartmentForRealm(realm);
> +    return comp->globalIsAboutToBeFinalized();

And again, why the enchantment with temporaries?

"Call GetCompartmentForRealm. I'd like to take time here to point out an interesting fact about compartments: you can abbreviate them to 'comp' and people will usually still know what you mean! Fascinating, isn't it? Now, look forward to our next segment, where we will be discussing the brilliant displays of outrage in response to a certain twitter feed, and ranking respondents by their wit and how well they..."

Sorry. I got a little carried away there.

	Steve Fink [:sfink] [:s:]
	Comment 15 • 7 years ago

Comment on attachment 8890333 [details] [diff] [review]
JSAPI for realms: JS_SetRealmNameCallback

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2669,5 @@
> +static void
> +RealmNameCallback(JSContext* cx, JS::Handle<JS::Realm*> realm, char* buf, size_t bufsize)
> +{
> +    JSCompartment* comp = JS::GetCompartmentForRealm(realm);
> +    CompartmentNameCallback(cx, comp, buf, bufsize);

(in case you're wondering, I have no issue with this use of a temporary.)

	Jason Orendorff [:jorendorff] Assignee
	Comment 17 • 7 years ago

The existing CompartmentPrivate struct will be split into two parts,
one part containing per-realm (i.e. per web page) data and one
containing per-compartment (i.e. per trust realm) data.

	Steve Fink [:sfink] [:s:]
	Comment 20 • 7 years ago

Comment on attachment 8890490 [details] [diff] [review]
JSAPI for realms: JS::Get/SetRealmPrivate()

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

::: js/public/Realm.h
@@ +64,5 @@
> +GetRealmPrivate(Handle<Realm*> realm);
> +
> +// Set the "private data" internal field of the given Realm.
> +extern JS_PUBLIC_API(void)
> +SetRealmPrivate(Handle<Realm*> realm, void* data);

Using Handle<> here is pretty paranoid, but I guess it's ok.

	Steve Fink [:sfink] [:s:]
	Comment 21 • 7 years ago

Comment on attachment 8890491 [details] [diff] [review]
JSAPI for realms: JS::SetDestroyRealmCallback

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

::: js/public/Realm.h
@@ +60,5 @@
>  // Get the value of the "private data" internal field of the given Realm.
>  // This field is initially null and is set using SetRealmPrivate.
>  // It's a pointer to embeddding-specific data that SpiderMonkey never uses.
>  extern JS_PUBLIC_API(void*)
> +GetRealmPrivate(Realm* realm);

Huh. You gave up on the Handle<>? I'm fine with that.

Oh. I guess you need to grab it during GC, and you didn't want to fake up a Handle<> for no real reason.

	Jason Orendorff [:jorendorff] Assignee
	Comment 22 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7fe00e1522cd02adf2ae2c1e048b4dac2bd6ed1

	Jason Orendorff [:jorendorff] Assignee
	Comment 23 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d071d76cbe83abf12f9b93444999da9335e2fc77

	Jason Orendorff [:jorendorff] Assignee
	Comment 24 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cc1c8e1a97640525d79c34a62e054a8f3eabdb8

	Jason Orendorff [:jorendorff] Assignee
	Comment 25 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e996e533dbfc9fad52011266384454f32c5a8f7

	Jason Orendorff [:jorendorff] Assignee
	Comment 26 • 7 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #21)
> Huh. You gave up on the Handle<>? I'm fine with that.

Yeah, sorry. I thought I histedit-ed it away, but apparently I missed a spot.

> Oh. I guess you need to grab it during GC, and you didn't want to fake up a
> Handle<> for no real reason.

Right. Plus, we have enough other functions that do the same thing--I am guessing the hazard analysis should have no trouble with this.

	Jason Orendorff [:jorendorff] Assignee
	Comment 27 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ebe8f22c28fa699b248fd77cdec824361d709d1
Bug 1363200 - JSAPI for realms: Add JS::Realm opaque type and GC rooting policy for it. r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/7556c838fcf7baf4000f54b068411874e1b9849d
Bug 1363200 - JSAPI for realms: JS_SetRealmNameCallback. r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8cb6a5096f3d472d1f65c41a93da4411742cc36
Bug 1363200 - JSAPI for realms: JS_SetVersionForCompartment() -> JS::SetVersionForCurrentRealm(). r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/687a55549ca080f579c0136c96ebff2b52fb6470
Bug 1363200 - JSAPI for realms: JS::Get/SetRealmPrivate(). r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e13a2a2660398e5eae0595fdaa696c9f5987e9c
Bug 1363200 - JSAPI for realms: JS::SetDestroyRealmCallback. r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4462198c312bb83f52d026dd8150466d0456dd0
Bug 1363200 - JSAPI for realms: Move SetAddonCallInterposition to the CompartmentPrivate. r=bholley

	Carsten Book [:Tomcat]
	Comment 28 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1ebe8f22c28f
https://hg.mozilla.org/mozilla-central/rev/7556c838fcf7
https://hg.mozilla.org/mozilla-central/rev/c8cb6a5096f3
https://hg.mozilla.org/mozilla-central/rev/687a55549ca0
https://hg.mozilla.org/mozilla-central/rev/1e13a2a26603
https://hg.mozilla.org/mozilla-central/rev/d4462198c312

	Jason Orendorff [:jorendorff] Assignee
	Comment 29 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6565e9b96b63786a5930a6b7a902c2c0e6792a82

	Jason Orendorff [:jorendorff] Assignee
	Comment 30 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dae4f2bf64c840a1ae3152a9adc859fe20054853

	Jason Orendorff [:jorendorff] Assignee
	Comment 31 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ec151408774c43bb5ee966a31b2ddece26aa8b2

	Jason Orendorff [:jorendorff] Assignee
	Comment 32 • 7 years ago

The entire purpose of this patch is to support accessing this bit from
WrapperFactory (see the last hunk) without going through a particular
scope.

	Jason Orendorff [:jorendorff] Assignee
	Comment 33 • 7 years ago

In the new order, it will be a compartment-level bit rather than a
realm-level bit, so it does not belong on the Scope.

	Jason Orendorff [:jorendorff] Assignee
	Comment 35 • 7 years ago

This also introduces JS::GetObjectRealmOrNull, which returns an object's realm,
or null if the object is a cross-compartment wrapper. In the new order,
wrappers can't have realms, since they must be shared across all realms in a
compartment. We're introducing this new function early (even though it's
*currently* possible to assign a realm to wrappers) in order to see in
advance if the possibility of returning null will cause problems.
(It looks like it won't.)

	Bobby Holley (:bholley)
	Comment 36 • 7 years ago

I'm pretty swamped right now with stylo stuff. Blake, do you have time to review these patches?

	Jason Orendorff [:jorendorff] Assignee
	Comment 38 • 7 years ago

The big difference here is that AutoEnterRealm will crash hard if given a CCW.
No problems so far, even during development.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 39 • 7 years ago

I have time to review these patches. I'll get to them tomorrow.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 40 • 7 years ago

Comment on attachment 8896015 [details] [diff] [review]
JSAPI for realms: Change a few XPConnect methods to take Realm arguments instead of JSCompartments

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

::: caps/nsScriptSecurityManager.cpp
@@ +1267,5 @@
>          return NS_OK;
>      }
>  
>      // We give remote-XUL whitelisted domains a free pass here. See bug 932906.
> +    JS::Rooted<JS::Realm*> contextRealm(cx, JS::GetCurrentRealmOrNull(cx));

In these cases where we're now calling GetCurrentRealmOrNull, do we know that we're not dealing with CCWs? I wonder if it wouldn't be worth it to add a convenience function that asserts GetCurrentRealmOrNull returns non-null and to call that to make this clearer.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 41 • 7 years ago

Comment on attachment 8896407 [details] [diff] [review]
JSAPI for realms: Split xpc::RealmPrivate from xpc::CompartmentPrivate

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2690,4 @@
>  {
> +    // Get the current compartment private into an AutoPtr (which will do the
> +    // cleanup for us), and null out the private field.
> +    nsAutoPtr<RealmPrivate> priv(RealmPrivate::Get(realm));

Dumb question: are we supposed to be using UniquePtr instead of AutoPtr now?

::: js/xpconnect/src/xpcprivate.h
@@ +3185,5 @@
>  {
>      MOZ_RELEASE_ASSERT(IsInAutomation());
>  }
>  
> +class RealmPrivate

For posterity (I asked about this on IRC). It would be good to add a comment here that explains when to choose RealmPrivate over CompartmentPrivate, since it isn't exactly obvious.

	Jason Orendorff [:jorendorff] Assignee
	Comment 42 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaae04ca26005576a5130dfe57d73e53823e4cdb

	Jason Orendorff [:jorendorff] Assignee
	Comment 43 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aeacc25ee569a03e467f5d44e23206da88badd1

	Jason Orendorff [:jorendorff] Assignee
	Comment 44 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab8612612a78de8de37e1dcab8104ef921a76d91

	Jason Orendorff [:jorendorff] Assignee
	Comment 45 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a313fe5f830d9c1d0a8d89902281014456f536

	Jason Orendorff [:jorendorff] Assignee
	Comment 46 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c2e346e46cc550318440d23e7d982c9ea4833ff

	Jason Orendorff [:jorendorff] Assignee
	Comment 47 • 7 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #40)
> ::: caps/nsScriptSecurityManager.cpp
> > +    JS::Rooted<JS::Realm*> contextRealm(cx, JS::GetCurrentRealmOrNull(cx));
> 
> In these cases where we're now calling GetCurrentRealmOrNull, do we know
> that we're not dealing with CCWs? I wonder if it wouldn't be worth it to add
> a convenience function that asserts GetCurrentRealmOrNull returns non-null
> and to call that to make this clearer.

There are two cases -- GetCurrentRealmOrNull and GetObjectRealmOrNull.

*   CurrentRealm: I don't know what static reasoning ensures that GetCurrentRealmOrNull(cx) doesn't return null at a given point in the code. I imagine there has to be some guard object on the stack, or else we were called from a JSNative.

*   ObjectRealm: In many cases it's "obvious" that the object isn't a CCW, e.g. because it's a DOM object, or a global object, or because we got it by calling an Unwrap function. In other cases it's less certain. But in no case is the invariant enforced by the C++ type system.

In both cases, the lack of existing invariants and my unfamiliarity make it hard to guess. I'll add MOZ_RELEASE_ASSERTs for now (the only reason I didn't already is that in all these cases we're about to touch the realm, so we're looking at an immediate crash if it's null).

	Jason Orendorff [:jorendorff] Assignee
	Comment 48 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5106b2249c767984e908bf0c2b57a1a6e206ac8a

	Jason Orendorff [:jorendorff] Assignee
	Comment 49 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8e84b812b6d04205ff284a63ccd3fdb2c35d426

	Jason Orendorff [:jorendorff] Assignee
	Comment 50 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d394e8d01d4df4c9a5b5a6db1d63864930cd409
Bug 1363200 - JSAPI for realms: Clone hasInterposition bit from the scope to the CompartmentPrivate. r=mrbkap

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc8ef854b497929161250f9aa067228abe85e63
Bug 1363200 - JSAPI for realms: Move mIsContentXBLScope to the CompartmentPrivate. r=mrbkap

https://hg.mozilla.org/integration/mozilla-inbound/rev/0495d0052c24fcc9fb7f3d6d96b06314e8fd3234
Bug 1363200 - JSAPI for realms: Move XPCWrappedNativeScope::mIsAddonScope to CompartmentPrivate. r=mrbkap

	Jason Orendorff [:jorendorff] Assignee
	Comment 51 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55b91b7eeef0010b45a8a95a62c81cd36e40a357

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 52 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8d394e8d01d4
https://hg.mozilla.org/mozilla-central/rev/0cc8ef854b49
https://hg.mozilla.org/mozilla-central/rev/0495d0052c24

	Jason Orendorff [:jorendorff] Assignee
	Comment 53 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3792a715a50dcd211d5a72ae3be1bf58a9de8ebf
Bug 1363200 - JSAPI for realms: Change a few XPConnect methods to take Realm arguments instead of JSCompartments. r=mrbkap

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8695eebf178b3ec51c250cf460053b58d7f2de
Bug 1363200 - JSAPI for realms: Split xpc::RealmPrivate from xpc::CompartmentPrivate. r=mrbkap

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 54 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3792a715a50d
https://hg.mozilla.org/mozilla-central/rev/4b8695eebf17

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

I think this is fixed now; if there's more we should file new bugs.