778085 - Implement getPrototype proxy trap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Right now, proxies don't function under normal prototype semantics, which makes them difficult to use on the prototype chain. Specifically, if the JS engine encounters a proxy on the prototype chain, it will call the getPropertyDescriptor and then return. If the proxy wants to bounce lookups up the prototype chain, it has to do so manually.

According to Eddy, there's currently some spec discussion about how to properly integrate prototypes with proxies. In the near-term though, here's a proposal for a simple solution:

Add a getPrototype trap to BaseProxyHandler. Make it optional (with a flag or something), and keep the default behavior the same. Then, figure out the places in the JS engine where we call the non-own variants of various proxy traps. If the proxy implements the getPrototype trap, the caller should do an |own| lookup on the object and bounce to the prototype on failure.

This is straightforward for getPropertyDescriptor and getPropertyNames. I can imagine it being tricky for the various derived traps, because DirectProxyHandler forwards those directly to the target object in order to transparently handle things like PropertyOps. I _think_ they're all manageable though with sufficient creativity.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Would be nice for DOM proxies, for sure!

Comment 2

[Bobby Holley (:bholley)]

This blocks my XBL stuff, so I'm working on it.

Comment 3

[Luke Wagner [:luke]]

Check out the 'proto and proxies' section:
http://wiki.ecmascript.org/doku.php?id=harmony:direct_proxies#discussed_during_tc39_july_2012_meeting_microsoft_redmond

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add optional getPrototypeOf trap. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add AutoVectorRooter.append(AutoVectorRooter). v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add prototype suppport in Proxy::foo implementations. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add simple withPrototype versions of DirectWrapper and CrossCompartmentWrapper. v1

These are helpful for testing, and will also be useful in XPConnect. Note that the Wrapper
and Proxy hierarchy will soon be merged.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Add a wrapWithProto function to the shell. v1

This allows us to test this stuff in script.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Tests. v1

Comment 10

[Eddy Bruel [:ejpbruel]]

Comment on attachment 647623 [details] [diff] [review]
Part 1 - Add optional getPrototypeOf trap. v1

Review of attachment 647623 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good in principle, but should getPrototypeOf be optional? The default implementation, BaseProxyHandler::getPrototypeOf, does the same thing as ObjectImpl::getProto does right now (i.e. it returns the prototype of the proxy object). Assuming that you intend to reimplement ObjectImpl::getProto to forward to the getPrototypeOf trap (which is what I would do), making it mandatory does not change any semantics, and does not require any changes in existing code. Making getPrototypeOf the only optional Proxy trap seems unnecessarily complex, unless you had a good reason for it.

r- for now, at least until either one of us convinces the other that he is right :-)

Comment 11

[Eddy Bruel [:ejpbruel]]

Ok, so looking at part 4 it seems that you made getPrototype an optional trap because certain traps, such as BaseProxyHandler::has would never be called if you made getPrototype mandatory. However, to take the has trap as an example, the only reason this is so is that you reimplemented Proxy::has in term of the getPrototypeOf and hasOwn traps. This makes sense, but I feel like this should be more properly done on the has trap itself.

To be more precise, I think Proxy::has should stay as it is, and BaseProxyHandler::has should be reimplemented in terms of getPrototypeOf and hasOwn (ditto for the other derived traps). This would allow getPrototypeOf to become a mandatory trap, something that seems necessary anyway because direct proxies will have both getPrototypeOf and has, as per spec: http://wiki.ecmascript.org/doku.php?id=harmony:direct_proxies.

Comment 12

[Eddy Bruel [:ejpbruel]]

Comment on attachment 647625 [details] [diff] [review]
Part 3 - Add prototype suppport in Proxy::foo implementations. v1

Review of attachment 647625 [details] [diff] [review]:
-----------------------------------------------------------------

r- due to my previous comment. I hope I'm not being overly anal here :-)

Comment 13

[Eddy Bruel [:ejpbruel]]

Since the remaining patches are based on the assumption that getPrototypeOf should be optional, I'll wait with reviewing them until bholley and I have reached a agreement on what approach to take.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't entirely understand what's happening in this bug. However, it would be really helpful to me if the prototype of a cross-compartment wrapper were lazily computed (by wrapping the target's proto on-demand). Hopefully that would allow us to remove the code here:
  http://hg.mozilla.org/mozilla-central/file/030feb4315fc/js/src/jscompartment.cpp#l266
which has been causing lots of problems for me, and which doesn't really make sense given the mutability of __proto__.

Is it possible that this bug could accomplish this? If so, I would be extremely grateful.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #14)
> I don't entirely understand what's happening in this bug. However, it would
> be really helpful to me if the prototype of a cross-compartment wrapper were
> lazily computed (by wrapping the target's proto on-demand). Hopefully that
> would allow us to remove the code here:
>  
> http://hg.mozilla.org/mozilla-central/file/030feb4315fc/js/src/jscompartment.
> cpp#l266
> which has been causing lots of problems for me, and which doesn't really
> make sense given the mutability of __proto__.
> 
> Is it possible that this bug could accomplish this? If so, I would be
> extremely grateful.

This bug as it stands will not do that. But we could maybe get there. I'll be in MV the week of the 20th. We could meet up and talk about it then?

Comment 16

[Eddy Bruel [:ejpbruel]]

Comment on attachment 647623 [details] [diff] [review]
Part 1 - Add optional getPrototypeOf trap. v1

Review of attachment 647623 [details] [diff] [review]:
-----------------------------------------------------------------

Figured out a way with bholley to make these patches work with direct proxies once they get the getPrototypeOf trap. The idea is to add a bitmap that allows a proxy to tell whether it has an explicit override for each non-own trap. An alternative solution would have been to simply have a default implementation for each non-own trap in terms of getPrototypeOf and the corresponding own trap, but if a proxy provides an explicit override in this case, we are already in the compartment of the wrappee (in the case of CrossCompartmentWrapper), so this is problematic. We would have to re-enter the compartment of the wrapper in this case. Either way, the bitmap solution involves the least change to the existing proxy hierarchy, which is why it is preferred.

Now that we have this solution, this patch is ok as it stands.

Comment 17

[Eddy Bruel [:ejpbruel]]

Comment on attachment 647625 [details] [diff] [review]
Part 3 - Add prototype suppport in Proxy::foo implementations. v1

Review of attachment 647625 [details] [diff] [review]:
-----------------------------------------------------------------

I'd just like to say that I *hate* the fact that we use a macro in this patch. That's XPConnect crap. We don't do that in SpiderMonkey :-P

Seriously though, I don't see any other way to avoid unnecessary code duplication here, so I guess I'll have to go along with it ;-)

Comment 18

[Eddy Bruel [:ejpbruel]]

Comment on attachment 647626 [details] [diff] [review]
Part 4 - Add simple withPrototype versions of DirectWrapper and CrossCompartmentWrapper. v1

Review of attachment 647626 [details] [diff] [review]:
-----------------------------------------------------------------

Nothing wrong with this patch :-)

Comment 19

[Bobby Holley (:bholley)]

Thanks Eddy!

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3b864d646268

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 0 - Use the Handle API in a few more places in Proxy. v1

Sigh. Some new handle API stuff landed that broke stuff when I rebased these patches. Let's pass the handles in a few more places.

Comment 21

[Bobby Holley (:bholley)]

Trying again: https://tbpl.mozilla.org/?tree=Try&rev=221ea4d810cc

Comment 22

[Bobby Holley (:bholley)]

Looks green! Pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c02e5467a98
https://hg.mozilla.org/integration/mozilla-inbound/rev/595868a94674
https://hg.mozilla.org/integration/mozilla-inbound/rev/087ae309f7dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/b591dd65cc5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5159ce49551c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a014e9ee1ed5
https://hg.mozilla.org/integration/mozilla-inbound/rev/440c8b75f1ce

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c02e5467a98
https://hg.mozilla.org/mozilla-central/rev/595868a94674
https://hg.mozilla.org/mozilla-central/rev/087ae309f7dd
https://hg.mozilla.org/mozilla-central/rev/b591dd65cc5d
https://hg.mozilla.org/mozilla-central/rev/5159ce49551c
https://hg.mozilla.org/mozilla-central/rev/a014e9ee1ed5
https://hg.mozilla.org/mozilla-central/rev/440c8b75f1ce

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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