Change JSAPI to distinguish realms and compartments

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(12 attachments)

10.03 KB, patch
sfink
: review+
Details | Diff | Splinter Review
6.70 KB, patch
sfink
: review+
Details | Diff | Splinter Review
6.06 KB, patch
sfink
: review+
Details | Diff | Splinter Review
3.53 KB, patch
sfink
: review+
Details | Diff | Splinter Review
10.51 KB, patch
sfink
: review+
Details | Diff | Splinter Review
8.36 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.05 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
16.16 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.73 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
14.43 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
26.76 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
17.48 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
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.
Assignee

Updated

2 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
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.
Attachment #8890331 - Flags: review?(sphink) → review+
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.)
Attachment #8890333 - Flags: review?(sphink) → review+
Assignee

Comment 17

2 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.
Attachment #8890490 - Flags: review?(sphink)
Attachment #8890494 - Flags: review?(bobbyholley) → review+
Attachment #8890489 - Flags: review?(sphink) → review+
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.
Attachment #8890490 - Flags: review?(sphink) → review+
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.
Attachment #8890491 - Flags: review?(sphink) → review+
Assignee

Comment 26

2 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.
Assignee

Comment 27

2 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
Assignee

Updated

2 years ago
Keywords: leave-open
Assignee

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 32

2 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.
Attachment #8896008 - Flags: review?(bobbyholley)
Assignee

Comment 33

2 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.
Attachment #8896009 - Flags: review?(bobbyholley)
Assignee

Comment 35

2 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.)
Attachment #8896015 - Flags: review?(bobbyholley)
I'm pretty swamped right now with stylo stuff. Blake, do you have time to review these patches?
Flags: needinfo?(jorendorff)
Assignee

Updated

2 years ago
Flags: needinfo?(jorendorff) → needinfo?(mrbkap)
Assignee

Comment 38

2 years ago
The big difference here is that AutoEnterRealm will crash hard if given a CCW.
No problems so far, even during development.
Attachment #8896414 - Flags: review?(mrbkap)
I have time to review these patches. I'll get to them tomorrow.
Flags: needinfo?(mrbkap)
Attachment #8896008 - Flags: review?(bobbyholley) → review?(mrbkap)
Attachment #8896009 - Flags: review?(bobbyholley) → review?(mrbkap)
Attachment #8896011 - Flags: review?(bobbyholley) → review?(mrbkap)
Attachment #8896015 - Flags: review?(bobbyholley) → review?(mrbkap)
Attachment #8896008 - Flags: review?(mrbkap) → review+
Attachment #8896009 - Flags: review?(mrbkap) → review+
Attachment #8896011 - Flags: review?(mrbkap) → review+
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.
Attachment #8896015 - Flags: review?(mrbkap) → review+
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.
Attachment #8896407 - Flags: review?(mrbkap) → review+
Attachment #8896414 - Flags: review?(mrbkap) → review+
Assignee

Comment 47

2 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).
Assignee

Comment 50

2 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
Assignee

Comment 53

2 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
Assignee

Updated

Last year
Priority: -- → P3
I think this is fixed now; if there's more we should file new bugs.
Status: REOPENED → RESOLVED
Closed: 2 years agoLast year
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.