Closed Bug 1461938 Opened 2 years ago Closed Last year

Start using JS::Realm instead of JSCompartment

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(40 files, 2 obsolete files)

31.14 KB, patch
luke
: review+
Details | Diff | Splinter Review
5.49 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.74 KB, patch
luke
: review+
Details | Diff | Splinter Review
52.62 KB, patch
luke
: review+
Details | Diff | Splinter Review
59.41 KB, patch
jonco
: review+
Details | Diff | Splinter Review
54.62 KB, patch
jonco
: review+
Details | Diff | Splinter Review
24.05 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
23.54 KB, patch
luke
: review+
Details | Diff | Splinter Review
22.29 KB, patch
luke
: review+
Details | Diff | Splinter Review
11.15 KB, patch
jonco
: review+
Details | Diff | Splinter Review
7.71 KB, patch
luke
: review+
Details | Diff | Splinter Review
20.66 KB, patch
luke
: review+
Details | Diff | Splinter Review
12.22 KB, patch
anba
: review+
Details | Diff | Splinter Review
4.92 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.06 KB, patch
jonco
: review+
Details | Diff | Splinter Review
2.87 KB, patch
anba
: review+
Details | Diff | Splinter Review
12.80 KB, patch
luke
: review+
Details | Diff | Splinter Review
17.78 KB, patch
luke
: review+
Details | Diff | Splinter Review
10.58 KB, patch
jonco
: review+
Details | Diff | Splinter Review
35.27 KB, patch
luke
: review+
Details | Diff | Splinter Review
13.97 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
10.88 KB, patch
anba
: review+
Details | Diff | Splinter Review
68.55 KB, patch
luke
: review+
Details | Diff | Splinter Review
8.05 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
5.34 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.98 KB, patch
luke
: review+
Details | Diff | Splinter Review
20.72 KB, patch
jonco
: review+
Details | Diff | Splinter Review
15.82 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.83 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
7.04 KB, patch
jonco
: review+
Details | Diff | Splinter Review
9.18 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
81.61 KB, patch
luke
: review+
Details | Diff | Splinter Review
40.13 KB, patch
jonco
: review+
Details | Diff | Splinter Review
4.89 KB, patch
jonco
: review+
Details | Diff | Splinter Review
22.24 KB, patch
luke
: review+
Details | Diff | Splinter Review
75.59 KB, patch
luke
: review+
Details | Diff | Splinter Review
6.15 KB, patch
jonco
: review+
Details | Diff | Splinter Review
15.24 KB, patch
luke
: review+
Details | Diff | Splinter Review
28.76 KB, patch
luke
: review+
Details | Diff | Splinter Review
13.00 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
No description provided.
This gives us cx->realm(). cx->compartment() still works but most of its uses will incrementally disappear.
Attachment #8976094 - Flags: review?(luke)
Attachment #8976095 - Flags: review?(luke)
objects/scripts/groups now all have a realm() method.
Attachment #8976096 - Flags: review?(luke)
Now we can start moving stuff from JSCompartment to JS::Realm; this patch starts with the RealmOptions.
Attachment #8976097 - Flags: review?(luke)
This is mostly renaming atoms compartment to atoms realm, except:

* We used to have both runtime->isAtomsCompartment(comp) and comp->isAtomsCompartment(). This patch standardizes on realm->isAtomsRealm() everywhere.

* JSRuntime::isAtomsCompartment still exists, but it still takes a JSCompartment*. It's used for some compartment/wrapper asserts. I added a comment explaining it can probably go away at some point because eventually the atoms realm likely won't have a compartment at all.

* At this point in the patch stack, JS::Realm inherits from JSCompartment. This is temporary. The GetRealmForCompartment and GetCompartmentForRealm calls will also disappear when we use realms in more places.
Attachment #8976176 - Flags: review?(jcoppeard)
Attachment #8976094 - Flags: review?(luke) → review+
Comment on attachment 8976095 [details] [diff] [review]
Part 2 - Store JS::Realm* in JSScript

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

I really like this incremental approach you're using, btw.
Attachment #8976095 - Flags: review?(luke) → review+
Attachment #8976096 - Flags: review?(luke) → review+
Attachment #8976097 - Flags: review?(luke) → review+
Comment on attachment 8976176 [details] [diff] [review]
Part 5 - Rename atoms compartment to atoms realm

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

Looks great, but there were lots of preexising uses of "compartment" where I think "zone" would make more sense.

::: js/src/gc/GC.cpp
@@ +3387,5 @@
>      }
>  #endif
>  
>      if (zone->isAtomsZone()) {
> +        /* We can't do a zone GC of the atoms realm. */

This comment was wrong already - we can only GC to the granularity of a zone.  It should say "atoms zone".

@@ +5333,5 @@
>      markWeakReferencesInCurrentGroup(gcstats::PhaseKind::SWEEP_MARK_WEAK);
>  
>      /*
>       * Change state of current group to MarkGray to restrict marking to this
> +     * group.  Note that there may be pointers to the atoms realm, and

This should say "atoms zone" too.

@@ +5338,2 @@
>       * these will be marked through, as they are not marked with
>       * MarkCrossCompartmentXXX.

Pre-existing, this should say TraceCrossCompartmentEdge not MarkCrossCompartmentXXX.

::: js/src/jit/Ion.cpp
@@ +336,5 @@
>  JitRuntime::debugTrapHandler(JSContext* cx)
>  {
>      if (!debugTrapHandler_) {
>          // JitRuntime code stubs are shared across compartments and have to
> +        // be allocated in the atoms realm.

Pre-exisiting, should be "atoms zone"

@@ +586,5 @@
>  JitRuntime::Trace(JSTracer* trc, AutoLockForExclusiveAccess& lock)
>  {
>      MOZ_ASSERT(!JS::CurrentThreadIsHeapMinorCollecting());
>  
> +    // Shared stubs are allocated in the atoms realm, so do not iterate

Ditto.

::: js/src/jsapi.cpp
@@ +7575,5 @@
>          return nullptr;
>  
>      GlobalObject* global = activation->compartment()->maybeGlobal();
>  
> +    // Noone should be running code in the atoms realm or running code in

Pre-existing: "no one"

::: js/src/vm/HelperThreads.cpp
@@ +664,5 @@
>  bool
>  js::OffThreadParsingMustWaitForGC(JSRuntime* rt)
>  {
>      // Off thread parsing can't occur during incremental collections on the
> +    // atoms realm, to avoid triggering barriers. (Outside the atoms realm, the

"atoms zone"

@@ +783,5 @@
>  bool
>  StartOffThreadParseTask(JSContext* cx, ParseTask* task, const ReadOnlyCompileOptions& options)
>  {
>      // Suppress GC so that calls below do not trigger a new incremental GC
> +    // which could require barriers on the atoms realm.

"atoms zone"

::: js/src/vm/JSCompartment-inl.h
@@ +97,5 @@
>          return true;
>  
>      /*
>       * Symbols are GC things, but never need to be wrapped or copied because
> +     * they are always allocated in the atoms realm. They still need to

"atoms zone"

::: js/src/vm/JSCompartment.cpp
@@ +130,5 @@
>  bool
>  JSCompartment::init(JSContext* maybecx)
>  {
>      /*
> +     * maybecx is null when called to create the atoms realm from

This is talking about an actual compartment so should say "atoms compartment" or "compartment for the atoms zone" or maybe "compartment for the atoms realm".

@@ +165,5 @@
>  
>  jit::JitRuntime*
>  JSRuntime::createJitRuntime(JSContext* cx)
>  {
> +    // The shared stubs are created in the atoms realm, which may be

"atoms zone"

::: js/src/vm/JSCompartment.h
@@ +650,5 @@
>      }
>  
>      /* The global object for this compartment.
>       *
> +     * This returns nullptr if this is the atoms realm.  (The global_

"atoms compartment" since we're talking about an actual compartment, or whatever we decide to call this compartment.

::: js/src/vm/JSContext.h
@@ +556,5 @@
>      // of any such threads also inhibits collection of atoms. We don't scan the
>      // stacks of exclusive threads, so we need to avoid collecting their
>      // objects in another way. The only GC thing pointers they have are to
>      // their exclusive compartment (which is not collected) or to the atoms
> +    // compartment. Therefore, we avoid collecting the atoms realm when

"atoms zone"

::: js/src/vm/Runtime.h
@@ +704,2 @@
>      // well as runtime wide IonCode stubs. Modifying the contents of this
> +    // realm requires the calling thread to use AutoLockForExclusiveAccess.

Probably should say "zone".

@@ +707,3 @@
>  
>      // Set of all live symbols produced by Symbol.for(). All such symbols are
> +    // allocated in the atomsRealm. Reading or writing the symbol

Ditto.
Attachment #8976176 - Flags: review?(jcoppeard) → review+
This patch is a bit bigger than I expected, but it's mostly just moving fields/methods and updating comments.

I also added some iterator classes to iterate over all realms in a Zone. Right now that's pretty easy to do because we can just base it on the compartment iterators but once a compartment can have multiple realms we'll need to change them to support this (but the public interface will stay the same).
Attachment #8976497 - Flags: review?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #7)
> there were lots of preexising uses of "compartment" where I
> think "zone" would make more sense.

Good point. We discussed this on IRC and it seems it doesn't make a lot of sense to have an atoms realm/compartment nowadays. It would be nice to refactor so we only have an atoms *zone*. (The self-hosting and off-thread parsing zones do a lot of actual VM work and GC object/script allocation so there it makes sense to have a global/realm/compartment.)
Attachment #8976497 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77be093ecde9
part 1 - Store JS::Realm* instead of JSCompartment* in JSContext. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec876d855d4
part 2 - Store JS::Realm* instead of JSCompartment* in JSScript. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe673f265b9b
part 3 - Store JS::Realm* instead of JSCompartment* in ObjectGroup. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/01fcd7343687
part 4 - Move RealmOptions from JSCompartment to JS::Realm. r=luke
Keywords: leave-open
Very straight-forward, except I also had to move |init| and |addSizeOfIncludingThis|. We can rely on the fact that Realm inherits from JSCompartment so we won't have to touch these methods for every other field we move.
Attachment #8976653 - Flags: review?(jwalden+bmo)
(Fix a comment typo.)
Attachment #8976653 - Attachment is obsolete: true
Attachment #8976653 - Flags: review?(jwalden+bmo)
Attachment #8976655 - Flags: review?(jwalden+bmo)
Attachment #8976655 - Flags: review?(jwalden+bmo) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32fc25dec892
part 5 - Some atoms compartment/realm related changes. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9e1b118451
part 6 - Move global object from JSCompartment to JS::Realm. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/4de1a5113482
part 7 - Move varNames from JSCompartment to JS::Realm. r=jwalden
Attachment #8976990 - Flags: review?(luke)
Comment on attachment 8976990 [details] [diff] [review]
Part 8 - Move some more fields

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

I think it was already agreed on in a previous email thread, but I like this trend of putting private fields together and separate from the public methods.
Attachment #8976990 - Flags: review?(luke) → review+
Attachment #8976991 - Flags: review?(luke) → review+
Duplicate of this bug: 1363206
The mapsWithNurseryMemory and setsWithNurseryMemory Vectors can be moved to the Nursery class; this saves some memory and is also consistent with the dictionaryModeObjects_ Vector there.

It avoids having to think about cx->realm() being different from obj->realm() when adding an object to the Vector.
Attachment #8979256 - Flags: review?(jcoppeard)
Attachment #8979257 - Flags: review?(luke) → review+
In theory the dtoa cache could be moved to the Zone, but it doesn't really matter.
Attachment #8979262 - Flags: review?(andrebargull)
Comment on attachment 8979260 [details] [diff] [review]
Part 12 - Move script maps to JS::Realm

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

::: js/src/vm/JSCompartment.cpp
@@ +120,5 @@
> +Realm::~Realm()
> +{
> +    js_delete(scriptCountsMap);
> +    js_delete(scriptNameMap);
> +    js_delete(debugScriptMap);

nit: just like you're tweaking conventions to put private fields together and use default member init, what about establishing a convention to use UniquePtr?  This can help propagate out to the code paths that create the uniquely-owned objects which tends to make everything cleaner and less error-prone.
Attachment #8979260 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #24)
> nit: just like you're tweaking conventions to put private fields together
> and use default member init, what about establishing a convention to use
> UniquePtr?  This can help propagate out to the code paths that create the
> uniquely-owned objects which tends to make everything cleaner and less
> error-prone.

Sure, I can post a follow-up patch for this to not break my patch stack too much :)
This field could probably also be moved to Zone at some point, but it doesn't matter much.
Attachment #8979270 - Flags: review?(jcoppeard)
This one is a bit less mechanical because it deals with compartment (realm) revival and I had to rename a bunch of GC things and fix up some comments.
Attachment #8979273 - Flags: review?(jcoppeard)
I filed bug 1463163 to fix the rest of the CreateArraySpecies code, because it needs an explicit realm check, but that should wait until we can actually test it.
Attachment #8979274 - Flags: review?(andrebargull)
Attachment #8979256 - Flags: review?(jcoppeard) → review+
Attachment #8979270 - Flags: review?(jcoppeard) → review+
Attachment #8979281 - Flags: review?(luke) → review+
Comment on attachment 8979273 [details] [diff] [review]
Part 16 - Move some GC fields to JS::Realm

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

::: js/src/gc/GC.cpp
@@ +4433,5 @@
>  {
> +    // XXX: should this stay as markCompartments and PhaseKind::MARK_COMPARTMENTS?
> +    //
> +    // XXX We're setting flags on realms but we're doing this by marking cross
> +    // XXX *compartment* wrappers...

That's an intersting point.  I think the final setup is that there will be multiple realms in a compartment and that thet may contain pointers between themelves without going thorough a wrapper, right?  In that case we can only tell what is dead down to the level of a compartment, and this should all operate in terms of compartments.  That would mean leaving the 'marked' state on the compartment.
Attachment #8979273 - Flags: review?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #31)
> That's an intersting point.  I think the final setup is that there will be
> multiple realms in a compartment and that thet may contain pointers between
> themelves without going thorough a wrapper, right?

Correct.

> In that case we can only
> tell what is dead down to the level of a compartment, and this should all
> operate in terms of compartments.  That would mean leaving the 'marked'
> state on the compartment.

Hm when two realms will be in the same compartment, I think things should still work because they'll be part of the same Zone so a GC is guaranteed to find all the pointers for objects/groups/scripts owned by a realm. The code here is accounting for incoming CCW edges but because each CCW target has an associated realm, it's fine to just mark that realm as alive.
Or is the question what if we have a CCW RealmX -> RealmY and the target object contains a pointer RealmY -> RealmZ and RealmY and RealmZ are same-compartment? Hm that would have to keep RealmZ alive...
What if we keep scheduledForDestruction and maybeAlive on the compartment, and move the "marked" flag to the Realm? I think that avoids the issue and would still let us collect dead realms before other realms in the compartment are dead.
Flags: needinfo?(jcoppeard)
(In reply to Jan de Mooij [:jandem] from comment #33)
> Or is the question what if we have a CCW RealmX -> RealmY and the target
> object contains a pointer RealmY -> RealmZ and RealmY and RealmZ are
> same-compartment? Hm that would have to keep RealmZ alive...

Yes, that's the problem.

> What if we keep scheduledForDestruction and maybeAlive on the compartment,
> and move the "marked" flag to the Realm? 

Yes that sounds fine.  We'll need to set maybeAlive if any realms in the compartment are marked though so I think we won't end up collecting dead realms as agressively as we do now.  I don't expect that will be a big problem though.
Flags: needinfo?(jcoppeard)
This uses UniquePtr both for the maps themselves and their values.
Attachment #8979319 - Flags: review?(luke)
Thanks for catching that, Jon. Would probably have caused some weird regressions otherwise.
Attachment #8979273 - Attachment is obsolete: true
Attachment #8979328 - Flags: review?(jcoppeard)
Comment on attachment 8979319 [details] [diff] [review]
Part 19 - Use UniquePtr for script maps

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

That was a non-trivial amount work, but resulting code looks nicer; thanks for making the change!
Attachment #8979319 - Flags: review?(luke) → review+
Attachment #8979262 - Flags: review?(andrebargull) → review+
Attachment #8979263 - Flags: review?(andrebargull) → review+
Attachment #8979274 - Flags: review?(andrebargull) → review+
Comment on attachment 8979328 [details] [diff] [review]
Part 16 - Move marked flag to JS::Realm

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

Great.  Thanks for adding the comment.
Attachment #8979328 - Flags: review?(jcoppeard) → review+
I asked bz about this and the principals really have to be on the realm, even for CPO, so this patch moves them there. The isSystem flag is sort of tied to the principals so I think it makes sense to move it there too.

This adds a number of GetRealmForCompartment calls, these are places we'll need to fix up later; most of them should be trivial.
Attachment #8979444 - Flags: review?(luke)
Also some minor cleanup:

* isSelfHosting => isSelfHostingRealm_ (private), for consistency with isAtomsRealm.

* I removed JSRuntime::isSelfHostingCompartment as it's just another way to answer realm->isSelfHostingRealm(). This is similar to what part 5 did for the atoms realm.
Attachment #8979464 - Flags: review?(evilpies)
Attachment #8979464 - Attachment description: Part 21 - Move some isSelfHosting and selfHostingScriptSource → Part 21 - Move isSelfHosting and selfHostingScriptSource
Attachment #8979469 - Flags: review?(andrebargull)
This moves a number of debugging-related methods to JS::Realm and uses realms instead of compartments in the debugger code in a few places.
Attachment #8979523 - Flags: review?(luke)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a6d73cb73e9
part 8 - Move some more fields from JSCompartment to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac021598156d
part 9 - Turn wasm::Compartment into wasm::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bceed84e639
part 10 - Move {maps,sets}WithNurseryMemory from JSCompartment to Nursery. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f16fd31d246
part 11 - Move RealmStats from JSCompartment to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce426cc34287
part 12 - Move script maps from JSCompartment to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/98ad6a903862
part 13 - Move dtoaCache and newProxyCache from JSCompartment to JS::Realm. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/955716a67dc0
part 14 - Move warnedAboutStringGenericsMethods and firedOnNewGlobalObject from JSCompartment to JS::Realm. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb25032815b
part 15 - Move lastAnimationTime from JSCompartment to JS::Realm. r=jonco
I also cleaned up the interface a bit: now the RNG is private and you have to call getOrCreateRandomNumberGenerator to get a reference to it, so callers don't have to remember to call ensureRandomNumberGenerator first.

Note that Ion inlining of MRandom is still fine because scripts are tied to a single realm.
Attachment #8979551 - Flags: review?(evilpies)
This code is only used by about:performance and I filed bug 1406872 a while ago to consider removing it. The Stopwatch may behave a bit differently with cross-realm calls (no Stopwatch for the target realm) but that's probably fine - about:performance only has tab granularity anyway.
Attachment #8979581 - Flags: review?(luke)
Attachment #8979581 - Attachment description: Part 16 - Move performanceMonitoring to JS::Realm → Part 26 - Move performanceMonitoring to JS::Realm
Attachment #8979444 - Flags: review?(luke) → review+
Attachment #8979523 - Flags: review?(luke) → review+
Attachment #8979581 - Flags: review?(luke) → review+
This uses UniquePtr for the remaining pointers in JSCompartment that were deleted/freed in the destructor.

The DebugEnvironments GCManagedDeletePolicy was also problematic, similar to what we discussed on IRC. DebugEnvironments is only used in JSCompartment so it should already have GC lifetime.
Attachment #8979653 - Flags: review?(jcoppeard)
Attachment #8979464 - Flags: review?(evilpies) → review+
Attachment #8979551 - Flags: review?(evilpies) → review+
Attachment #8979563 - Flags: review?(evilpies) → review+
Attachment #8979469 - Flags: review?(andrebargull) → review+
Comment on attachment 8979653 [details] [diff] [review]
Part 27 - Use UniquePtr more

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

The DeletePolicy thing is a pain in general as it's easy to forget to add it to types when necessary.  But that's not related to this patch.

I agree that DebugEnvironments doesn't need this policy.

::: js/src/jsapi-tests/testGCGrayMarking.cpp
@@ +14,5 @@
>  using namespace js::gc;
>  
> +namespace js {
> +
> +struct GCManagedObjectWeakMap : public ObjectWeakMap

I'm starting to think that "GCManaged" is the wrong name for this concept, but I'll file a different bug for that.

::: js/src/vm/JSCompartment.h
@@ +753,5 @@
>          return offsetof(JSCompartment, regExps);
>      }
>  
>      /* Bookkeeping information for debug scope objects. */
> +    js::UniquePtr<js::DebugEnvironments> debugEnvs;

Would it be simpler to call this debugEnvs_ and give it an accessor to avoid all the get() calls?
Attachment #8979653 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca76ab5c29dc
part 16 - Move marked flag from JSCompartment to JS::Realm. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a317ad122f8
part 17 - Move ArraySpeciesLookup from JSCompartment to JS::Realm. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f59efaed142
part 18 - Move objectMetadataState_ from JSCompartment to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6c61be7966
part 19 - Use UniquePtr for script maps. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/716d49972dba
part 20 - Move principals and isSystem from JSCompartment to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/19295db05a92
part 21 - Move isSelfHosting and selfHostingScriptSource from JSCompartment to JS::Realm. r=evilpie
(In reply to Jon Coppeard (:jonco) from comment #49)
> Would it be simpler to call this debugEnvs_ and give it an accessor to avoid
> all the get() calls?

Good idea. I'll make this change when we move this to Realm.
I double checked with bz that they really want this on JS::Realm instead of JSCompartment.

I'll rename the js::SetCompartmentValidAccessPtr friend API in a follow-up bug.
Attachment #8979873 - Flags: review?(evilpies)
See patch for why this moves to the zone.
Attachment #8979875 - Flags: review?(jwalden+bmo)
Attachment #8979874 - Flags: review?(jcoppeard) → review+
JSCompartment has some maps (and the enumerators list) that store info related to a single object. With multiple realms within a compartment, one potential source of bugs is code confusing cx->realm() vs obj->realm().

To avoid this class of bugs for these object-related fields, I grouped them in ObjectRealm. ObjectRealm is a private field of Realm and the only way to get hold of an ObjectRealm is ObjectRealm::get(obj), where we use obj->realm().
Attachment #8979881 - Flags: review?(jcoppeard)
At this point there are a handful of stragglers left, so probably 5 more patches or so for this bug...
Comment on attachment 8979873 [details] [diff] [review]
Part 29 - Move validAccessPtr to JS::Realm

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

Curious, I have never noticed this before.
Attachment #8979873 - Flags: review?(evilpies) → review+
Comment on attachment 8979881 [details] [diff] [review]
Part 33 - Use ObjectRealm for object data

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

Good idea.

::: js/src/vm/JSCompartment.h
@@ +771,5 @@
> +
> +    js::LexicalEnvironmentObject*
> +    getOrCreateNonSyntacticLexicalEnvironment(JSContext* cx, js::HandleObject enclosing);
> +    js::LexicalEnvironmentObject* getNonSyntacticLexicalEnvironment(JSObject* enclosing) const;
> +};

Can you delete the copy constructor / assignment operator so people can't accidentally copy these?
Attachment #8979881 - Flags: review?(jcoppeard) → review+
Hm thinking more about |enumerators|, that one should probably stay on the compartment, along with the iteratorCache.

I keep going back-and-forth on this, but PropertyIteratorObject is not exposed (directly) to script so it should be fine to reuse iterators when iterating an object in realm X and then in realm Y. More importantly, it means we don't have to worry about the case where the object and its iterator are in different realms - the patch I posted got that wrong.
Well nevermind; I'll just use ni->obj in RegisterEnumerator and then in sweepNativeIterators assert same-realm, that should be sufficient.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d23c763dfa4b
part 22 - Move template objects from JSCompartment to JS::Realm. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/20512f4a1de5
part 23 - Move debugModeBits from JSCompartment to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa9543ec7f36
part 24 - Move randomNumberGenerator from JSCompartment to JS::Realm. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec090f5b477
part 25 - Move randomKeyGenerator_ from JSCompartment to JS::Realm. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/51743cb0e425
part 26 - Move performanceMonitoring from JSCompartment to JS::Realm. r=luke
Attachment #8979871 - Flags: review?(luke) → review+
Attachment #8979876 - Flags: review?(luke) → review+
(In reply to Jon Coppeard (:jonco) from comment #61)
> Can you delete the copy constructor / assignment operator so people can't
> accidentally copy these?

Oh nevermind, the UniquePtrs will stop the compiler generating a copy constructor for this because they have deleted copy constructors themselves.
(In reply to Jon Coppeard (:jonco) from comment #65)
> Oh nevermind, the UniquePtrs will stop the compiler generating a copy
> constructor for this because they have deleted copy constructors themselves.

True, but I had already changed the patch to add this to Realm and ObjectRealm so will just keep it :)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6335bbd6c3
part 27 - Use UniquePtr for various compartment pointers. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/84f6e67dcd1c
part 28 - Rename LCovCompartment to LCovRealm and move to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/72b513e25bec
part 29 - Move validAccessPtr to JS::Realm. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/def02132a4cf
part 30 - Move globalWriteBarriered to JS::Realm. r=jonco
Attachment #8979875 - Flags: review?(jwalden+bmo) → review+
I made this private and added ObjectGroupRealm::get(group) and ObjectGroupRealm::getForNewObject(cx) accessors. I also added some same-realm asserts.

The TI/ObjectGroup code is definitely one of the areas where I want to spend a day or so adding assertions and auditing the code for potential cross-realm issues, but that will be easier to do when we can actually test this.
Attachment #8980210 - Flags: review?(luke)
This way we can ensure the UnboxedLayout for a group is always in the group's realm's unboxedLayout list.
Attachment #8980211 - Flags: review?(jcoppeard)
The last one. This also tidies up JSCompartment (what's left of it) a bit.

I changed most checks/asserts in the SavedStacks code to realm checks because that's the stronger invariant, but this is also something I should probably audit more later.
Attachment #8980213 - Flags: review?(luke)
Attachment #8980208 - Flags: review?(jcoppeard) → review+
Attachment #8980211 - Flags: review?(jcoppeard) → review+
evilpie suggested using private inheritance for Realm/JSCompartment, to avoid implicit conversions. The patch makes this change and fixes a number of places where we relied on the implicit conversion. These are mostly just places where we have to push JS::Realm* instead of JSCompartment* to more code.
Attachment #8980221 - Flags: review?(evilpies)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b350681b94
part 31 - Move detachedTypedObjects flag to JS::Zone. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c669b99bd1
part 32 - Rename JitCompartment to JitRealm and move to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a363dbae273
part 33 - Introduce ObjectRealm and use it for some fields. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/dff489ff6e4c
part 34 - Move IteratorCache from JSCompartment to ObjectRealm. r=jonco
Attachment #8980209 - Flags: review?(luke) → review+
Comment on attachment 8980210 [details] [diff] [review]
Part 36 - ObjectGroupCompartment => ObjectGroupRealm

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

Nice to be explicit with the two ways of requesting a realm.
Attachment #8980210 - Flags: review?(luke) → review+
Attachment #8980212 - Flags: review?(luke) → review+
Attachment #8980213 - Flags: review?(luke) → review+
Attachment #8980221 - Flags: review?(evilpies) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe5ca35982d
part 35 - Move debugEnvs to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/12a249851bed
part 36 - Rename ObjectGroupCompartment to ObjectGroupRealm and move to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c8a89ecb9d
part 37 - Move unboxedLayouts list to ObjectGroupRealm. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce6ce0e6291
part 38 - Rename RegExpCompartment to RegExpRealm and move to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/385b54738fb6
part 39 - Move savedStacks_ to JS::Realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/d99b7e4e8cd9
part 40 - Use private inheritance. r=evilpie
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.