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