Closed
Bug 1466083
Opened 6 years ago
Closed 6 years ago
Fix remaining places relying on single realm per compartment
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(9 files, 1 obsolete file)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 1•6 years ago
|
||
This removes the last GetRealmForCompartment from the debugger.
Attachment #8982467 -
Flags: review?(luke)
Assignee | ||
Comment 2•6 years 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•6 years 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•6 years 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•6 years ago
|
||
Attachment #8982471 -
Attachment is obsolete: true
Attachment #8982472 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8982473 -
Flags: review?(jwalden+bmo)
Updated•6 years ago
|
Attachment #8982472 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 7•6 years 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•6 years ago
|
Attachment #8982473 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Not strictly necessary, but this lets us remove some JS::GetCompartmentForRealm and JS_GetCompartmentPrincipals calls.
Attachment #8982486 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•6 years 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•6 years 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)
Assignee | ||
Comment 11•6 years ago
|
||
The last one, for this bug.
Attachment #8982492 -
Flags: review?(jcoppeard)
Comment 12•6 years ago
|
||
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•6 years ago
|
Attachment #8982492 -
Flags: review?(jcoppeard) → review+
Comment 13•6 years 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•6 years ago
|
Attachment #8982470 -
Flags: review?(luke) → review+
Updated•6 years ago
|
Attachment #8982487 -
Flags: review?(luke) → review+
Comment 14•6 years 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 15•6 years ago
|
||
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•6 years 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•6 years ago
|
Keywords: leave-open
Comment 17•6 years ago
|
||
bugherder |
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•6 years 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•6 years ago
|
Keywords: leave-open
Comment 19•6 years 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
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•