Last Comment Bug 778085 - Implement getPrototype proxy trap
: Implement getPrototype proxy trap
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 842390 842940 851673
Blocks: 778086
  Show dependency treegraph
 
Reported: 2012-07-27 03:07 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2013-03-15 17:09 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Add optional getPrototypeOf trap. v1 (3.62 KB, patch)
2012-07-31 11:49 PDT, Bobby Holley (:bholley) (busy with Stylo)
ejpbruel: review+
Details | Diff | Splinter Review
Part 2 - Add AutoVectorRooter.append(AutoVectorRooter). v1 (916 bytes, patch)
2012-07-31 11:49 PDT, Bobby Holley (:bholley) (busy with Stylo)
luke: review+
Details | Diff | Splinter Review
Part 3 - Add prototype suppport in Proxy::foo implementations. v1 (8.79 KB, patch)
2012-07-31 11:49 PDT, Bobby Holley (:bholley) (busy with Stylo)
ejpbruel: review+
Details | Diff | Splinter Review
Part 4 - Add simple withPrototype versions of DirectWrapper and CrossCompartmentWrapper. v1 (7.58 KB, patch)
2012-07-31 11:50 PDT, Bobby Holley (:bholley) (busy with Stylo)
ejpbruel: review+
Details | Diff | Splinter Review
Part 5 - Add a wrapWithProto function to the shell. v1 (2.12 KB, patch)
2012-07-31 11:51 PDT, Bobby Holley (:bholley) (busy with Stylo)
ejpbruel: review+
Details | Diff | Splinter Review
Part 6 - Tests. v1 (3.43 KB, patch)
2012-07-31 11:52 PDT, Bobby Holley (:bholley) (busy with Stylo)
ejpbruel: review+
Details | Diff | Splinter Review
Part 0 - Use the Handle API in a few more places in Proxy. v1 (9.62 KB, patch)
2012-08-10 01:00 PDT, Bobby Holley (:bholley) (busy with Stylo)
efaustbmo: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-07-27 03:07:57 PDT
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 [:bz] 2012-07-27 09:24:45 PDT
Would be nice for DOM proxies, for sure!
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-07-30 04:46:54 PDT
This blocks my XBL stuff, so I'm working on it.
Comment 3 Luke Wagner [:luke] 2012-07-30 09:59:40 PDT
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) (busy with Stylo) 2012-07-31 11:49:14 PDT
Created attachment 647623 [details] [diff] [review]
Part 1 - Add optional getPrototypeOf trap. v1
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-07-31 11:49:30 PDT
Created attachment 647624 [details] [diff] [review]
Part 2 - Add AutoVectorRooter.append(AutoVectorRooter). v1
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-07-31 11:49:48 PDT
Created attachment 647625 [details] [diff] [review]
Part 3 - Add prototype suppport in Proxy::foo implementations. v1
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-07-31 11:50:59 PDT
Created attachment 647626 [details] [diff] [review]
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) (busy with Stylo) 2012-07-31 11:51:19 PDT
Created attachment 647627 [details] [diff] [review]
Part 5 - Add a wrapWithProto function to the shell. v1

This allows us to test this stuff in script.
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-07-31 11:52:03 PDT
Created attachment 647628 [details] [diff] [review]
Part 6 - Tests. v1
Comment 10 Eddy Bruel [:ejpbruel] 2012-08-02 08:19:15 PDT
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] 2012-08-02 08:54:17 PDT
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] 2012-08-02 08:56:23 PDT
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] 2012-08-02 08:58:18 PDT
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 (:billm) 2012-08-02 18:28:30 PDT
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) (busy with Stylo) 2012-08-03 07:00:17 PDT
(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] 2012-08-09 13:16:50 PDT
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] 2012-08-09 13:19:25 PDT
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] 2012-08-09 13:21:26 PDT
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) (busy with Stylo) 2012-08-09 13:38:26 PDT
Thanks Eddy!

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3b864d646268
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2012-08-10 01:00:35 PDT
Created attachment 650817 [details] [diff] [review]
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) (busy with Stylo) 2012-08-10 01:16:28 PDT
Trying again: https://tbpl.mozilla.org/?tree=Try&rev=221ea4d810cc
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-01-31 13:33:35 PST
https://hg.mozilla.org/mozilla-central/rev/f59f66def525

Note You need to log in before you can comment on or make changes to this bug.