1237503 - Add support for [Cached] things on DOM proxies

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This, of course, requires that the JS engine allow us to have identical slot layouts on proxies and non-proxies.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

And in particular, seems to me like DOMXrayTraits::preserveWrapper already assumes that calling PreserveWrapper on its own is enough, or am I missing something?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: work in progress

Comment 4

[Peter Van der Beken [:peterv]]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d23ef00079
Add support for [Cached] and [StoreInSlot] things on DOM proxies. r=qdot

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7d23ef00079