Jan de Mooij [:jandem] Assignee
	Description • 6 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 1 • 6 years ago

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

	Pulsebot
	Comment 2 • 6 years ago

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

	Cosmin Sabou [:CosminS]
	Comment 3 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bf36035bed13

	Jan de Mooij [:jandem] Assignee
	Comment 4 • 6 years ago

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

	Jan de Mooij [:jandem] Assignee
	Comment 5 • 6 years ago

oldCompartment is never set so the LeaveRealm call in JSAPITest::uninit is unreachable.

	André Bargull [:anba]
	Comment 6 • 6 years ago

Comment on attachment 8981120 [details] [diff] [review]
Part 3 - Remove GetRealmForCompartment calls from jsapi-tests

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

Easy!

	Jan de Mooij [:jandem] Assignee
	Comment 7 • 6 years ago

This one is kind of nice because it removes 6 calls to GetRealmForCompartment, and there aren't *that* many left at this point.

	Jan de Mooij [:jandem] Assignee
	Comment 8 • 6 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 9 • 6 years ago

Also an easy one. This has a drive-by fix for SavedStacks::insertFrames to use iter.realm() instead of iter.compartment().

	Jan de Mooij [:jandem] Assignee
	Comment 11 • 6 years ago

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

	Tom S [:evilpie]
	Comment 12 • 6 years ago

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++?

	Jan de Mooij [:jandem] Assignee
	Comment 13 • 6 years ago

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

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 14 • 6 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 15 • 6 years ago

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.

	Pulsebot
	Comment 16 • 6 years ago

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

	Pulsebot
	Comment 17 • 6 years ago

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

	Arthur Iakab [arthur_iakab]
	Comment 18 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8de4be88b8ca
https://hg.mozilla.org/mozilla-central/rev/bdadfc22d3fe
https://hg.mozilla.org/mozilla-central/rev/8af7dd4fb5e2

	Pulsebot
	Comment 19 • 6 years ago

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

	Jan de Mooij [:jandem] Assignee
	Comment 20 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 21 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/51748a8a2967
https://hg.mozilla.org/mozilla-central/rev/e73059705526
https://hg.mozilla.org/mozilla-central/rev/24a0788bae5f
https://hg.mozilla.org/mozilla-central/rev/16106d1e0abd

	Noemi Erli[:noemi_erli]
	Comment 22 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a42a47e58247