Use Realm more instead of JSCompartment

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
5 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(8 attachments)

Bug 1461938 moved most of the fields to JS::Realm, but we still have a bunch of places where we do unnecessary conversions or assume a compartment has a single realm. Many of these are easy to fix up so let's do that now.
Most of these changes are pretty simple. NewCompartment => NewRealm, MergeCompartments => MergeRealms, we now have FrameIter::realm(), etc.

This gets rid of a number of GetCompartmentForRealm and GetRealmForCompartment calls and things are starting to look more natural :)
Attachment #8980368 - Flags: review?(luke)
Attachment #8980368 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf36035bed13
part 1 - Fix various places to use Realm instead of JSCompartment. r=luke
Keywords: leave-open
This adds a Vector of Realms to JSCompartment and a RealmsInCompartmentIter iterator class that's very similar to CompartmentsInZoneIter (I considered having them share code but it's probably not worth it).
Attachment #8981119 - Flags: review?(jcoppeard)
oldCompartment is never set so the LeaveRealm call in JSAPITest::uninit is unreachable.
Attachment #8981120 - Flags: review?(andrebargull)
Comment on attachment 8981120 [details] [diff] [review]
Part 3 - Remove GetRealmForCompartment calls from jsapi-tests

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

Easy!
Attachment #8981120 - Flags: review?(andrebargull) → review+
This one is kind of nice because it removes 6 calls to GetRealmForCompartment, and there aren't *that* many left at this point.
Attachment #8981124 - Flags: review?(evilpies)
RealmsIterT still uses GetRealmForCompartment. To fix this, the patch removes RealmsIterT and turns CompartmentsIterT into CompartmentsOrRealmsIterT with an InnerIterT template parameter that's either CompartmentsInZoneIter or RealmsInZoneIter.
Attachment #8981144 - Flags: review?(jcoppeard)
Also an easy one. This has a drive-by fix for SavedStacks::insertFrames to use iter.realm() instead of iter.compartment().
Attachment #8981148 - Flags: review?(andrebargull)
Attachment #8981148 - Flags: review?(andrebargull) → review+
Attachment #8981153 - Flags: review?(luke) → review+
For the most part pretty straight-forward.

One issue is that some of the Realm APIs we want to call take a Handle<Realm*> so I changed IterateRealmCallback to take a Handle<Realm*> instead of a plain Realm*.
Attachment #8981187 - Flags: review?(jcoppeard)
Comment on attachment 8981124 [details] [diff] [review]
Part 4 - Rename CompileCompartment to CompileRealm

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

::: js/src/jit/CompileWrappers.cpp
@@ +235,2 @@
>  {
> +    return reinterpret_cast<JS::Realm*>(this);

Is this even safe C++?
Attachment #8981124 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #12)
> > +    return reinterpret_cast<JS::Realm*>(this);
> 
> Is this even safe C++?

I was wondering about that too... It would be nicer to use private inheritance with "using Base::bar;" to expose the white-listed methods, but that's a pre-existing issue for another day :)
Comment on attachment 8981119 [details] [diff] [review]
Part 2 - Add a Vector of Realms to JSCompartment and add RealmsInCompartmentIter

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

::: js/src/gc/GC.cpp
@@ +7966,5 @@
> +    JSCompartment* comp = realm->compartment();
> +    if (!comp->realms().append(realm)) {
> +        ReportOutOfMemory(cx);
> +        return nullptr;
> +    }

This confused me until I realised that realms are still 1:1 with compartments at this point.
Attachment #8981119 - Flags: review?(jcoppeard) → review+
Comment on attachment 8981144 [details] [diff] [review]
Part 5 -  Combine CompartmentsIterT and RealmsIterT in a single template class

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

Nice.  I sometimes wonder if we should have a library for combining iterators or whether that would be overkill.
Attachment #8981144 - Flags: review?(jcoppeard) → review+
Attachment #8981187 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de4be88b8ca
part 2 - Add a Vector of Realms to JSCompartment and add RealmsInCompartmentIter. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdadfc22d3fe
part 3 - Remove GetRealmForCompartment calls from jsapi-tests. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/8af7dd4fb5e2
part 4 - Rename CompileCompartment to CompileRealm. r=evilpie
Priority: -- → P2
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51748a8a2967
part 5 - Combine CompartmentsIterT and RealmsIterT in a single template class. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73059705526
part 6 - Replace AbstractFramePtr::compartment with AbstractFramePtr::realm. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/24a0788bae5f
part 7 - Replace GetAnyCompartmentInZone with GetAnyRealmInZone. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/16106d1e0abd
part 8 - Make IterateHeapUnbarriered and related code use realms instead of compartments. r=jonco
Keywords: leave-open
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a42a47e58247
part 1 - Fix various places to use Realm instead of JSCompartment. r=luke
(In reply to Pulsebot from comment #19)
> Pushed by jandemooij@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a42a47e58247
> part 1 - Fix various places to use Realm instead of JSCompartment. r=luke

Oops, this belonged to bug 1464374 :/
You need to log in before you can comment on or make changes to this bug.