Closed Bug 1466083 Opened 2 years ago Closed 2 years ago

Fix remaining places relying on single realm per compartment

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(9 files, 1 obsolete file)

15.10 KB, patch
luke
: review+
Details | Diff | Splinter Review
6.33 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.88 KB, patch
jonco
: review+
Details | Diff | Splinter Review
3.74 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.23 KB, patch
jonco
: review+
Details | Diff | Splinter Review
3.97 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.52 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.61 KB, patch
luke
: review+
Details | Diff | Splinter Review
3.28 KB, patch
jonco
: review+
Details | Diff | Splinter Review
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.
This removes the last GetRealmForCompartment from the debugger.
Attachment #8982467 - Flags: review?(luke)
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.
Attachment #8982470 - Flags: review?(luke)
* 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.
Attachment #8982471 - Flags: review?(jcoppeard)
Comment on attachment 8982471 [details] [diff] [review]
Part 3 - Some minor Zone::sweepCompartments cleanup

Actually this isn't quite right..
Attachment #8982471 - Flags: review?(jcoppeard)
Attachment #8982471 - Attachment is obsolete: true
Attachment #8982472 - Flags: review?(jcoppeard)
Attachment #8982472 - Flags: review?(jcoppeard) → review+
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.
Attachment #8982480 - Flags: review?(jcoppeard)
Attachment #8982473 - Flags: review?(jwalden+bmo) → review+
Not strictly necessary, but this lets us remove some JS::GetCompartmentForRealm and JS_GetCompartmentPrincipals calls.
Attachment #8982486 - Flags: review?(bzbarsky)
All callers have a Realm so it's a bit more ergonomic if they can pass it directly instead of relying on GetCompartmentForRealm.
Attachment #8982487 - Flags: review?(luke)
* 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.
Attachment #8982490 - Flags: review?(luke)
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.
Attachment #8982480 - Flags: review?(jcoppeard) → review+
Attachment #8982492 - Flags: review?(jcoppeard) → review+
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
Attachment #8982467 - Flags: review?(luke) → review+
Attachment #8982470 - Flags: review?(luke) → review+
Attachment #8982487 - Flags: review?(luke) → review+
Comment on attachment 8982490 [details] [diff] [review]
Part 8 - Various minor API changes

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

Makes sense
Attachment #8982490 - Flags: review?(luke) → review+
Comment on attachment 8982486 [details] [diff] [review]
Part 6 - Add xpc::GetRealmPrincipal and use it in a few places

r=me
Attachment #8982486 - Flags: review?(bzbarsky) → review+
Pushed by jandemooij@gmail.com:
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
Keywords: leave-open
Pushed by jandemooij@gmail.com:
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
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.