1466083 - Fix remaining places relying on single realm per compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

Before we can remove the JSCompartment/Realm inheritance and make them completely separate classes, there are still various things we need to fix up. With these fixed, the patch splitting them up will be pretty small.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Make IterateScripts take a realm instead of a compartment

This removes the last GetRealmForCompartment from the debugger.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Replace JSRuntime::numCompartments with JSRuntime::numRealms

It's sort of funny that JSRuntime::numCompartment has two uses and one of them wants numCompartments and the other wants numRealms.

However, in RemapAllWrappersForObject it seems unnecessary to |reserve(rt->numCompartments)| so I just changed that to use append instead of infallibleAppend. The Vector has space for 8 inline entries and that should be sufficient for most objects anyway.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Some minor Zone::sweepCompartments cleanup

* If destroyingRuntime is true, keepAtleastOne is always false so we don't need to check both.

* It's a bit simpler to set keepAtleastOne to false when we find a live compartment, instead of using a separate foundOne bool.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8982471 [details] [diff] [review]
Part 3 - Some minor Zone::sweepCompartments cleanup

Actually this isn't quite right..

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Some minor Zone::sweepCompartments cleanup

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Use UniquePtr instead of ScopedJSDeletePtr when allocating Zones and Realms

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Assume we have a single compartment/realm in Zone::deleteEmptyCompartment

Handling the generic > 1 compartment with > 1 realm case is a bit annoying when we have separate compartment/realm allocations. The caller (mergeRealms) should always have a zone with 1 realm so it's easier to assert that.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Add xpc::GetRealmPrincipal and use it in a few places

Not strictly necessary, but this lets us remove some JS::GetCompartmentForRealm and JS_GetCompartmentPrincipals calls.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Replace GetCompartmentZone with GetRealmZone

All callers have a Realm so it's a bit more ergonomic if they can pass it directly instead of relying on GetCompartmentForRealm.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Various minor API changes

* GetScriptCompartment => GetScriptRealm

* Adds IsSystemRealm in addition to IsSystemCompartment and uses it where we can.

* JS_GetCompartmentPrincipals and IsSystemCompartment now release-assert they have a single realm. This is temporary until we know what Gecko will do/need exactly.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Introduce JS::IterateRealmsInCompartment and use it in NukeAllWrappersForCompartment

The last one, for this bug.

Comment 12

[Jon Coppeard (:jonco)]

Comment on attachment 8982480 [details] [diff] [review]
Part 5 - Assume we have a single compartment/realm in Zone::deleteEmptyCompartment

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

Agreed.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8982467 [details] [diff] [review]
Part 1 - Make IterateScripts take a realm instead of a compartment

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

Nice

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 8982490 [details] [diff] [review]
Part 8 - Various minor API changes

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

Makes sense

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8982486 [details] [diff] [review]
Part 6 - Add xpc::GetRealmPrincipal and use it in a few places

r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/497592872516
part 1 - Make IterateScripts take a realm instead of a compartment. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/480eb5a4c02e
part 2 - Replace JSRuntime::numCompartments with JSRuntime::numRealms. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b5ecb14d55
part 3 - Some minor Zone::sweepCompartments cleanup. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/aabf0f4dc613
part 4 - Use UniquePtr instead of ScopedJSDeletePtr when allocating Zones and Realms. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/266765d448e3
part 5 - Assume we have a single compartment/realm in Zone::deleteEmptyCompartment. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/db700985e1be
part 6 - Add xpc::GetRealmPrincipal and use it in a few places. r=bz

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/497592872516
https://hg.mozilla.org/mozilla-central/rev/480eb5a4c02e
https://hg.mozilla.org/mozilla-central/rev/f9b5ecb14d55
https://hg.mozilla.org/mozilla-central/rev/aabf0f4dc613
https://hg.mozilla.org/mozilla-central/rev/266765d448e3
https://hg.mozilla.org/mozilla-central/rev/db700985e1be

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eff5e370cb33
part 7 - Replace GetCompartmentZone with GetRealmZone. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5be9b21c3c
part 8 - Various minor API changes. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c591df0a06
part 9 - Introduce JS::IterateRealmsInCompartment and use it in NukeAllWrappersForCompartment. r=jonco

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/eff5e370cb33
https://hg.mozilla.org/mozilla-central/rev/bf5be9b21c3c
https://hg.mozilla.org/mozilla-central/rev/55c591df0a06