If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add support for [Cached] things on DOM proxies

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 affected, firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

This, of course, requires that the JS engine allow us to have identical slot layouts on proxies and non-proxies.
Depends on: 1237504
Blocks: 580070
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.
Flags: needinfo?(peterv)
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
Created attachment 8863462 [details] [diff] [review]
work in progress
Depends on: 1361125
(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.
Flags: needinfo?(peterv)
Created attachment 8868396 [details] [diff] [review]
Add support for [Cached] and [StoreInSlot] things on DOM 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+

Comment 6

4 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d23ef00079
Add support for [Cached] and [StoreInSlot] things on DOM proxies. r=qdot

Comment 7

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7d23ef00079
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.