Closed
Bug 1461938
Opened 6 years ago
Closed 6 years ago
Start using JS::Realm instead of JSCompartment
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(40 files, 2 obsolete files)
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
This gives us cx->realm(). cx->compartment() still works but most of its uses will incrementally disappear.
Attachment #8976094 -
Flags: review?(luke)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8976095 -
Flags: review?(luke)
Assignee | ||
Comment 3•6 years ago
|
||
objects/scripts/groups now all have a realm() method.
Attachment #8976096 -
Flags: review?(luke)
Assignee | ||
Comment 4•6 years ago
|
||
Now we can start moving stuff from JSCompartment to JS::Realm; this patch starts with the RealmOptions.
Attachment #8976097 -
Flags: review?(luke)
Assignee | ||
Comment 5•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8976094 -
Flags: review?(luke) → review+
Comment 6•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8976096 -
Flags: review?(luke) → review+
Updated•6 years ago
|
Attachment #8976097 -
Flags: review?(luke) → review+
Comment 7•6 years ago
|
||
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+
Assignee | ||
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
(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.)
Updated•6 years ago
|
Attachment #8976497 -
Flags: review?(jcoppeard) → review+
Comment 10•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Assignee | ||
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
(Fix a comment typo.)
Attachment #8976653 -
Attachment is obsolete: true
Attachment #8976653 -
Flags: review?(jwalden+bmo)
Attachment #8976655 -
Flags: review?(jwalden+bmo)
Comment 13•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Attachment #8976655 -
Flags: review?(jwalden+bmo) → review+
Comment 14•6 years ago
|
||
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
Assignee | ||
Comment 15•6 years ago
|
||
Attachment #8976990 -
Flags: review?(luke)
Assignee | ||
Comment 16•6 years ago
|
||
Attachment #8976991 -
Flags: review?(luke)
Comment 17•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8976991 -
Flags: review?(luke) → review+
Comment 18•6 years ago
|
||
bugherder |
Assignee | ||
Comment 20•6 years ago
|
||
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)
Assignee | ||
Comment 21•6 years ago
|
||
Attachment #8979257 -
Flags: review?(luke)
Assignee | ||
Comment 22•6 years ago
|
||
Attachment #8979260 -
Flags: review?(luke)
Updated•6 years ago
|
Attachment #8979257 -
Flags: review?(luke) → review+
Assignee | ||
Comment 23•6 years ago
|
||
In theory the dtoa cache could be moved to the Zone, but it doesn't really matter.
Attachment #8979262 -
Flags: review?(andrebargull)
Comment 24•6 years ago
|
||
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+
Assignee | ||
Comment 25•6 years ago
|
||
(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 :)
Assignee | ||
Comment 26•6 years ago
|
||
These are pretty trivial.
Attachment #8979263 -
Flags: review?(andrebargull)
Assignee | ||
Comment 27•6 years ago
|
||
This field could probably also be moved to Zone at some point, but it doesn't matter much.
Attachment #8979270 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 28•6 years ago
|
||
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)
Assignee | ||
Comment 29•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8979256 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8979270 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 30•6 years ago
|
||
Attachment #8979281 -
Flags: review?(luke)
Updated•6 years ago
|
Attachment #8979281 -
Flags: review?(luke) → review+
Comment 31•6 years ago
|
||
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)
Assignee | ||
Comment 32•6 years ago
|
||
(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.
Assignee | ||
Comment 33•6 years ago
|
||
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...
Assignee | ||
Comment 34•6 years ago
|
||
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)
Comment 35•6 years ago
|
||
(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)
Assignee | ||
Comment 36•6 years ago
|
||
This uses UniquePtr both for the maps themselves and their values.
Attachment #8979319 -
Flags: review?(luke)
Assignee | ||
Comment 37•6 years ago
|
||
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 38•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8979262 -
Flags: review?(andrebargull) → review+
Updated•6 years ago
|
Attachment #8979263 -
Flags: review?(andrebargull) → review+
Updated•6 years ago
|
Attachment #8979274 -
Flags: review?(andrebargull) → review+
Comment 39•6 years ago
|
||
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+
Assignee | ||
Comment 40•6 years ago
|
||
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)
Assignee | ||
Comment 41•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8979464 -
Attachment description: Part 21 - Move some isSelfHosting and selfHostingScriptSource → Part 21 - Move isSelfHosting and selfHostingScriptSource
Assignee | ||
Comment 42•6 years ago
|
||
Attachment #8979469 -
Flags: review?(andrebargull)
Assignee | ||
Comment 43•6 years ago
|
||
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)
Comment 44•6 years ago
|
||
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
Assignee | ||
Comment 45•6 years ago
|
||
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)
Assignee | ||
Comment 46•6 years ago
|
||
Attachment #8979563 -
Flags: review?(evilpies)
Assignee | ||
Comment 47•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8979581 -
Attachment description: Part 16 - Move performanceMonitoring to JS::Realm → Part 26 - Move performanceMonitoring to JS::Realm
Updated•6 years ago
|
Attachment #8979444 -
Flags: review?(luke) → review+
Updated•6 years ago
|
Attachment #8979523 -
Flags: review?(luke) → review+
Updated•6 years ago
|
Attachment #8979581 -
Flags: review?(luke) → review+
Assignee | ||
Comment 48•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8979464 -
Flags: review?(evilpies) → review+
Updated•6 years ago
|
Attachment #8979551 -
Flags: review?(evilpies) → review+
Updated•6 years ago
|
Attachment #8979563 -
Flags: review?(evilpies) → review+
Updated•6 years ago
|
Attachment #8979469 -
Flags: review?(andrebargull) → review+
Comment 49•6 years ago
|
||
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+
Comment 50•6 years ago
|
||
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
Assignee | ||
Comment 51•6 years ago
|
||
(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.
Assignee | ||
Comment 52•6 years ago
|
||
Attachment #8979871 -
Flags: review?(luke)
Assignee | ||
Comment 53•6 years ago
|
||
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)
Assignee | ||
Comment 54•6 years ago
|
||
Attachment #8979874 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 55•6 years ago
|
||
See patch for why this moves to the zone.
Attachment #8979875 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 56•6 years ago
|
||
Attachment #8979876 -
Flags: review?(luke)
Updated•6 years ago
|
Attachment #8979874 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 57•6 years ago
|
||
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)
Assignee | ||
Comment 58•6 years ago
|
||
At this point there are a handful of stragglers left, so probably 5 more patches or so for this bug...
Comment 59•6 years ago
|
||
bugherder |
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•6 years ago
|
||
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 61•6 years ago
|
||
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+
Assignee | ||
Comment 62•6 years ago
|
||
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.
Assignee | ||
Comment 63•6 years ago
|
||
Well nevermind; I'll just use ni->obj in RegisterEnumerator and then in sweepNativeIterators assert same-realm, that should be sufficient.
Comment 64•6 years ago
|
||
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
Updated•6 years ago
|
Attachment #8979871 -
Flags: review?(luke) → review+
Updated•6 years ago
|
Attachment #8979876 -
Flags: review?(luke) → review+
Comment 65•6 years ago
|
||
(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.
Assignee | ||
Comment 66•6 years ago
|
||
(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•6 years ago
|
||
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
Updated•6 years ago
|
Attachment #8979875 -
Flags: review?(jwalden+bmo) → review+
Comment 68•6 years ago
|
||
bugherder |
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
Assignee | ||
Comment 69•6 years ago
|
||
Attachment #8980208 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 70•6 years ago
|
||
Attachment #8980209 -
Flags: review?(luke)
Assignee | ||
Comment 71•6 years ago
|
||
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)
Assignee | ||
Comment 72•6 years ago
|
||
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)
Assignee | ||
Comment 73•6 years ago
|
||
Attachment #8980212 -
Flags: review?(luke)
Assignee | ||
Comment 74•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8980208 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8980211 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 75•6 years ago
|
||
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)
Comment 76•6 years ago
|
||
bugherder |
Comment 77•6 years ago
|
||
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
Updated•6 years ago
|
Attachment #8980209 -
Flags: review?(luke) → review+
Comment 78•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8980212 -
Flags: review?(luke) → review+
Updated•6 years ago
|
Attachment #8980213 -
Flags: review?(luke) → review+
Comment 79•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Attachment #8980221 -
Flags: review?(evilpies) → review+
Comment 80•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 81•6 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•