Closed
Bug 1469082
Opened 6 years ago
Closed 6 years ago
Consider not having obj->group->realm_ for CCWs
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file)
Once bug 1466112 is finished, we'll have asserts in place to check ~nobody asks for the realm/global of a CCW (because it's meaningless since CCWs are shared by all realms). However, internally the wrapper's ObjectGroup will still have a realm pointer and GC will trace group->realm->global so a potential risk is "leaking" globals that would have been dead if not for some CCW. We can't just set group->realm to nullptr for CCW ObjectGroups, because then we also have no way to determine obj->compartment(). Some options: (1) Make group->realm a RealmOrCompartment tagged pointer (JS::Realm* for most objects, JS::Compartment* for CCWs). This will require an extra branch on the obj->compartment() path but that's probably okay.. (2) Keep group->realm, but don't trace it for CCW groups. This makes it effectively a weak pointer - when we destroy a realm, we then have to go through all the wrappers in the compartment's wrapper map and change their group->realm to point at a realm in that compartment that's still alive. This gets complicated when all realms in the compartment are dead except for a CCW root. (3) Have a dedicated wrapper realm. The first one is probably the simplest to implement and to reason about. Not high priority, but we should fix this at some point.
Assignee | ||
Comment 1•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #0) > (1) Make group->realm a RealmOrCompartment tagged pointer (JS::Realm* for > most objects, JS::Compartment* for CCWs). This will require an extra branch > on the obj->compartment() path but that's probably okay.. ...and this could be optimized when we know |obj| is definitely not a CCW.
Assignee | ||
Comment 2•6 years ago
|
||
I've been thinking a bit about this lately. We should probably just allocate all CCWs in compartment->realms[0] for now. That way, worst-case that realm we keep alive a bit longer than necessary (when it only contains wrappers being used by other realms), but it's just a single realm and often the first realm will be the "main" realm anyway. I'll write a patch for this soon.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•6 years ago
|
||
CCWs are not really associated with a single realm and we assert code doesn't enter/request the realm or global of a CCW. However they currently still have an ObjectGroup that's associated with a single realm. Using the same realm avoids some potential memory usage issues.
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b67b31f6a81b Always allocate CCWs in the compartment's first realm. r=jonco
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b67b31f6a81b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•