Closed Bug 1237503 Opened 4 years ago Closed 3 years ago
Add support for [Cached] things on DOM proxies
This, of course, requires that the JS engine allow us to have identical slot layouts on proxies and non-proxies.
OK, I just tried to do this, but ran into a problem. Getting a [Cached] value requires preserving the wrapper, so the value won't die. But proxies and non-proxies preserve the wrapper differently: the latter just calls PreserveWrapper, but the former calls EnsureExpandoObject, with comments about how it's the only reliable way to preserve the wrapper... And the actual code in the value getter doesn't know whether the concrete implementation is a proxy or not, so has no way to do the right thing at the moment. I could add dynamic checks for the reflector being a proxy, but that seems pretty annoying. Peter, can you remind me why we have this difference? If I actually do call PreserveWrapper() on a DOM proxy's C++ object, will that somehow fail to preserve the wrapper or something? It's really not clear to me what the story is here.
And in particular, seems to me like DOMXrayTraits::preserveWrapper already assumes that calling PreserveWrapper on its own is enough, or am I missing something?
Depends on: 1189822
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #1) > Peter, can you remind me why we have this difference? If I actually do call > PreserveWrapper() on a DOM proxy's C++ object, will that somehow fail to > preserve the wrapper or something? It's really not clear to me what the > story is here. I think there's some confusion, certainly in the comments in the generated code (you added that and then filed bug 1189822). We've talked about the difference between PreserveWrapper and RegisterDOMExpandoObject before. RegisterDOMExpandoObject was an attempt to simplify preserving a wrapper (triggered by the fact that we wanted to preserve the expandos, which for proxies really means the expando object, not really the wrapper). I added it when we did DOM proxies, and I've mentioned before that at this point I think we should remove it and switch everything to PreserveWrapper. And afaik that should already work as is, I certainly had to deal with PreserveWrapper having been called for proxies in nsWrapperCache::ReleaseWrapper. Now, the comment is sort of right in that if you want to call RegisterDOMExpandoObject you have to go through EnsureExpandoObject. It's just that I don't think RegisterDOMExpandoObject is the only way to preserve proxies.
The only difference between proxies and non-proxies is that proxies only support up to MAX_FIXED_SLOTS slots all told (reserved plus private slot). SpiderMonkey already has static asserts to make sure we don't ask for too many reserved slots on a proxy.
Attachment #8868396 - Flags: review?(kyle)
Attachment #8863462 - Attachment is obsolete: true
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8868396 - Flags: review?(kyle) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d23ef00079 Add support for [Cached] and [StoreInSlot] things on DOM proxies. r=qdot
You need to log in before you can comment on or make changes to this bug.