1461938 - Start using JS::Realm instead of JSCompartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Store JS::Realm* in JSContext

This gives us cx->realm(). cx->compartment() still works but most of its uses will incrementally disappear.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Store JS::Realm* in JSScript

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Store JS::Realm* in ObjectGroup

objects/scripts/groups now all have a realm() method.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Move RealmOptions from JSCompartment to JS::Realm

Now we can start moving stuff from JSCompartment to JS::Realm; this patch starts with the RealmOptions.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Rename atoms compartment to atoms realm

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.

Comment 6

[Luke Wagner [:luke]]

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.

Comment 7

[Jon Coppeard (:jonco)]

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.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Move global from JSCompartment to JS::Realm

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

Comment 9

[Jan de Mooij [:jandem]]

(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.)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Move varNames from JSCompartment to JS::Realm

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.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Move varNames from JSCompartment to JS::Realm

(Fix a comment typo.)

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/77be093ecde9
https://hg.mozilla.org/mozilla-central/rev/1ec876d855d4
https://hg.mozilla.org/mozilla-central/rev/fe673f265b9b
https://hg.mozilla.org/mozilla-central/rev/01fcd7343687

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Move some more fields

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Turn wasm::Compartment into wasm::Realm

Comment 17

[Luke Wagner [: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.

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/32fc25dec892
https://hg.mozilla.org/mozilla-central/rev/9d9e1b118451
https://hg.mozilla.org/mozilla-central/rev/4de1a5113482

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Part 10 - Move {maps,sets}WithNurseryMemory to Nursery

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.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Part 11 - Move RealmStats to JS::Realm

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Part 12 - Move script maps to JS::Realm

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Part 13 - Move dtoaCache and newProxyCache to JS::Realm

In theory the dtoa cache could be moved to the Zone, but it doesn't really matter.

Comment 24

[Luke Wagner [:luke]]

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.

Comment 25

[Jan de Mooij [:jandem]]

(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 :)

Comment 26

[Jan de Mooij [:jandem]]

Attachment: Part 14 - Move warnedAboutStringGenericsMethods and firedOnNewGlobalObject to JS::Realm

These are pretty trivial.

Comment 27

[Jan de Mooij [:jandem]]

Attachment: Part 15 - Move lastAnimationTime to JS::Realm

This field could probably also be moved to Zone at some point, but it doesn't matter much.

Comment 28

[Jan de Mooij [:jandem]]

Attachment: Part 16 - Move some GC fields to JS::Realm

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.

Comment 29

[Jan de Mooij [:jandem]]

Attachment: Part 17 - Move ArraySpeciesLookup to JS::Realm

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.

Comment 30

[Jan de Mooij [:jandem]]

Attachment: Part 18 - Move objectMetadataState_ to JS::Realm

Comment 31

[Jon Coppeard (:jonco)]

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.

Comment 32

[Jan de Mooij [:jandem]]

(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.

Comment 33

[Jan de Mooij [:jandem]]

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

Comment 34

[Jan de Mooij [:jandem]]

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.

Comment 35

[Jon Coppeard (:jonco)]

(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.

Comment 36

[Jan de Mooij [:jandem]]

Attachment: Part 19 - Use UniquePtr for script maps

This uses UniquePtr both for the maps themselves and their values.

Comment 37

[Jan de Mooij [:jandem]]

Attachment: Part 16 - Move marked flag to JS::Realm

Thanks for catching that, Jon. Would probably have caused some weird regressions otherwise.

Comment 38

[Luke Wagner [:luke]]

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!

Comment 39

[Jon Coppeard (:jonco)]

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.

Comment 40

[Jan de Mooij [:jandem]]

Attachment: Part 20 - Move principals and isSystem

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.

Comment 41

[Jan de Mooij [:jandem]]

Attachment: Part 21 - Move isSelfHosting and selfHostingScriptSource

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.

Comment 42

[Jan de Mooij [:jandem]]

Attachment: Part 22 - Move template objects to JS::Realm

Comment 43

[Jan de Mooij [:jandem]]

Attachment: Part 23 - Move debugModeBits to JS::Realm

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.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
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

Comment 45

[Jan de Mooij [:jandem]]

Attachment: Part 24 - Move randomNumberGenerator to JS::Realm

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.

Comment 46

[Jan de Mooij [:jandem]]

Attachment: Part 25 - Move randomKeyGenerator_ to JS::Realm

Comment 47

[Jan de Mooij [:jandem]]

Attachment: Part 26 - Move performanceMonitoring to JS::Realm

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.

Comment 48

[Jan de Mooij [:jandem]]

Attachment: Part 27 - Use UniquePtr more

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.

Comment 49

[Jon Coppeard (:jonco)]

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?

Comment 50

[Pulsebot]

Pushed by [e-mail]:
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

Comment 51

[Jan de Mooij [:jandem]]

(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.

Comment 52

[Jan de Mooij [:jandem]]

Attachment: Part 28 - LCovCompartment => LCovRealm

Comment 53

[Jan de Mooij [:jandem]]

Attachment: Part 29 - Move validAccessPtr to JS::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.

Comment 54

[Jan de Mooij [:jandem]]

Attachment: Part 30 - Move globalWriteBarriered to JS::Realm

Comment 55

[Jan de Mooij [:jandem]]

Attachment: Part 31 - Move detachedTypedObjects to JS::Zone

See patch for why this moves to the zone.

Comment 56

[Jan de Mooij [:jandem]]

Attachment: Part 32 - JitCompartment => JitRealm

Comment 57

[Jan de Mooij [:jandem]]

Attachment: Part 33 - Use ObjectRealm for object data

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().

Comment 58

[Jan de Mooij [:jandem]]

At this point there are a handful of stragglers left, so probably 5 more patches or so for this bug...

Comment 59

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8a6d73cb73e9
https://hg.mozilla.org/mozilla-central/rev/ac021598156d
https://hg.mozilla.org/mozilla-central/rev/4bceed84e639
https://hg.mozilla.org/mozilla-central/rev/2f16fd31d246
https://hg.mozilla.org/mozilla-central/rev/ce426cc34287
https://hg.mozilla.org/mozilla-central/rev/98ad6a903862
https://hg.mozilla.org/mozilla-central/rev/955716a67dc0
https://hg.mozilla.org/mozilla-central/rev/6eb25032815b

Comment 60

[Tom S [:evilpie]]

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.

Comment 61

[Jon Coppeard (:jonco)]

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?

Comment 62

[Jan de Mooij [:jandem]]

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.

Comment 63

[Jan de Mooij [:jandem]]

Well nevermind; I'll just use ni->obj in RegisterEnumerator and then in sweepNativeIterators assert same-realm, that should be sufficient.

Comment 64

[Pulsebot]

Pushed by [e-mail]:
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

Comment 65

[Jon Coppeard (:jonco)]

(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.

Comment 66

[Jan de Mooij [:jandem]]

(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 :)

Comment 67

[Pulsebot]

Pushed by [e-mail]:
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

Comment 68

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ca76ab5c29dc
https://hg.mozilla.org/mozilla-central/rev/2a317ad122f8
https://hg.mozilla.org/mozilla-central/rev/9f59efaed142
https://hg.mozilla.org/mozilla-central/rev/fd6c61be7966
https://hg.mozilla.org/mozilla-central/rev/716d49972dba
https://hg.mozilla.org/mozilla-central/rev/19295db05a92
https://hg.mozilla.org/mozilla-central/rev/d23c763dfa4b
https://hg.mozilla.org/mozilla-central/rev/20512f4a1de5
https://hg.mozilla.org/mozilla-central/rev/aa9543ec7f36
https://hg.mozilla.org/mozilla-central/rev/aec090f5b477
https://hg.mozilla.org/mozilla-central/rev/51743cb0e425

Comment 69

[Jan de Mooij [:jandem]]

Attachment: Part 34 - Move IteratorCache to ObjectRealm

Comment 70

[Jan de Mooij [:jandem]]

Attachment: Part 35 - Move debugEnvs to JS::Realm

Comment 71

[Jan de Mooij [:jandem]]

Attachment: Part 36 - ObjectGroupCompartment => ObjectGroupRealm

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.

Comment 72

[Jan de Mooij [:jandem]]

Attachment: Part 37 - Move unboxedLayouts list to ObjectGroupRealm

This way we can ensure the UnboxedLayout for a group is always in the group's realm's unboxedLayout list.

Comment 73

[Jan de Mooij [:jandem]]

Attachment: Part 38 - RegExpCompartment => RegExpRealm

Comment 74

[Jan de Mooij [:jandem]]

Attachment: Part 39 - Move savedStacks_ to JS::Realm

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.

Comment 75

[Jan de Mooij [:jandem]]

Attachment: Part 40 - Use private inheritance

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.

Comment 76

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7d6335bbd6c3
https://hg.mozilla.org/mozilla-central/rev/84f6e67dcd1c
https://hg.mozilla.org/mozilla-central/rev/72b513e25bec
https://hg.mozilla.org/mozilla-central/rev/def02132a4cf

Comment 77

[Pulsebot]

Pushed by [e-mail]:
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

Comment 78

[Luke Wagner [:luke]]

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.

Comment 79

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/19b350681b94
https://hg.mozilla.org/mozilla-central/rev/a7c669b99bd1
https://hg.mozilla.org/mozilla-central/rev/6a363dbae273
https://hg.mozilla.org/mozilla-central/rev/dff489ff6e4c

Comment 80

[Pulsebot]

Pushed by [e-mail]:
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

Comment 81

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/7fe5ca35982d
https://hg.mozilla.org/mozilla-central/rev/12a249851bed
https://hg.mozilla.org/mozilla-central/rev/b9c8a89ecb9d
https://hg.mozilla.org/mozilla-central/rev/2ce6ce0e6291
https://hg.mozilla.org/mozilla-central/rev/385b54738fb6
https://hg.mozilla.org/mozilla-central/rev/d99b7e4e8cd9