Fix remaining places relying on single realm per compartment

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

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

Description

11 months ago
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.
(Assignee)

Comment 1

11 months ago
This removes the last GetRealmForCompartment from the debugger.
Attachment #8982467 - Flags: review?(luke)
(Assignee)

Comment 2

11 months ago
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)
(Assignee)

Comment 3

11 months ago
* 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)
(Assignee)

Comment 4

11 months ago
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)
(Assignee)

Comment 5

11 months ago
Attachment #8982471 - Attachment is obsolete: true
Attachment #8982472 - Flags: review?(jcoppeard)

Updated

11 months ago
Attachment #8982472 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 7

11 months ago
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)

Updated

11 months ago
Attachment #8982473 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 8

11 months ago
Not strictly necessary, but this lets us remove some JS::GetCompartmentForRealm and JS_GetCompartmentPrincipals calls.
Attachment #8982486 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

11 months ago
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)
(Assignee)

Comment 10

11 months ago
* 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+

Updated

11 months ago
Attachment #8982492 - Flags: review?(jcoppeard) → review+

Comment 13

11 months ago
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+

Updated

11 months ago
Attachment #8982470 - Flags: review?(luke) → review+

Updated

11 months ago
Attachment #8982487 - Flags: review?(luke) → review+

Comment 14

11 months ago
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+

Comment 16

11 months ago
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
(Assignee)

Updated

11 months ago
Keywords: leave-open

Comment 18

11 months ago
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
(Assignee)

Updated

11 months ago
Keywords: leave-open

Comment 19

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eff5e370cb33
https://hg.mozilla.org/mozilla-central/rev/bf5be9b21c3c
https://hg.mozilla.org/mozilla-central/rev/55c591df0a06
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.