Closed Bug 703537 Opened 13 years ago Closed 12 years ago

Implement Harmony direct proxies

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: bruant.d, Assigned: ejpbruel)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(26 files, 77 obsolete files)

41.99 KB, patch
Details | Diff | Splinter Review
30.10 KB, patch
bholley
: review+
ejpbruel
: checkin+
Details | Diff | Splinter Review
5.48 KB, patch
bholley
: review+
Details | Diff | Splinter Review
22.23 KB, patch
bholley
: review+
Details | Diff | Splinter Review
27.28 KB, patch
bholley
: review+
Details | Diff | Splinter Review
10.56 KB, patch
bholley
: review+
Details | Diff | Splinter Review
17.33 KB, patch
bholley
: review+
Details | Diff | Splinter Review
28.13 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
15.28 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.65 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.22 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
12.71 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
13.85 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.42 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
10.41 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.51 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.00 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.57 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.41 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.20 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.40 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.98 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
3.99 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.73 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.42 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
9.29 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
During the last TC-39 meeting (https://mail.mozilla.org/pipermail/es-discuss/2011-November/018610.html), the direct proxy proposal (http://wiki.ecmascript.org/doku.php?id=strawman:direct_proxies) has been accepted to replace proxies as they were previously defined (Proxy.create, Proxy.createFunction).

In a nutshell, Proxy.for(target, handler) creates a proxy which calls the handler traps when an operation is performed on it and forward the operation on the target if there is no corresponding traps.
There are a bunch of details in the direct proxies strawman including invariants enforcement, special cases for the construct trap, etc.
OS: Linux → All
Hardware: x86 → All
API change.
A direct proxy is created via:
var p = Proxy(target, handler);
or:
var p = new Proxy(target, handler);
Same semantic in both cases.
Summary: Implement Proxy.for → Implement Harmony direct proxies
Blocks: 706041
I'd like to take this bug, since we're having a couple of bugs with a dependency on this one. Note that I can only work on it occasionally, though, since it is not my main priority. If somebody with more time on his hands wants to take over at some point, feel free to contact me.
Blocks: 687775, 635719
No longer blocks: 706041
No longer blocks: 687775
The draft specification is already fairly complete:
http://wiki.ecmascript.org/doku.php?id=harmony:proxies_spec
Blocks: es6
Important note: the implementation for this bug should include tests to demonstrate that Bug 706041 has been adequately resolved.

Dave
Understood.

Please be aware that I'll be in Paris next week to learn more about the security wrappers from Blake. After that, I'm attending the workweek of the Jetpack team, so there might be a lack of updates on this bug the upcoming 2 weeks. Even so, it's definitely on my to do list, so if this becomes a blocker for anyone, please drop me a line.
Eddy, is this still on your to do list?
(In reply to Jason Orendorff [:jorendorff] from comment #7)
> Eddy, is this still on your to do list?

Hi Jason. Yes it is. Unfortunately, my father passed away right after I got back from the work week, which is why you didn't hear from me for a while. I'm back to work again now though.
I really don't like the fact that a handler is essentially two things in the current proxy implementation.

On the one hand, we have ProxyHandler, which is the abstract base class for all proxy handlers, including ScriptedProxyHandler, and which is stored in JSSLOT_PROXY_HANDLER.

On the other hand, there is the JSObject to which ScriptedProxyHandler forwards all traps, This object is named the handler *object* in some contexts and the proxy private in some others (for instance, the handler object is stored in JSSLOT_PROXY_PRIVATE, can be accessed via GetProxyPrivate, but is named handler in NewProxyObject, and can also be accessed via GetProxyHandlerObject). This is all very confusing.

In any case, I would like to get rid of the name 'private', since it is essentially a catch all name with no specific meaning, and makes the code harder to understand. Since JSSLOT_PROXY_HANDLER is already taken, we could rename it to JSSLOT_PROXY_HANDLER_OBJECT, and add another slot JSSLOT_PROXY_TARGET_OBJECT to store the target object, needed for direct proxies.

Another option is to come up with a better name for ProxyHandler. In that case we could rename JSSLOT_PROXY_PRIVATE to JSSLOT_PROXY_HANDLER, and store the target in JSSLOT_PROXY_TARGET. I actually prefer this solution, but I can't come up with any better name. Does anybody have any suggestions?
Another question. Do we actually use outer window proxies anywhere? Outer window proxies are special in that they allow the handler to be accessed as the inner object via proxy_innerObject. I'm trying to understand in what scenarios this would be necessary, because I have this hunch that for direct proxies, the target, not the handler, might be a more suitable candidate for the inner object (could be dead wrong here though)
This patch implements the proposed naming change.

I've looked at the code in jswrapper.cpp, which uses the handler value as the object being wrapped. Now that we are switching to direct proxies, the target value seems to be a more obvious candidate for this. Unless I'm mistaken, direct proxies allow for the handler value to be null, but not the target value.
Attachment #605394 - Attachment is patch: true
Assignee: general → ejpbruel
(In reply to Eddy Bruel [:ejpbruel] from comment #9)
> I really don't like the fact that a handler is essentially two things in the
> current proxy implementation.

I think the existing code is kind of ugly and insufficiently documented, so let's take a closer look before we rename anything...

The C++ ProxyHandler class is more fundamental. All proxies have a ProxyHandler. The ProxyHandler implements the proxy's custom behavior. This has to be something we can implement in C++, because the *main* purpose of proxies for us is to implement all sorts of objects with magic behavior, *not* only or primarily ES direct proxies, but also wrappers and DOM objects.

So there are several different ProxyHandler subclasses, each one with different custom behavior. Each ProxyHandler may use the JSSLOT_PROXY_PRIVATE slot of the proxy object for whatever it wants.

Scripted proxies are just one kind of proxy (implemented by ScriptedProxyHandler in jsproxy.cpp); they use the JSSLOT_PROXY_PRIVATE slot to store the JS proxy handler object.

Wrappers are a different kind of proxy; they use the JSSLOT_PROXY_PRIVATE slot to store the wrapped object. No scripted proxies are also wrappers. (Note that for some wrappers, the wrapped object is in a different compartment, so the GC has to be aware of this.)

I hope this explains a little better why JSSLOT_PROXY_PRIVATE is named the way it is. Obviously the name does not adequately convey what's going on, so I'm open to renaming it -- but the proposed name "JSSLOT_PROXY_HANDLER" would only be correct for scripted proxies; it would be totally misleading for wrappers.
(In reply to Jason Orendorff [:jorendorff] from comment #12)
> (In reply to Eddy Bruel [:ejpbruel] from comment #9)
> > I really don't like the fact that a handler is essentially two things in the
> > current proxy implementation.
> 
> I think the existing code is kind of ugly and insufficiently documented, so
> let's take a closer look before we rename anything...
> 
> The C++ ProxyHandler class is more fundamental. All proxies have a
> ProxyHandler. The ProxyHandler implements the proxy's custom behavior. This
> has to be something we can implement in C++, because the *main* purpose of
> proxies for us is to implement all sorts of objects with magic behavior,
> *not* only or primarily ES direct proxies, but also wrappers and DOM objects.
> 
> So there are several different ProxyHandler subclasses, each one with
> different custom behavior. Each ProxyHandler may use the
> JSSLOT_PROXY_PRIVATE slot of the proxy object for whatever it wants.
> 
> Scripted proxies are just one kind of proxy (implemented by
> ScriptedProxyHandler in jsproxy.cpp); they use the JSSLOT_PROXY_PRIVATE slot
> to store the JS proxy handler object.
> 
> Wrappers are a different kind of proxy; they use the JSSLOT_PROXY_PRIVATE
> slot to store the wrapped object. No scripted proxies are also wrappers.
> (Note that for some wrappers, the wrapped object is in a different
> compartment, so the GC has to be aware of this.)
> 
> I hope this explains a little better why JSSLOT_PROXY_PRIVATE is named the
> way it is. Obviously the name does not adequately convey what's going on, so
> I'm open to renaming it -- but the proposed name "JSSLOT_PROXY_HANDLER"
> would only be correct for scripted proxies; it would be totally misleading
> for wrappers.

Fair enough, but in my last comment I suggested that with direct proxies, it makes more sense to use JSSLOT_PROXY_TARGET for the object being proxied to, since that's what it is. So if we had a naming scheme like this:

JSSLOT_PROXY_HANDLER
JSSLOT_PROXY_TARGET_VALUE
JSSLOT_PROXY_HANDLER_VALUE

Where JSSLOT_PROXY_TARGET_VALUE is where each different wrapper stores the object it wraps on. This makes a lot more sense to me than JSSLOT_PROXY_PRIVATE, since the 'object being proxied to' is what the target *is*, conceptually
(In reply to Eddy Bruel [:ejpbruel] from comment #13)
> JSSLOT_PROXY_HANDLER
> JSSLOT_PROXY_TARGET_VALUE
> JSSLOT_PROXY_HANDLER_VALUE
> 
> Where JSSLOT_PROXY_TARGET_VALUE is where each different wrapper stores the
> object it wraps on. This makes a lot more sense to me than
> JSSLOT_PROXY_PRIVATE, since the 'object being proxied to' is what the target
> *is*, conceptually

That sounds good, as long as no other ProxyHandler classes are using that slot for yet another purpose.

Good comments will help too, particularly on JSSLOT_PROXY_TARGET_VALUE and JSSLOT_PROXY_HANDLER_VALUE noting that the details of how those slots are populated depend entirely on the ProxyHandler subclass.
This patch adds the constructor from the draft spec. Unlike the old spec, where you had to explicitly state whether you wanted to create an object or a function proxy, this constructor bases that decision on the type of the target value.

Note that js::NewProxyObject expects explicit values for the call and construct slot if the proxy to be created is a function proxy. js_Proxy doesn't have these values, so it cannot create callable function proxies right now.

However, with the new proposal, the call and construct slots have become obsolete anyway. The obvious next step is to remove these slots and reimplement the call and construct traps in terms of the target value, instead.
This patch removes the call and construct slots, as suggested in the previous comment. I've also removed the code that implements the fix trap, since it will be replaced anyway, and I didn't want to waste time making it work in the absence of these slots.

Consequently, I've also removed all the code associated with callable objects, which is what fix creates in the case of function objects. This no longer makes sense, since callable objects assume the presence of the call and/or construct slot. All this functionality will be replaced by the freeze/seal/etc. traps in the direct proxy spec, which have access to the target object, making callable object obsolete.

Unfortunately, but not surprisingly, this patch breaks a couple of dozen tests. Most, if not all, of these are associated with testing proxies, so before moving on, I will either make these tests work again, or disable them if they no longer makes sense (in which case I will replace them with other tests later).

After fixing up the tests, I will move forward by removing some of the utility functions in jsproxy.cpp associated with traps (in particular, the notion of fundamental vs derived traps doesn't exist anymore in the new proposal, since the availability of the target implies that all traps are now optional).
This patch makes all the tests work again. I had to remove a couple of tests related to constructor calls and the fix trap, because these no longer made sense. I will replace them with equivalent tests for the direct proxies later on. All the other tests were simply a matter of using the new Proxy constructor syntax.

I also removed a bug in js::NewProxyObject. This function determines the class of the proxy object based on the values of the call and/or construct arguments. However, these values are no longer used. Instead, the class of the proxy should be based on whether the target object is callable or not (see patch for details)
Attachment #607294 - Attachment is patch: true
I've removed some of the utility functions related to traps. These are no longer necessary, as with direct proxies there is no longer any distinction between fundamental and derived traps (recall that fundamental traps are traps that did not have any default implementation).

More importantly, I've added default implementations for the previously fundamental traps, that forward their operations to the target object. I ran into some trouble here, as the traps we use internally for JSObjects do not correspond one-on-one with the traps we use for proxies.

In particular, whereas I could forward ProxyHandler::getOwnPropertyDescriptor in jsproxy.cpp to GetOwnPropertyDescriptor in jsobj.cpp, there is no such equivalent for ProxyHandler::getPropertyDescriptor (there is no GetPropertyDescriptor in jsobj.cpp). Note that this trap is not included in the direct proxy spec, but it cannot be removed until we've refactored the way proto climbing works.

Until we do, we need some default implementation for ProxyHandler::getPropertyDescriptor that, conceptually at least, works as if GetPropertyDescriptor exists, and we've forwarded the operation to there. I've come up with an implementation, but I'm less than confident that it is correct. I'd appreciate it if somebody could review it (we probably need to change the way getProto() works for proxies. The spec states that getProto() should return the prototype of the target, but I'd like to talk to dherman first if that actually makes sense)

Less importantly, there is also a mismatch between DefineProperty and ProxyHandler::defineProperty. The former uses PropDesc, whereas the latter uses PropertyDescriptor. Note that DefineProperty indirectly calls ProxyHandler::defineProperty if the object is a proxy, passing the value stored in d.pd (where d is an instance of PropDesc), which gets converted to a PropertyDescriptor in ParsePropertyDescriptor (in jsproxy.cpp).

ProxyHandler::defineProperty then in turns calls DefineProperty on its target, passing an instance PropDesc that is initialized from the PropertyDescriptor. Afaict, we don't lose any information this way (although waldo suggested that I might be wrong). It would be nice if someone could double check this as well.

Having said that, all this converting back and forth is very ugly and probably unnecessary, but fixing this is probably more appropriate as part of a larger refactor that moves our internal traps closer to matching the ones in the spec. I don't want to do that right now, as my goal is to land the direct proxies asap, so I'm going to try how close I can stay to the spec without refactoring the existing traps. Hopefully, the end result will be good enough to land.

Once somebody reviews my results so far, my next step will be to add the assertions mentioned in the spec to the traps in ScriptedProxyHandler (for instance, you cannot report a new own property on a non-extensible object, stuff like that). The final step will then be to replace the now obsolete fix trap with the freeze and seal trap.
Attachment #609333 - Flags: review?(luke)
(In reply to Eddy Bruel [:ejpbruel] from comment #18)
> More importantly, I've added default implementations for the previously
> fundamental traps, that forward their operations to the target object. I ran
> into some trouble here, as the traps we use internally for JSObjects do not
> correspond one-on-one with the traps we use for proxies.
> 
> In particular, whereas I could forward
> ProxyHandler::getOwnPropertyDescriptor in jsproxy.cpp to
> GetOwnPropertyDescriptor in jsobj.cpp, there is no such equivalent for
> ProxyHandler::getPropertyDescriptor (there is no GetPropertyDescriptor in
> jsobj.cpp). Note that this trap is not included in the direct proxy spec,
> but it cannot be removed until we've refactored the way proto climbing works.
getPropertyDescriptor and getPropertyNames were introduced to implement some derived traps in the previous design (namely, get and set).
Based on how direct proxies work, these are not necessary any longer.
Regardless, I had shown a while ago (http://wiki.ecmascript.org/doku.php?id=strawman:proxy_derived_traps) that getPropertyDescriptor did not need to be a fundamental trap assuming access to the proxy in the trap function (which had been accepted before switching to direct proxies which do the same, but with the target)


> Until we do, we need some default implementation for
> ProxyHandler::getPropertyDescriptor that, conceptually at least, works as if
> GetPropertyDescriptor exists, and we've forwarded the operation to there.
A page of the draft spec http://wiki.ecmascript.org/doku.php?id=harmony:extended_object_api&s=getpropertynames mentions that with direct proxies, neither getPropertyDescriptor nor getPropertyNames should call a specific trap if they are ever defined. Instead, they'll use the getOwnPropertyDescriptor and getOwnPropertyNames traps of the objects on the prototype chain.

ProxyHandler::getPropertyDescriptor can go away, I think.

> I've come up with an implementation, but I'm less than confident that it is
> correct. I'd appreciate it if somebody could review it (we probably need to
> change the way getProto() works for proxies. The spec states that getProto()
> should return the prototype of the target, but I'd like to talk to dherman
> first if that actually makes sense)
As far as I know, it does make sense. What makes you think it may not?


> my next step will be to add the
> assertions mentioned in the spec to the traps in ScriptedProxyHandler (for
> instance, you cannot report a new own property on a non-extensible object,
> stuff like that). The final step will then be to replace the now obsolete
> fix trap with the freeze and seal trap.
Thanks. I'm so excited about direct proxies :-)


Additionally, the semantics of get and set have gone through a spec refactoring:
http://wiki.ecmascript.org/doku.php?id=harmony:proto_climbing_refactoring
While how they were defined in the spec was not observable, it will now be observable through the order of and nature of trap calls. It's likely that this observability will be hard to achieve and may become a source of non-interoperability between the different implementations.

Thanks for all the great work !
(In reply to David Bruant from comment #19)
> (In reply to Eddy Bruel [:ejpbruel] from comment #18)
> > More importantly, I've added default implementations for the previously
> > fundamental traps, that forward their operations to the target object. I ran
> > into some trouble here, as the traps we use internally for JSObjects do not
> > correspond one-on-one with the traps we use for proxies.
> > 
> > In particular, whereas I could forward
> > ProxyHandler::getOwnPropertyDescriptor in jsproxy.cpp to
> > GetOwnPropertyDescriptor in jsobj.cpp, there is no such equivalent for
> > ProxyHandler::getPropertyDescriptor (there is no GetPropertyDescriptor in
> > jsobj.cpp). Note that this trap is not included in the direct proxy spec,
> > but it cannot be removed until we've refactored the way proto climbing works.
> getPropertyDescriptor and getPropertyNames were introduced to implement some
> derived traps in the previous design (namely, get and set).
> Based on how direct proxies work, these are not necessary any longer.
> Regardless, I had shown a while ago
> (http://wiki.ecmascript.org/doku.php?id=strawman:proxy_derived_traps) that
> getPropertyDescriptor did not need to be a fundamental trap assuming access
> to the proxy in the trap function (which had been accepted before switching
> to direct proxies which do the same, but with the target)
> 
> 
> > Until we do, we need some default implementation for
> > ProxyHandler::getPropertyDescriptor that, conceptually at least, works as if
> > GetPropertyDescriptor exists, and we've forwarded the operation to there.
> A page of the draft spec
> http://wiki.ecmascript.org/doku.php?id=harmony:
> extended_object_api&s=getpropertynames mentions that with direct proxies,
> neither getPropertyDescriptor nor getPropertyNames should call a specific
> trap if they are ever defined. Instead, they'll use the
> getOwnPropertyDescriptor and getOwnPropertyNames traps of the objects on the
> prototype chain.
> 
> ProxyHandler::getPropertyDescriptor can go away, I think.
> 
> > I've come up with an implementation, but I'm less than confident that it is
> > correct. I'd appreciate it if somebody could review it (we probably need to
> > change the way getProto() works for proxies. The spec states that getProto()
> > should return the prototype of the target, but I'd like to talk to dherman
> > first if that actually makes sense)
> As far as I know, it does make sense. What makes you think it may not?
> 
> 
> > my next step will be to add the
> > assertions mentioned in the spec to the traps in ScriptedProxyHandler (for
> > instance, you cannot report a new own property on a non-extensible object,
> > stuff like that). The final step will then be to replace the now obsolete
> > fix trap with the freeze and seal trap.
> Thanks. I'm so excited about direct proxies :-)
> 
> 
> Additionally, the semantics of get and set have gone through a spec
> refactoring:
> http://wiki.ecmascript.org/doku.php?id=harmony:proto_climbing_refactoring
> While how they were defined in the spec was not observable, it will now be
> observable through the order of and nature of trap calls. It's likely that
> this observability will be hard to achieve and may become a source of
> non-interoperability between the different implementations.
> 
> Thanks for all the great work !

Hmmm. You have me convinced. With that, my todo list now looks as follows:

1. Refactor has, get and set to be defined in terms of getOwnPropertyDescriptor and getOwnPropertyNames
2. Get rid of getPropertyDescriptor and getPropertyNames
3. Add the freeze/seal/preventExtensions traps
4. Add the assertions mentioned earlier to the traps
(In reply to Eddy Bruel [:ejpbruel] from comment #18)
> More importantly, I've added default implementations for the previously fundamental traps, that forward their operations to the target object.

For direct proxies, the previously "derived" traps (like has, get, set, ...etc.) also have a default implementation that forwards to the target object.

Which is why I don't understand the following from comment #20:
> 1. Refactor has, get and set to be defined in terms of getOwnPropertyDescriptor and getOwnPropertyNames

The default has, get and set traps should just forward to the target.
Separate from direct proxies, but closely related, there is the "VirtualHandler" API that does provide derived traps with the fallback behavior you describe: http://wiki.ecmascript.org/doku.php?id=harmony:virtual_object_api
(In reply to Tom Van Cutsem from comment #21)
> (In reply to Eddy Bruel [:ejpbruel] from comment #18)
> > More importantly, I've added default implementations for the previously fundamental traps, that forward their operations to the target object.
> 
> For direct proxies, the previously "derived" traps (like has, get, set,
> ...etc.) also have a default implementation that forwards to the target
> object.
> 
> Which is why I don't understand the following from comment #20:
> > 1. Refactor has, get and set to be defined in terms of getOwnPropertyDescriptor and getOwnPropertyNames
> 
> The default has, get and set traps should just forward to the target.
> Separate from direct proxies, but closely related, there is the
> "VirtualHandler" API that does provide derived traps with the fallback
> behavior you describe:
> http://wiki.ecmascript.org/doku.php?id=harmony:virtual_object_api

You are correct. This is an instance of me not reading the spec properly. I should refactor the derived traps to forward to the target.
This patch should be almost feature complete. It contains the following major changes:

1. Got rid of the getPropertyDesc and getPropertyNames traps on ProxyHandler (but not on
   Proxy. Some parts of the code still rely on Proxy::getPropertyDesc. It should be removed 
   as part of the proto climbing refactor imho)
2. Added a deleteGeneric trap to ObjectOps. This is consistent with the lookup, get and set
   traps and made it easier to forward ProxyHandler::delete_. For Arrays and TypedArrays I've
   implemented deleteGeneric by forwarding to either deleteProperty or deleteElement.
3. Forwarded the remaining traps to the target, including has, hasOwn, get, set, etc
4. Refactored all the scriptable proxy traps according to the spec, including consistency
   checks mandated by the spec.
5. Added a LOT of comments to convince myself that the scriptable proxy traps actually
   adheres to the spec. I hope this actually improves readability, not hamper it. Feedback is
   welcome on this.
6. Added several comments explaining where I had to diverge from the spec and why.
7. Refactored the function signature of the call trap to be more consistent with the rest of
   the code. 
8. Made several changes in jsobj.cpp to make sure that the appropriate traps are being called
   when the object is a proxy.

I think we're now at a point where I can start writing tests for this patch. The code is still completely untested, so I'm almost certain that it is still riddled with bugs. Once I have a decent amount of tests and no more failures, I will go for a R?

In the meantime, feedback would be most welcome!
Attachment #605394 - Attachment is obsolete: true
Attachment #607088 - Attachment is obsolete: true
Attachment #607294 - Attachment is obsolete: true
Attachment #608454 - Attachment is obsolete: true
Attachment #609333 - Attachment is obsolete: true
Attachment #609333 - Flags: review?(luke)
Attachment #612578 - Attachment is patch: true
(In reply to Eddy Bruel [:ejpbruel] from comment #23)
> 6. Added several comments explaining where I had to diverge from the spec
> and why.

If these are substantial and you suspect other implementations will bump into these as well, you might want to take them to es-discuss. The proxies spec is still very much open to feedback.
(In reply to Tom Van Cutsem from comment #24)
> (In reply to Eddy Bruel [:ejpbruel] from comment #23)
> > 6. Added several comments explaining where I had to diverge from the spec
> > and why.
> 
> If these are substantial and you suspect other implementations will bump
> into these as well, you might want to take them to es-discuss. The proxies
> spec is still very much open to feedback.

I don't think that's necessary. Whenever I had to diverge from the spec, this was caused by some other part of SpiderMonkey's internals diverging from the spec. The solution should therefore be to refactor those parts of SpiderMonkey, not to refactor the spec. The most important issues I ran into were:

1. Our use of PropertyDescriptor doesn't match well with that of PropDesc. In particular, the
   latter can represent missing fields, whereas the latter cannot. According to Waldo, we 
   really should have gone with PropDesc from the start, so my suggestion is to get rid of 
   PropertyDescriptor altogether.
2. Since the Reflect API has not been implemented yet, I've been forced to forward the
   operation of each trap to the target by directly calling the traps on JSObject. Once the
   Reflect API has been implemented, this will no longer be necessary.
3. The internal traps used by JSObject do not really match with those mandated by the spec.
   This will be a problem when we want to implement the Reflect API. In most cases we can
   forward the operation of the specced traps to the internal traps without loss of semantics,
   but this is not always the case (see the patch for details).
4. Enumeration does not seem to work according to spec. In particular, based on the code in
   jsinterp.cpp, the for..in statement tries to get an iterator for the proxy, which causes 
   the iterate trap to be called. According to the spec, however, the enumerate trap should 
   be the one that gets called. The code to call this trap is present in the Snapshot function
   in jsiter.cpp, but it looks like it is never reached.

IMHO, resolving these four issues is out of scope for this particular bug, but once we do, we should be able to reach 100% conformance. Until that time, I've documented any deviations as well as I could, so we can resolve them later.
Attached patch Patch to be reviewed (obsolete) — Splinter Review
This patch is ready for review. I've added about 26 new tests, so it should be of fairly high quality. Oh well, fingers crossed :-)
Attachment #612578 - Attachment is obsolete: true
Attachment #614828 - Flags: review?(jorendorff)
Comment on attachment 614828 [details] [diff] [review]
Patch to be reviewed

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

Will this break the browser?

::: js/src/jsobj.cpp
@@ +2536,5 @@
> +                last = JS_PROPERTY_TREE(cx).getChild(cx, last,
> +                                                     self->numFixedSlots(),
> +                                                     child);
> +                if (!last)
> +                    return NULL;

false

::: js/src/jsproxy.cpp
@@ +179,5 @@
> +
> +    bool ok = false;
> +    if (JSID_IS_INT(id))
> +        ok = target->deleteElement(cx, JSID_TO_INT(id), vp, strict);
> +    else {

Moar {}

@@ +831,5 @@
> +        bool found = false;
> +        for (size_t j = 0; j < props.length(); ++j)
> +            if (props[j] == id) {
> +                found = true;
> +                break;

Wdyt about implementing a AutoIdVector::contains()? (Hmm, I thought I'd seen more places where you did this... If this is just the one place, *shrug*)

@@ +1184,5 @@
> +    /* 8. If success is true, */
> +    if (rval.value().toBoolean()) {
> +        /* 
> +         * a. Let isFixed be the result of calling
> +         *    target.[HasOwnProperty]](P)

[[

@@ +1725,5 @@
> +    /* 7. If desc is not undefined, */
> +    if (desc.obj) {
> +        /*
> +         * i. If IsDataDescriptor(desc) and desc.[[Configurable]] is false
> +         *    and desc.[[Writable] is false,

]]

@@ +1811,5 @@
> +                rval.addr()))
> +        return false;
> +
> +    /* 6. Let success be ToBoolean(result) */
> +    rval.set(BooleanValue(js_ValueToBoolean(rval.value())));

Whoa lisp parens :)))))

@@ +1824,5 @@
> +        /* b. If desc is not undefined, */
> +        if (desc.obj) {
> +            /*
> +             * i. If IsDataDescriptor(desc) and desc.[[Configurable]] is false
> +             *    and desc.[[Writable] is false,

]]

@@ +1851,5 @@
> +             */
> +            if (IsAccessorDescriptor(&desc) &&
> +                (desc.attrs & JSPROP_PERMANENT)) {
> +                /*
> +                 * 1. If desc.[[Get] is undefined, throw a TypeError (cannot

]]

@@ +2379,5 @@
>                      JSProperty **propp)
>  {
>      jsid id;
>      if (!IndexToId(cx, index, &id))
> +        return JS_FALSE;

false. Also further above.

@@ +2479,3 @@
>  {
> +    return Proxy::set(cx, obj, obj, js_CheckForStringIndex(id), vp, strict)
> +         ? JS_TRUE : JS_FALSE;

Drop ? JS_TRUE : JS_FALSE; Also further above.

@@ +2512,4 @@
>      AutoPropertyDescriptorRooter desc(cx);
> +    if (!Proxy::getOwnPropertyDescriptor(cx, obj, js_CheckForStringIndex(id),
> +                                         false, &desc))
> +        return JS_FALSE;

false

@@ +2516,2 @@
>      *attrsp = desc.attrs;
> +    return JS_TRUE;

true

@@ +2550,2 @@
>      if (!Proxy::getOwnPropertyDescriptor(cx, obj, id, true, &desc))
> +        return JS_FALSE;

false

@@ +2552,2 @@
>      desc.attrs = (*attrsp & (~JSPROP_SHORTID));
> +    return Proxy::defineProperty(cx, obj, id, &desc) ? JS_TRUE : JS_FALSE;

Drop ? JS_TRUE : JS_FALSE;

@@ +2599,4 @@
>  {
>      jsid id;
> +    return IndexToId(cx, index, &id)
> +        && proxy_DeleteGeneric(cx, obj, id, rval, strict) ? JS_TRUE : JS_FALSE;

&& at end if line, and drop ? JS_TRUE : JS_FALSE;

@@ +2901,5 @@
>      return obj;
>  }
>  
> +JSBool
> +js_Proxy(JSContext *cx, unsigned argc, Value *vp)

Would benefit from CallArgs

::: js/src/jsscope.cpp
@@ +1215,5 @@
> +     * Note that we cannot slowify the array in its preventExtensions trap,
> +     * since we set the NON_EXTENSIBLE flag in that case (see below).
> +     */
> +    if (this->isDenseArray())
> +        this->makeDenseArraySlow(cx);

Is the 'this->' required?

::: js/src/jswrapper.cpp
@@ +707,5 @@
>      AutoCompartment call(cx, wrappedObject(wrapper));
>      if (!call.enter())
>          return false;
>  
> +    // vp[0] = ObjectValue(*call.target);

Hmm?
Comment on attachment 614828 [details] [diff] [review]
Patch to be reviewed

This patch seems to be about a month out of date. It doesn't apply cleanly to mozilla-inbound tip (rejects in 10 files). Could you please rebase it and request review again?

All the test changes apply cleanly, so I can at least look at those until the new patch is up.
Attachment #614828 - Flags: review?(jorendorff)
After discussing it with bholley, I decided to land this patch incrementally, rather than in one big chunk. This should make everything much easier to review. This patch starts out by removing the fix trap from ObjectOps and ProxyHandler.
Attachment #614828 - Attachment is obsolete: true
Attachment #616298 - Flags: review?
Attachment #616298 - Flags: review? → review?(bobbyholley+bmo)
Comment on attachment 616298 [details] [diff] [review]
Patch to be reviewed (removing the fix trap)

Please make sure to look at DOM bindings that use proxies too, see http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dombindings.cpp#796
Attachment #616298 - Flags: review?(bobbyholley+bmo) → review-
Done. Thanks for pointing that out.
Attachment #616298 - Attachment is obsolete: true
Attachment #616554 - Flags: review?(bobbyholley+bmo)
Comment on attachment 616554 [details] [diff] [review]
Patch to be reviewed (removing the fix trap)

r=bholley on the stuff outside js/src assuming it builds.
Attachment #616554 - Flags: review?(bobbyholley+bmo) → review+
Attachment #616554 - Flags: review+ → review?(jorendorff)
Comment on attachment 616554 [details] [diff] [review]
Patch to be reviewed (removing the fix trap)

Please delete CallableObjectClass. The line in jscntxtinlines.h where it's mentioned can just be deleted.

In JSObject::preventExtensions, where the code does GetPropertyNames but then ignores the result, please add a comment explaining what's going on (we're forcing lazily-defined properties to be physically added ahead of setting the NOT_EXTENSIBLE bit). Without a comment, the code is pretty mysterious.
Attachment #616554 - Flags: review?(jorendorff) → review+
Done and done
Attachment #616554 - Attachment is obsolete: true
Attachment #617720 - Attachment is obsolete: true
Attachment #617746 - Attachment is obsolete: true
This patch implements AbstractProxyHandler. The idea is to create the following
inheritance hierarchy:

ProxyHandler
	No fundamental traps
	Derived traps in terms of fundamental traps
|
AbstractProxyHandler
	Fundamental traps forwarding to target
	Derived traps in terms of fundamental traps
|
DirectProxyHandler
	Fundamental traps forwarding to target
	Derived traps in terms of fundamental traps
|
BasicWrapper<Base>
	Derives from either AbstractProxyHandler or DirectProxyhandler
	Adds policy enforcement checks (enter/leave)

AbstractWrapper == BasicWrapper<AbstractProxyHandler>
DirectWrapper == BasicWrapper<DirectProxyHandler>

Between jorendorff, bholley, and myself, several alternatives to this scheme
were discussed and rejected, for different reasons. The initial idea was to
create a hierarchy like this:

ProxyHandler
|
AbstractWrapper
|
Wrapper
|
ScriptedProxyHandler

The problem with this scheme is that AbstractWrapper uses the mFamily field in
ProxyHandler to indicate that this particular proxy is a wrapper. XPConnect uses
this information to downcast ProxyHandler to AbstractWrapper and call
wrapperHandler on it. This mechanism is invalidated by the fact that
ScriptedProxyHandler also inherits from Wrapper.

We could override the mFamily field in ScriptedProxyHandler, but a much cleaner
solution in my opinion is to separate the inheritance hierarchies for wrappers
and proxies, since they represent two orthogonal concepts (trap mechanism vs
security). In doing so, we would end up with something like this:

ProxyHandler
| |
| AbstractWrapper
| |
| Wrapper
|
AbstractProxyHandler
|
DirectProxyHandler
|
ScriptedProxyHandler

But now the problem is that both AbstractWrapper and AbstractProxyHandler
implement the fundamental traps, in exactly the same way. Similarly,
Wrapper and DirectProxyHandler override the derived traps, in exactly
the same way. This leads to unnecessary code duplication.

Since the wrappers only add the policy enforcement traps, but are otherwise
identical to their corresponding proxy class, it might make sense to inherit
the wrapper classes from the proxy classes, as follows:

ProxyHandler
|
AbstractProxyHandler
| |
| AbstractWrapper
|
DirectProxyHandler
| |
| Wrapper
|
ScriptedProxyHandler

But now the problem is that AbstractWrapper and Wrapper both implement the
policy enforcement checks, in exactly the same way. Again, this leads to
unnecessary code duplication.

The only way out of this dilemma is to use multiple inheritance, but afaik
thats generally considered evil by most people. As a compromise, I propose
the use of the BasicWrapper template, which allow us to implement a wrapper
on top of an arbitrary proxy base class, by reimplementing all traps in
terms of the traps on the base class, and adding the necessary calls to
enter and leave.
Attachment #617761 - Flags: review?(jorendorff)
This patch adds the DirectProxyHandler class, as described in the previous comment.
Attachment #617775 - Flags: review?(jorendorff)
(In reply to Eddy Bruel [:ejpbruel] from comment #37)
> Created attachment 617761 [details] [diff] [review]
> Patch to be reviewed (adding the AbstractProxyHandler class)
> 
> This patch implements AbstractProxyHandler. The idea is to create the
> following
> inheritance hierarchy:
> 
> ProxyHandler
> 	No fundamental traps
> 	Derived traps in terms of fundamental traps
> |
> AbstractProxyHandler
> 	Fundamental traps forwarding to target
> 	Derived traps in terms of fundamental traps
> |
> DirectProxyHandler
> 	Fundamental traps forwarding to target
> 	Derived traps in terms of fundamental traps

Don't you mean "Derived traps forwarding to target", here?

> |
> BasicWrapper<Base>
> 	Derives from either AbstractProxyHandler or DirectProxyhandler
> 	Adds policy enforcement checks (enter/leave)
> 
> AbstractWrapper == BasicWrapper<AbstractProxyHandler>
> DirectWrapper == BasicWrapper<DirectProxyHandler>

Modulo my previous comment, this sounds good, I think. Though I might still be in favor of calling "DirectWrapper" "Wrapper" to avoid code churn (though maybe it might be good to audit all the consumers?)

So under this scheme, do AbstractWrapper and DirectWrapper have the same proxy family? My gut feeling is that it's probably better for them not to, so that anything can be safely cast to AbstractWrapper or DirectWrapper after checking the family. But I haven't thought about it too much. This would require consumers selecting the appropriate AbstractWrapper::wrappedObject vs DirectWrapper::wrappedObject, but that's probably a good thing.
> Don't you mean "Derived traps forwarding to target", here?

That's right. I messed up there.

> 
> > |
> > BasicWrapper<Base>
> > 	Derives from either AbstractProxyHandler or DirectProxyhandler
> > 	Adds policy enforcement checks (enter/leave)
> > 
> > AbstractWrapper == BasicWrapper<AbstractProxyHandler>
> > DirectWrapper == BasicWrapper<DirectProxyHandler>
> 
> Modulo my previous comment, this sounds good, I think. Though I might still
> be in favor of calling "DirectWrapper" "Wrapper" to avoid code churn (though
> maybe it might be good to audit all the consumers?)
> 

I have no particular strong feelings about what name to use. I chose DirectWrapper because it inherits from DirectProxyHandler, which is analogous to AbstractWrapper inheriting from AbstractProxyHandler. The current implementation of Wrapper will be refactored anyway: the forwarding behavior of its derived traps moves to DirectProxyHandler, and the policy enforcement functions will move to BasicWrapper<Base>. Code churn would
therefore be limited to renaming the wrappers that inherit from Wrapper.

> So under this scheme, do AbstractWrapper and DirectWrapper have the same
> proxy family? My gut feeling is that it's probably better for them not to,
> so that anything can be safely cast to AbstractWrapper or DirectWrapper
> after checking the family. But I haven't thought about it too much. This
> would require consumers selecting the appropriate
> AbstractWrapper::wrappedObject vs DirectWrapper::wrappedObject, but that's
> probably a good thing.

Yeah, I feel the same way. AbstractWrapper and DirectWrapper should be distinguishable. What I'd propose is that the proxy family is set by the leaf classes, like CrossCompartmentWrapper, ScriptedProxyHandler, etc.
(In reply to Eddy Bruel [:ejpbruel] from comment #40)
> > Don't you mean "Derived traps forwarding to target", here?
> 
> That's right. I messed up there.
> 
> > 
> > > |
> > > BasicWrapper<Base>
> > > 	Derives from either AbstractProxyHandler or DirectProxyhandler
> > > 	Adds policy enforcement checks (enter/leave)
> > > 
> > > AbstractWrapper == BasicWrapper<AbstractProxyHandler>
> > > DirectWrapper == BasicWrapper<DirectProxyHandler>
> > 
> > Modulo my previous comment, this sounds good, I think. Though I might still
> > be in favor of calling "DirectWrapper" "Wrapper" to avoid code churn (though
> > maybe it might be good to audit all the consumers?)
> > 
> 
> I have no particular strong feelings about what name to use. I chose
> DirectWrapper because it inherits from DirectProxyHandler, which is
> analogous to AbstractWrapper inheriting from AbstractProxyHandler. The
> current implementation of Wrapper will be refactored anyway: the forwarding
> behavior of its derived traps moves to DirectProxyHandler, and the policy
> enforcement functions will move to BasicWrapper<Base>. Code churn would
> therefore be limited to renaming the wrappers that inherit from Wrapper.

I was thinking more about the uses of Wrapper::Action, Wrapper::wrapperHandler, Wrapper::wrappedObject, and Wrapper::New. Checking MXR now though, and it looks like there are only a handful sprinkled throughout gecko though (and one in dom/base), so I think we should feel free to name it what we want. DirectWrapper sounds good to me.

> 
> > So under this scheme, do AbstractWrapper and DirectWrapper have the same
> > proxy family? My gut feeling is that it's probably better for them not to,
> > so that anything can be safely cast to AbstractWrapper or DirectWrapper
> > after checking the family. But I haven't thought about it too much. This
> > would require consumers selecting the appropriate
> > AbstractWrapper::wrappedObject vs DirectWrapper::wrappedObject, but that's
> > probably a good thing.
> 

> Yeah, I feel the same way. AbstractWrapper and DirectWrapper should be
> distinguishable. What I'd propose is that the proxy family is set by the
> leaf classes, like CrossCompartmentWrapper, ScriptedProxyHandler, etc.

Hmm, that would break a lot of things. We've got a huge hierarchy of wrappers on the XPConnect side, and we rely on js::UnwrapObject being able to unwrap any/all of them.

On that note though, we've got some crappy ill-defined semantics of when we unwrap what in UnwrapObject. Currently we have this stupid "stopAtOuterWindow" parameter to UnwrapObject that tells us to stop when we hit the outer window (since it's really a separate object, not a security wrapper). It would be nice to have some sort of category system "unwrap these kinds of wrappers, but not these ones", but I could never think of a good way to do this. Suggestions welcome.
Also, what's the story for spidermonkey extensions? Right now khuey is implementing all of them in a boilerplate fashion over in bug 695480, which really sucks.
(In reply to Bobby Holley (:bholley) from comment #42)
> Also, what's the story for spidermonkey extensions? Right now khuey is
> implementing all of them in a boilerplate fashion over in bug 695480, which
> really sucks.

Conceivably, we could pass the proxy family as a template parameter to BasicWrapper, allowing AbstractWrapper and DirectWrapper to have different proxy families. Would that work for you?

The Spidermonkey extensions are currently sort of implemented in ProxyHandler. However, traps like call and construct depend on proxies storing a function in their call and/or construct slots, respectively. These slots will disappear with the new scheme; instead, the target object will be used to forward the call and construct traps to directly. However, since ProxyHandler doesn't have the notion of a target object, that effectively changes call and construct to become fundamental traps.

Summarizing, under the new scheme, ProxyHandler will probably not be able to provide default implementations for some, if not all of the SpiderMonkey extensions. Therefore, these should be explicitly implemented whenever you're implementing a wrapper based on AbstractWrapper. DirectProxyHandler, on the other hand, *can* provide default implementations for the extensions, so a wrapper based on DirectWrapper doesn't need to provide explicit implementations for these traps.
(In reply to Eddy Bruel [:ejpbruel] from comment #43)
> (In reply to Bobby Holley (:bholley) from comment #42)
> > Also, what's the story for spidermonkey extensions? Right now khuey is
> > implementing all of them in a boilerplate fashion over in bug 695480, which
> > really sucks.
> 
> Conceivably, we could pass the proxy family as a template parameter to
> BasicWrapper, allowing AbstractWrapper and DirectWrapper to have different
> proxy families. Would that work for you?
> 
> The Spidermonkey extensions are currently sort of implemented in
> ProxyHandler. However, traps like call and construct depend on proxies
> storing a function in their call and/or construct slots, respectively. These
> slots will disappear with the new scheme; instead, the target object will be
> used to forward the call and construct traps to directly. However, since
> ProxyHandler doesn't have the notion of a target object, that effectively
> changes call and construct to become fundamental traps.
> 
> Summarizing, under the new scheme, ProxyHandler will probably not be able to
> provide default implementations for some, if not all of the SpiderMonkey
> extensions. Therefore, these should be explicitly implemented whenever
> you're implementing a wrapper based on AbstractWrapper. DirectProxyHandler,
> on the other hand, *can* provide default implementations for the extensions,
> so a wrapper based on DirectWrapper doesn't need to provide explicit
> implementations for these traps.

After thinking about it some more, I think we should just leave the SpiderMonkey extensions where they are. Earlier I said that ProxyHandler doesn't know about the target object, but that's not actually correct. The fact that AbstractProxyHandler implements the fundamental traps by forwarding them to the target, whereas ProxyHandler leaves them unimplemented does not imply that the latter doesn't know about the target object. The only reason we have this distinction is that some DOM bindings want to implement the fundamental traps in a different way, and therefore do not want to inherit from AbstractProxyHandler.

In fact, it makes sense that ProxyHandler *would* know about the target object. The reason is that when a proxy object is created its class is set to either ObjectProxyClass or FunctionProxyClass. In the new proposal, this decision will be based on the type of the target object. In other words, its impossible to create a proxy object without passing it a target object. The target object is stored in the private field of the proxy object, and consequently, ProxyHandler should be able to access it.

This is convenient for us, because it allows the default implementation of the SpiderMonkey extensions that currently live in ProxyHandler to stay where they are, and therefore, the DOM bindings that inherit from ProxyHandler don't have to be changed in that respect.
> Conceivably, we could pass the proxy family as a template parameter to
> BasicWrapper, allowing AbstractWrapper and DirectWrapper to have different proxy
> families. Would that work for you?

It seems like it would be simpler to have it be the address of a member of the singleton, giving us one-per-specialization and guaranteeing uniqueness. Is there a reason this wouldn't work?

> Summarizing, under the new scheme, ProxyHandler will probably not be able to
> provide default implementations for some, if not all of the SpiderMonkey
> extensions. Therefore, these should be explicitly implemented whenever you're
> implementing a wrapper based on AbstractWrapper.

So, I don't so much care that they do something useful. I just want them to not assert. I was (mistakenly) under the impression that they _all_ asserted, but it looks like it's just regexp_toShared that asserts. So in general I withdraw my criticism and think that the ProxyHandler implementations of the SM extensions (by and large, throwing) are fine.

> In fact, it makes sense that ProxyHandler *would* know about the target object.
> The reason is that when a proxy object is created its class is set to either 
> ObjectProxyClass or FunctionProxyClass. In the new proposal, this decision will
> be based on the type of the target object. In other words, its impossible to
> create a proxy object without passing it a target object. The target object is
> stored in the private field of the proxy object, and consequently, ProxyHandler
> should be able to access it.

I disagree with this. The whole point of this proxy/wrapper distinction is that wrappers have a target object, whereas proxies are just objects whose behavior is implemented via traps (which makes sense for consumers like NodeLists). If we need to make the object/function distinction, the caller doing the proxy construction can just specify what kind of object they want to impersonate. AFAICT, the ProxyHandler in trunk has no notion of a target object (which is why most of the SM extension just throw), so this would be a departure from the current setup.
(In reply to Bobby Holley (:bholley) from comment #45)
> > In fact, it makes sense that ProxyHandler *would* know about the target object.
> > The reason is that when a proxy object is created its class is set to either 
> > ObjectProxyClass or FunctionProxyClass. In the new proposal, this decision will
> > be based on the type of the target object. In other words, its impossible to
> > create a proxy object without passing it a target object. The target object is
> > stored in the private field of the proxy object, and consequently, ProxyHandler
> > should be able to access it.
> 
> I disagree with this. The whole point of this proxy/wrapper distinction is
> that wrappers have a target object, whereas proxies are just objects whose
> behavior is implemented via traps (which makes sense for consumers like
> NodeLists). If we need to make the object/function distinction, the caller
> doing the proxy construction can just specify what kind of object they want
> to impersonate. AFAICT, the ProxyHandler in trunk has no notion of a target
> object (which is why most of the SM extension just throw), so this would be
> a departure from the current setup.

Its true that the ProxyHandler in trunk has no notion of a target object, but that's only achieved through somewhat artifical means. First of all, ProxyHandler can infer information about the target by looking at the class of the proxy object. Furthermore, a proxy object with class FunctionProxyClass stores a call and/or construct trap in some reserved slots. This information is used by ProxyHandler to provide default implementations for the SpiderMonkey extensions, and call and construct in particular.

With the new scheme, these call and construct traps will disappear. Instead, the default implementation for call and construct should forward their operation directly to the target object (which is assumed to be a function object in that case). So we really have two choices here. Either we say that ProxyHandler is aware of the target object. The default implementations for call and construct can stay where they are in that case, and we break no existing code. This is what I did right now.

The alternative is to say that Proxy handler is *not* aware of the target object. In that case, the default implementations for call and construct have to move to AbstractProxyHandler, since we can no longer implement them using the call and/or construct trap. This implies that the call and construct extensions are promoted to fundamental traps in ProxyHandler. However, note that ProxyHandler can still infer information about the type of the target object in that case, and even access the target object directly by calling GetProxyTargetObject, even though it's not supposed to be aware of it.
Eddy and I talked about this on IRC. We decided that ProxyHandler::{call,construct} would just throw, and that the forwarding versions would live on AbstractProxyHandler.
I revised the patch that adds the AbtractWrapper class to incorporate the changes that I discussed with bholley. Summarizing:

- ProxyHandler:
	- No fundamental traps
	- Derived traps in terms of fundamental traps
	- Extensions throw

- AbstractProxyHandler:
	- Fundamental traps forward to target
	- Derived traps in terms of fundamental traps
	- Extensions forward to target

Note that for scripted proxies, the target object is currently the same as the handler object. This will be split up eventually. For wrappers, the target object is the wrapped object.

Both AbstractWrapper and ScriptedProxyHandler now inherit from AbstractProxyHandler. Wrapper still inherits from AbstractWrapper (this will change to DirectProxyHandler eventually).

AbstractWrapper also forwarded the fundamental traps to the wrapped object. In addition, Wrapper also forwarded some extensions to the wrapped object. AbstractWrapper is responsible for this now, so this functionality has been removed.

Since ScriptedProxyHandler now inherits from AbstractProxyHandler instead of ProxyHandler, a couple of extensions that would previously throw for scripted proxies now instead forward to the target object. This invalidates several tests.
Attachment #617761 - Attachment is obsolete: true
Attachment #617775 - Attachment is obsolete: true
Attachment #617761 - Flags: review?(jorendorff)
Attachment #617775 - Flags: review?(jorendorff)
Attachment #618435 - Flags: review?(jorendorff)
Reuploaded the patch that adds the DirectProxyHandler class. Note that all DirectProxyHandler needs to do is forward the fundamental traps to the target/wrapped object. This functionality is basically just copied from Wrapper. Once BasicWrapper<Base> is added, we can implement Wrapper in terms of DirectProxyHandler and remove the duplicated functionality.
Attachment #618440 - Flags: review?(jorendorff)
This patch adds the BaseWrapper template.

The next step will be to reimplement AbstractWrapper and Wrapper as simple typedefs and make sure that this doesn't break the tests or anything in XPConnect. Once that works, we will have the class hierarchy described in comment 37.
Attachment #618444 - Flags: review?(jorendorff)
Attachment #618435 - Attachment is patch: true
Attachment #618435 - Attachment is obsolete: true
Attachment #618435 - Flags: review?(jorendorff)
Attachment #618773 - Flags: review?(jorendorff)
I'm backing out the patch that added the BaseWrapper template; as it turns out this template it depends on several private definition, and therefore wouldn't compile. This patch changes the nativeCall trap to take CallArgs* instead of CallArgs as an argument, thus partially solving that problem.
Attachment #618444 - Attachment is obsolete: true
Attachment #618444 - Flags: review?(jorendorff)
Attachment #618781 - Flags: review?(jorendorff)
This patch renames the Wrapper class to DirectWrapper. This is necessary because once the BasicWrapper template lands, DirectWrapper will no longer inherit from AbstractWrapper, yet functions like UnwrapObjectChecked expect wrappers to have a common base class, and Wrapper is the most obvious name for such a class.
Attachment #619033 - Flags: review?
Fixed an oversight in the patch that adds the AbstractWrapper class; although the forwarding behavior of this class now moved to AbstractProxyHandler, it still needs to wrap the fundamental traps in an enter/leave pair.
Attachment #618773 - Attachment is obsolete: true
Attachment #618773 - Flags: review?(jorendorff)
Attachment #619062 - Flags: review?(jorendorff)
Attachment #619033 - Flags: review? → review?(jorendorff)
Attachment #619062 - Attachment is obsolete: true
Attachment #619062 - Flags: review?(jorendorff)
Attachment #619077 - Flags: review?
Comment on attachment 618440 [details] [diff] [review]
Patch to be reviewed (adding the DirectProxyHandler class)

Clearing the r? flag on a few of these since ejpbruel and bholley figured out another approach.
Attachment #618440 - Flags: review?(jorendorff)
Attachment #618781 - Flags: review?(jorendorff)
Attachment #619033 - Flags: review?(jorendorff)
Keywords: dev-doc-needed
Target Milestone: --- → mozilla14
Version: unspecified → Trunk
Attachment 617748 [details] [diff] landed on inbound as the following changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9be410ceaf

Merged to mozilla-central as:
http://hg.mozilla.org/mozilla-central/rev/cf9be410ceaf

Eddy, please follow the tree rules when landing on inbound:
https://wiki.mozilla.org/Tree_Rules/Inbound

Also, since I have no clue if what landed is all that's intended for this bug, I'm leaving it open. Please resolve it if nothing else is landing.
Flags: in-testsuite?
Target Milestone: mozilla14 → mozilla15
(In reply to Ryan VanderMeulen from comment #57)
> http://hg.mozilla.org/mozilla-central/rev/cf9be410ceaf

The change to js/src/tests/e4x/extensions/extensibility.js doesn't appear to have been reviewed?
(In reply to Ms2ger from comment #58)
> (In reply to Ryan VanderMeulen from comment #57)
> > http://hg.mozilla.org/mozilla-central/rev/cf9be410ceaf
> 
> The change to js/src/tests/e4x/extensions/extensibility.js doesn't appear to
> have been reviewed?

This was noted after the review and discussed with jorendorff on irc. It should be disabled until the preventExtensions trap is added.
(In reply to Ryan VanderMeulen from comment #57)
> Attachment 617748 [details] [diff] landed on inbound as the following
> changeset:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9be410ceaf
> 
> Merged to mozilla-central as:
> http://hg.mozilla.org/mozilla-central/rev/cf9be410ceaf
> 
> Eddy, please follow the tree rules when landing on inbound:
> https://wiki.mozilla.org/Tree_Rules/Inbound
> 
> Also, since I have no clue if what landed is all that's intended for this
> bug, I'm leaving it open. Please resolve it if nothing else is landing.

This bug will land in a series of patches, so expect a couple of more things to land before the bug is resolved.

Also, was your comment about the tree rules meant to be generic, or meant to imply that I broke the tree rules in some manner? Seeing as this was my first push to inbound, please let me know if there is something in specific that I should or shouldn't do in the future.
(In reply to Eddy Bruel [:ejpbruel] from comment #60)
> Also, was your comment about the tree rules meant to be generic, or meant to
> imply that I broke the tree rules in some manner? Seeing as this was my
> first push to inbound, please let me know if there is something in specific
> that I should or shouldn't do in the future.

It was directed at you. Please take note of the section about what to do after landing on inbound (such as setting target milestone, posting the changeset link in the bug, and so forth). In this case, it would also have been useful to set the checkin+ flag on the patch that was checked in so it's clear what has landed and what hasn't. Thanks!
Because jorendorff is currently swamped with his review queue, bholley
volunteered to review the patches that relate to the refactor of the proxy
handler/wrapper class hierarchy. Incidentally, this should also be the only
patches that affect xpconnect, so bholley is a perfect candidate for this.

Anyway, bholley, to make your life a little easier, let's recap once again what
we're trying to do here. Currently, our class hierarchy looks like this:

ProxyHandler
| |
| AbstractWrapper
| |
| Wrapper
|
ScriptedProxyHandler

In our new scheme, ScriptedProxyHandler and Wrapper both forward all their traps
to the target/wrapped object, so this can code can be abstracted into a common
base class, DirectProxyHandler. DirectProxyHandler and AbstractWrapper both
forward their fundamental traps to the target/wrapped object, so we can further
abstract into a second common base class, AbstractProxyHandler.

However, Wrapper inherits from AbstractWrapper in order to get the policy
enforcement methods enter and leave. It cannot inherit from DirectProxyHandler
as well, because that would create a dreaded diamond with ProxyHandler at the
top. To avoid this, we rename Wrapper to DirectWrapper, and move enter and leave
to a separate base class, Wrapper, that does not inherit from ProxyHandler.

The end result will be thus:

ProxyHandler     Wrapper
|                    | |
AbstractProxyHandler | |
|                  | | |
|      AbstractWrapper |
|                      |
DirectProxyHandler     |
|                |     |
|          DirectWrapper
|
ScriptedProxyHandler

As you can see, this is refactor will be quite complex. To keep this complexity
managable, I'm breaking it up in a couple of steps:

1. Add AbstractProxyHandler
   Inherit AbstractWrapper from AbstractProxyHandler
   Inherit ScriptedProxyHandler from AbstractProxyHandler
   Move all forwarding code from AbstractWrapper to AbstractProxyHandler
   Make sure that all the SpiderMonkey extensions in ProxyHandler throw
   Make sure that all the SpiderMonkey extensions in AbstractProxyHandler forward
2. Rename Wrapper to DirectWrapper
3. Add Wrapper 
   Inherit AbstractWrapper from Wrapper
   Move the policy enforcement code to Wrapper
4. Add DirectProxyHandler
   Inherit DirectWrapper from Wrapper
   Inherit DirectWrapper from DirectProxyHandler
   Move all forwarding code from DirectWrapper to DirectProxyHandler

The following patch implements the first of these steps. Note that because
ScriptedProxyHandler now inherits from AbstractProxyHandler, the behavior of
some of its SpiderMonkey extensions has now changed (in particular that of
defaultValue and obj_toString) Instead of throwing, these now forward to the
handler object (which will eventually be split up into a separate handler and
target object)
Attachment #618440 - Attachment is obsolete: true
Attachment #618781 - Attachment is obsolete: true
Attachment #619033 - Attachment is obsolete: true
Attachment #619077 - Attachment is obsolete: true
Attachment #619077 - Flags: review?
Attachment #621058 - Flags: review?
Attachment #621058 - Flags: review? → review?(bobbyholley+bmo)
Eddy and I just talked about this on IRC, and decided to rename Abstract* to Indirect*. He's on PTO today, so I did an s/AbstractProxyHandler/IndirectProxyHandler/g and am reposting the patch.
Attachment #621058 - Attachment is obsolete: true
Attachment #621058 - Flags: review?(bobbyholley+bmo)
Attachment #622363 - Flags: review?(bobbyholley+bmo)
Attachment #622363 - Attachment description: Part 1 - Add IndirectProcyHandler → Part 1 - Add IndirectProxyHandler
Comment on attachment 622363 [details] [diff] [review]
Part 1 - Add IndirectProxyHandler

>diff -r 9ebf3dc839c5 js/src/jit-test/tests/basic/testCrossCompartmentTransparency2.js

>diff -r 9ebf3dc839c5 js/src/jit-test/tests/basic/testCrossGlobalInvokeSession.js

>diff -r 9ebf3dc839c5 js/src/jit-test/tests/basic/testReconstructImacroPCStack.js

Why do these need to die? I'm sure you have a reason, but it's not immediately clear to me.

> JSString *
> ProxyHandler::obj_toString(JSContext *cx, JSObject *proxy)
> {
>-    JS_ASSERT(proxy->isProxy());
>-
>-    return JS_NewStringCopyZ(cx, IsFunctionProxy(proxy)
>-                                 ? "[object Function]"
>-                                 : "[object Object]");
>+    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                         JSMSG_INCOMPATIBLE_PROTO,
>+                         js_Object_str, js_toString_str,
>+                         "object");
>+    return NULL;
> }

I'd almost be tempted to have this just output "[object Proxy]". But I don't have a strong opinion.


>+IndirectProxyHandler::getPropertyDescriptor(JSContext *cx, JSObject *proxy,

I didn't look at this stuff closely. I'm assuming that all the implementations are the same as they were for Wrapper.


>+class JS_PUBLIC_API(IndirectProxyHandler) : public ProxyHandler {

Same here.

>+  public:
>+    explicit IndirectProxyHandler(void *family);

I think we should have some sort of ::New function here, that does what Wrapper::New does (ie, stores the target object in the proxy private). This is the class where the notion of target object is introduced, so I think it makes sense for that function to live here.

>+
>+    /* ES5 Harmony fundamental proxy traps. */
>+    virtual bool getPropertyDescriptor(JSContext *cx, JSObject *proxy, jsid id,
>+                                       bool set,
>+                                       PropertyDescriptor *desc) MOZ_OVERRIDE;
>+    virtual bool getOwnPropertyDescriptor(JSContext *cx, JSObject *proxy,
>+                                          jsid id, bool set,
>+                                          PropertyDescriptor *desc) MOZ_OVERRIDE;
>+    virtual bool defineProperty(JSContext *cx, JSObject *proxy, jsid id,
>+                                PropertyDescriptor *desc) MOZ_OVERRIDE;
>+    virtual bool getOwnPropertyNames(JSContext *cx, JSObject *proxy,
>+                                     AutoIdVector &props) MOZ_OVERRIDE;
>+    virtual bool delete_(JSContext *cx, JSObject *proxy, jsid id,
>+                         bool *bp) MOZ_OVERRIDE;
>+    virtual bool enumerate(JSContext *cx, JSObject *proxy,
>+                           AutoIdVector &props) MOZ_OVERRIDE;

Add a comment here noting that the derived traps are implemented in terms of the fundamental ones.

>-bool
>-Wrapper::defaultValue(JSContext *cx, JSObject *wrapper, JSType hint, Value *vp)

This is going to race with another bug. I'll CC you on it.

>-    virtual void trace(JSTracer *trc, JSObject *wrapper) MOZ_OVERRIDE;
>+    virtual void trace(JSTracer *trc, JSObject *wrapper);

Why is this no longer MOZ_OVERRIDE?


These Proxies are functionally wrappers, so the only thing Wrapper is going to be good for is the policy enforcement traps. So maybe we should start calling "Wrapper" "SecurityWrapper".

Also, I'm starting to think that we should in fact call ProxyHandler AbstractProxyHandler. The reason is that, currently, 'IndirectProxyHandler' sounds like it's somehow "less direct" than "ProxyHandler", given that ProxyHandler has no adjective. So I'd be in favor of that.

r=bholley on this part, contingent on a green try run.
Attachment #622363 - Flags: review?(bobbyholley+bmo) → review+
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b98aee2cdf4f to mozilla-inbound.

Note that I had to add a few more lines in order to avoid a regression. Discussed the changes with jorendorff and he agreed.
Attachment #622363 - Flags: checkin+
(In reply to Eddy Bruel [:ejpbruel] from comment #65)
> Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b98aee2cdf4f
> to mozilla-inbound.
> 

http://hg.mozilla.org/mozilla-central/rev/b98aee2cdf4f

Not resolving per comment 60. Please put [leave open] in the whiteboard to facilitate that in the future.
Attached patch Part 2 - Add DirectProxyHandler (obsolete) — Splinter Review
Attachment #625538 - Flags: review?(bobbyholley+bmo)
Comment on attachment 625538 [details] [diff] [review]
Part 2 - Add DirectProxyHandler

Erm, there is something very wrong with this patch.
Attachment #625538 - Flags: review?(bobbyholley+bmo) → review-
(In reply to Bobby Holley (:bholley) from comment #68)
> Erm, there is something very wrong with this patch.

(And it case it wasn't clear, I mean that the file is corrupt).
Attached patch Part 2 - Add DirectProxyHandler (obsolete) — Splinter Review
Attachment #625538 - Attachment is obsolete: true
Attachment #625648 - Flags: review?(bobbyholley+bmo)
Comment on attachment 625648 [details] [diff] [review]
Part 2 - Add DirectProxyHandler

As noted on IRC, s/AbstractProxyHandler/IndirectProxyHandler/.

Also, the Wrapper functions you're cribbing from just changed in bug 750733 to use the new Handle API. Please update the ones in your patch.

r=bholley with those changes.
Attachment #625648 - Flags: review?(bobbyholley+bmo) → review+
Depends on: 757063
No longer depends on: 756214
Done, and made sure it compiles this time. Sorry for being sloppy! :-)
Attachment #625648 - Attachment is obsolete: true
Attachment #625662 - Flags: review?(bobbyholley+bmo)
Attachment #625662 - Flags: review?(bobbyholley+bmo) → review+
Whiteboard: [leave open]
Attachment #626914 - Flags: review?
Attachment #626914 - Flags: review? → review?(bobbyholley+bmo)
Comment on attachment 626914 [details] [diff] [review]
Part 3 - Rename Wrapper to DirectWrapper

As discussed on IRC, r- on the grounds that this code churn can be avoided with a typedef.
Attachment #626914 - Flags: review?(bobbyholley+bmo) → review-
This patch adds a typedef that allows us to avoid code churning inside xpconnect.

Note that when the Wrapper base class lands, for almost every current use of Wrapper as DirectWrapper, we will be able to use the base class Wrapper instead (all enums and functions like wrappedObject will all move to this base). One notable exception though is Wrapper::New. This should be renamed to DirectWrapper::New, because we want to be explicit about the kind of wrapper that is created.
Attachment #626914 - Attachment is obsolete: true
Attachment #626993 - Flags: review?(bobbyholley+bmo)
(In reply to Eddy Bruel [:ejpbruel] from comment #77)
> This should be renamed
> to DirectWrapper::New, because we want to be explicit about the kind of
> wrapper that is created.

Hm. Why? We're already quite explicit in that we pass in the proxy handler that's to be used. I don't see why it's particularly important to make this distinction, or how the implementation of IndirectWrapper::New would vary at all.

Also, this patch still seems to have a number of renames of Wrapper::wrapperHandler to DirectWrapper::wrapperHandler and such. Or am I looking at the wrong thing?
Annoyingly, you are right on both accounts :-)
Attachment #626993 - Attachment is obsolete: true
Attachment #626993 - Flags: review?(bobbyholley+bmo)
Attachment #627417 - Flags: review?(bobbyholley+bmo)
Attached patch Part 4 - Add Wrapper base class (obsolete) — Splinter Review
Attachment #628012 - Flags: review?(bobbyholley+bmo)
Comment on attachment 627417 [details] [diff] [review]
Part 3 - Rename Wrapper to DirectWrapper

Er, doesn't comment 78 still apply?
Attachment #627417 - Flags: review?(bobbyholley+bmo) → review-
I wrongly assumed that you only wanted to avoid theses uses in XPConnect, but after the Wrapper base class will land, it actually makes sense to use Wrapper::wrapperHandler and Wrapper::New everywhere.
Attachment #627417 - Attachment is obsolete: true
Attachment #628120 - Flags: review?(bobbyholley+bmo)
Attached patch Part 4 - Add Wrapper base class (obsolete) — Splinter Review
Attachment #628012 - Attachment is obsolete: true
Attachment #628012 - Flags: review?(bobbyholley+bmo)
Attachment #628121 - Flags: review?(bobbyholley+bmo)
Attachment #628120 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 628121 [details] [diff] [review]
Part 4 - Add Wrapper base class

This appears to be noise.
Attachment #628121 - Flags: review?(bobbyholley+bmo) → review-
Huh? How did that happen? That's just typical :-)
Attachment #628121 - Attachment is obsolete: true
Attachment #628256 - Flags: review?(bobbyholley+bmo)
Just for the heads up, at the TC39 May meeting, it was strongly considered to standardize __proto__. It led to considering a new getPrototypeOf trap: https://mail.mozilla.org/pipermail/es-discuss/2012-May/022894.html
Even though its hasn't reached the spec draft yet, it does have gathered general agreement on es-discuss (see following messages in the same thread).

My opinion is that it should be implemented.
If it's too much work for the first iteration, that's not a big deal since a getPrototypeOf trap can be easily polyfilled by monkey-patching both Proxy and Object.getPrototypeOf.


I haven't taken the time to test with the current proxy implementation nor with the upcoming one, but I wish some tests to be added specifically to test that __proto__ on a proxy triggers the get and set trap of the proxy and doesn't change its prototype without being trapped.

Thanks for all the work. I see progress and it's quite exciting :-)
I plan to raise the getPrototypeOf trap change briefly at the July TC39 meeting, to get actual approval. Hopefully by that time there will be more details as to how __proto__ will be specced in ES6.

In any case, I agree |proxy.__proto__| and |proxy.__proto__ = val| should pass via the get/set traps, even today (without a getPrototypeOf trap).
(In reply to David Bruant from comment #86)
> Just for the heads up, at the TC39 May meeting, it was strongly considered
> to standardize __proto__. It led to considering a new getPrototypeOf trap:
> https://mail.mozilla.org/pipermail/es-discuss/2012-May/022894.html
> Even though its hasn't reached the spec draft yet, it does have gathered
> general agreement on es-discuss (see following messages in the same thread).
> 
> My opinion is that it should be implemented.
> If it's too much work for the first iteration, that's not a big deal since a
> getPrototypeOf trap can be easily polyfilled by monkey-patching both Proxy
> and Object.getPrototypeOf.
> 
> 
> I haven't taken the time to test with the current proxy implementation nor
> with the upcoming one, but I wish some tests to be added specifically to
> test that __proto__ on a proxy triggers the get and set trap of the proxy
> and doesn't change its prototype without being trapped.
> 
> Thanks for all the work. I see progress and it's quite exciting :-)

I'm fine with adding a getPrototypeOf trap in this iteration, provided we don't run into any major problems. I want to land a working version as soon as possible. After patch 4 lands, we have 2 more patches to go before the class refactor is complete. The patches after that will implement the direct proxies proper.
Comment on attachment 628256 [details] [diff] [review]
Part 4 - Add Wrapper base class


>+    /*
>+     * The function Wrapper::wrapperHandler takes a pointer to a
>+     * BaseProxyHandler and returns a pointer to a Wrapper if and only if the
>+     * BaseProxyHandler is a wrapper handler (otherwise, it returns NULL).
>+     *
>+     * Unfortunately, we can't inherit Wrapper from BaseProxyHandler, since that
>+     * would create a dreaded diamond, and we can't use dynamic_cast to cast
>+     * BaseProxyHandler to Wrapper, since that would require us to compile with
>+     * run-time type information. Hence the need for this virtual function.
>+     */
>+    virtual Wrapper *toWrapper() {
>+        return NULL;
>+    }

Can't we just check for the Wrapper proxy family? Though it's probably a useful method to have so that it's easy to go back and forth.

As a matter of fact, if we reimplemented isWrapper as |toWrapper() != NULL|, we could probably get rid of proxy families entirely.



> bool
> DirectWrapper::has(JSContext *cx, JSObject *wrapper, jsid id, bool *bp)
> {
>     *bp = false; // default result if we refuse to perform this action
>     JSBool found;
>-    GET(JS_HasPropertyById(cx, wrappedObject(wrapper), id, &found) &&
>+    GET(JS_HasPropertyById(cx, GetProxyTargetObject(wrapper), id, &found) &&
>         Cond(found, bp));
> }
> 
> bool
> DirectWrapper::hasOwn(JSContext *cx, JSObject *wrapper, jsid id, bool *bp)
> {
>     *bp = false; // default result if we refuse to perform this action
>     PropertyDescriptor desc;
>-    JSObject *wobj = wrappedObject(wrapper);
>+    JSObject *wobj = GetProxyTargetObject(wrapper);
>     GET(JS_GetPropertyDescriptorById(cx, wobj, id, JSRESOLVE_QUALIFIED, &desc) &&
>         Cond(desc.obj == wobj, bp));
> }
> 
> bool
> DirectWrapper::get(JSContext *cx, JSObject *wrapper, JSObject *receiver, jsid id, Value *vp)
> {
>     vp->setUndefined(); // default result if we refuse to perform this action
>-    GET(wrappedObject(wrapper)->getGeneric(cx, RootedVarObject(cx, receiver), RootedVarId(cx, id), vp));
>+    GET(GetProxyTargetObject(wrapper)->getGeneric(cx, RootedVarObject(cx, receiver), RootedVarId(cx, id), vp));
> }
> 
> bool
> DirectWrapper::set(JSContext *cx, JSObject *wrapper, JSObject *receiver, jsid id, bool strict,
>                Value *vp)
> {
>-    SET(wrappedObject(wrapper)->setGeneric(cx, RootedVarId(cx, id), vp, strict));
>+    SET(GetProxyTargetObject(wrapper)->setGeneric(cx, RootedVarId(cx, id), vp, strict));
> }
> 
> bool
> DirectWrapper::keys(JSContext *cx, JSObject *wrapper, AutoIdVector &props)
> {
>     // if we refuse to perform this action, props remains empty
>     const jsid id = JSID_VOID;
>-    GET(GetPropertyNames(cx, wrappedObject(wrapper), JSITER_OWNONLY, &props));
>+    GET(GetPropertyNames(cx, GetProxyTargetObject(wrapper), JSITER_OWNONLY, &props));
> }
> 
> bool
> DirectWrapper::iterate(JSContext *cx, JSObject *wrapper, unsigned flags, Value *vp)
> {
>     vp->setUndefined(); // default result if we refuse to perform this action
>     const jsid id = JSID_VOID;
>-    GET(GetIterator(cx, RootedVarObject(cx, wrappedObject(wrapper)), flags, vp));
>+    GET(GetIterator(cx, RootedVarObject(cx, GetProxyTargetObject(wrapper)), flags, vp));
> }
> 
>-#define PIERCE(cx, wrapper, mode, pre, op, post)            \
>-    JS_BEGIN_MACRO                                          \
>-        AutoCompartment call(cx, wrappedObject(wrapper));   \
>-        if (!call.enter())                                  \
>-            return false;                                   \
>-        bool ok = (pre) && (op);                            \
>-        call.leave();                                       \
>-        return ok && (post);                                \
>+#define PIERCE(cx, wrapper, mode, pre, op, post)                   \
>+    JS_BEGIN_MACRO                                                 \
>+        AutoCompartment call(cx, GetProxyTargetObject(wrapper));   \
>+        if (!call.enter())                                         \
>+            return false;                                          \
>+        bool ok = (pre) && (op);                                   \
>+        call.leave();                                              \
>+        return ok && (post);                                       \
>     JS_END_MACRO
> 
> #define NOTHING (true)
> 
> bool
> CrossCompartmentWrapper::getPropertyDescriptor(JSContext *cx, JSObject *wrapper, jsid id,
>                                                bool set, PropertyDescriptor *desc)
> {
>@@ -631,17 +637,17 @@ CrossCompartmentWrapper::iterate(JSConte
>            CanReify(vp) ? Reify(cx, call.origin, vp) : call.origin->wrap(cx, vp));
> }
> 
> bool
> CrossCompartmentWrapper::call(JSContext *cx, JSObject *wrapper_, unsigned argc, Value *vp)
> {
>     RootedVarObject wrapper(cx, wrapper_);
> 
>-    AutoCompartment call(cx, wrappedObject(wrapper));
>+    AutoCompartment call(cx, GetProxyTargetObject(wrapper));
>     if (!call.enter())
>         return false;
> 
>     vp[0] = ObjectValue(*call.target);
>     if (!call.destination->wrap(cx, &vp[1]))
>         return false;
>     Value *argv = JS_ARGV(cx, vp);
>     for (size_t n = 0; n < argc; ++n) {
>@@ -656,17 +662,17 @@ CrossCompartmentWrapper::call(JSContext 
> }
> 
> bool
> CrossCompartmentWrapper::construct(JSContext *cx, JSObject *wrapper_, unsigned argc, Value *argv,
>                                    Value *rval)
> {
>     RootedVarObject wrapper(cx, wrapper_);
> 
>-    AutoCompartment call(cx, wrappedObject(wrapper));
>+    AutoCompartment call(cx, GetProxyTargetObject(wrapper));
>     if (!call.enter())
>         return false;
> 
>     for (size_t n = 0; n < argc; ++n) {
>         if (!call.destination->wrap(cx, &argv[n]))
>             return false;
>     }
>     if (!DirectWrapper::construct(cx, wrapper, argc, argv, rval))
>@@ -683,17 +689,17 @@ bool
> CrossCompartmentWrapper::nativeCall(JSContext *cx, JSObject *wrapper, Class *clasp, Native native, CallArgs srcArgs)
> {
>     JS_ASSERT_IF(!srcArgs.calleev().isUndefined(),
>                  srcArgs.callee().toFunction()->native() == native ||
>                  srcArgs.callee().toFunction()->native() == js_generic_native_method_dispatcher);
>     JS_ASSERT(srcArgs.thisv().isMagic(JS_IS_CONSTRUCTING) || &srcArgs.thisv().toObject() == wrapper);
>     JS_ASSERT(!UnwrapObject(wrapper)->isCrossCompartmentWrapper());
> 
>-    JSObject *wrapped = wrappedObject(wrapper);
>+    JSObject *wrapped = GetProxyTargetObject(wrapper);
>     AutoCompartment call(cx, wrapped);
>     if (!call.enter())
>         return false;
> 
>     InvokeArgsGuard dstArgs;
>     if (!cx->stack.pushInvokeArgs(cx, srcArgs.length(), &dstArgs))
>         return false;
> 
>@@ -713,64 +719,64 @@ CrossCompartmentWrapper::nativeCall(JSCo
>     dstArgs.pop();
>     call.leave();
>     return call.origin->wrap(cx, &srcArgs.rval());
> }
> 
> bool
> CrossCompartmentWrapper::hasInstance(JSContext *cx, JSObject *wrapper, const Value *vp, bool *bp)
> {
>-    AutoCompartment call(cx, wrappedObject(wrapper));
>+    AutoCompartment call(cx, GetProxyTargetObject(wrapper));
>     if (!call.enter())
>         return false;
> 
>     Value v = *vp;
>     if (!call.destination->wrap(cx, &v))
>         return false;
>     return DirectWrapper::hasInstance(cx, wrapper, &v, bp);
> }
> 
> JSString *
> CrossCompartmentWrapper::obj_toString(JSContext *cx, JSObject *wrapper)
> {
>-    AutoCompartment call(cx, wrappedObject(wrapper));
>+    AutoCompartment call(cx, GetProxyTargetObject(wrapper));
>     if (!call.enter())
>         return NULL;
> 
>     JSString *str = DirectWrapper::obj_toString(cx, wrapper);
>     if (!str)
>         return NULL;
> 
>     call.leave();
>     if (!call.origin->wrap(cx, &str))
>         return NULL;
>     return str;
> }
> 
> JSString *
> CrossCompartmentWrapper::fun_toString(JSContext *cx, JSObject *wrapper, unsigned indent)
> {
>-    AutoCompartment call(cx, wrappedObject(wrapper));
>+    AutoCompartment call(cx, GetProxyTargetObject(wrapper));
>     if (!call.enter())
>         return NULL;
> 
>     JSString *str = DirectWrapper::fun_toString(cx, wrapper, indent);
>     if (!str)
>         return NULL;
> 
>     call.leave();
>     if (!call.origin->wrap(cx, &str))
>         return NULL;
>     return str;
> }
> 
> bool
> CrossCompartmentWrapper::defaultValue(JSContext *cx, JSObject *wrapper, JSType hint, Value *vp)
> {
>-    AutoCompartment call(cx, wrappedObject(wrapper));
>+    AutoCompartment call(cx, GetProxyTargetObject(wrapper));
>     if (!call.enter())
>         return false;
> 
>     if (!IndirectProxyHandler::defaultValue(cx, wrapper, hint, vp))
>         return false;
> 
>     call.leave();
>     return call.origin->wrap(cx, vp);


This whole chunk of changes seems unnecessary to me. Doesn't wrappedObject still work just fine, and give us a useful assertion while we're at it? I can't overstate the importance of keeping patches like this as small as possible. It's massively important for archaeology and regression-finding.

> 
>-    static JSObject *wrappedObject(const JSObject *wrapper);
>-    static AbstractWrapper *wrapperHandler(const JSObject *wrapper);
>+/* Base class that just implements no-op forwarding methods for fundamental
>+ * traps. This is meant to be used as a base class for ProxyHandlers that
>+ * want transparent forwarding behavior but don't want to use the derived
>+ * traps and other baggage of js::Wrapper.

This comment is no longer correct.

>+ */
>+class JS_FRIEND_API(AbstractWrapper) : public Wrapper,
>+                                       public IndirectProxyHandler

This should be IndirectWrapper, in our new parlance. Presumably that's coming in the next patch?

>-    nsIPrincipal *principal = GetCompartmentPrincipal(js::GetObjectCompartment(wrappedObject(wrapper)));
>+    nsIPrincipal *principal = GetCompartmentPrincipal(js::GetObjectCompartment(js::GetProxyTargetObject(wrapper)));

This seems unnecessary as well.


r=bholley with those comments addressed.
Attachment #628256 - Flags: review?(bobbyholley+bmo) → review+
Forgot to refresh the patch after resolving some post-pull conflicts. Here's the followup that fixes those:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f264782f5e
I put [leave open] in the whiteboard for a reason :-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 94 was merged to m-c earlier today:
  https://hg.mozilla.org/mozilla-central/rev/5deea00d55bd
(It got noted on the wrong bug, bug 70357, because the commit message had the wrong bug # :))
(In reply to Daniel Holbert [:dholbert] from comment #95)
> Comment 94 was merged to m-c earlier today:
>   https://hg.mozilla.org/mozilla-central/rev/5deea00d55bd
> (It got noted on the wrong bug, bug 70357, because the commit message had
> the wrong bug # :))

Oops. Sorry about that!
Depends on: 769377
This patch rebases DirectWrapper to inherit from DirectProxyHandler instead of AbstractWrapper. Since both DirectWrapper and DirectProxyHandler need to forward their derived traps to the target object, this allows us to avoid some code duplication.

A propos, you are correct that AbstractWrapper should be renamed to IndirectWrapper. This will happen in the next patch. With that, the refactor of the proxy hierarchy should be complete and we can start landing direct proxies proper!
Attachment #637659 - Flags: review?(bobbyholley+bmo)
(In reply to Eddy Bruel [:ejpbruel] from comment #97)
> A propos, you are correct that AbstractWrapper should be renamed to
> IndirectWrapper. This will happen in the next patch.

In that case, can you just incorporate bug 769377 (flipping the AbstractWrapper init list ordering to fix a build warning) into that next patch?  That's probably simpler than having patches in this bug layer on top of that bug's patch, which itself layers on this bug's prior patch.
(In reply to Daniel Holbert [:dholbert] from comment #98)
> (In reply to Eddy Bruel [:ejpbruel] from comment #97)
> > A propos, you are correct that AbstractWrapper should be renamed to
> > IndirectWrapper. This will happen in the next patch.
> 
> In that case, can you just incorporate bug 769377 (flipping the
> AbstractWrapper init list ordering to fix a build warning) into that next
> patch?  That's probably simpler than having patches in this bug layer on top
> of that bug's patch, which itself layers on this bug's prior patch.

Sure thing. I was already planning on doing so after reading your comment.
Attachment #637659 - Attachment is patch: true
Comment on attachment 637659 [details] [diff] [review]
Part 5 - Inherit DirectWrapper from DirectProxyHandler

This is all going to need a big comment in the final patch explaining how the hierarchy works.
Attachment #637659 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Bobby Holley (:bholley) from comment #100)
> Comment on attachment 637659 [details] [diff] [review]
> Part 5 - Inherit DirectWrapper from DirectProxyHandler
> 
> This is all going to need a big comment in the final patch explaining how
> the hierarchy works.

Thanks for the quick review. You rock!
Are the comments clear enough like this?
Attachment #637944 - Flags: review?
Attachment #637944 - Flags: review? → review?(bobbyholley+bmo)
Comment on attachment 637944 [details] [diff] [review]
Part 6 - Rename AbstractWrapper to IndirectWrapper + adding comments

>diff -r 53e0f8e066f4 js/src/jsproxy.h
>--- a/js/src/jsproxy.h	Fri Jun 29 16:43:16 2012 +0200
>+++ b/js/src/jsproxy.h	Fri Jun 29 20:06:41 2012 +0200
>@@ -10,17 +10,44 @@
> 
> #include "jsapi.h"
> #include "jsfriendapi.h"
> 
> namespace js {
> 
> class Wrapper;
> 
>-/* Base class for all C++ proxy handlers. */
>+/*
>+ * A proxy is a JSObject that forwards its traps to another object, known as the
>+ * target. In general, the target can be any C++ object, not just another
>+ * JSObject. For this reason many different kinds of proxies exist, each of
>+ * which forwards to a different kind of target.

I understand your desire to frame things in terms of "everything has a target, some targets are just more concrete than others". But I don't think it accurately reflects the way that the code works. Proxies allow implementing objects with generic behavior. _Usually_, that behavior involves some kind of forwarding. But it doesn't always. For example, DeadObjectProxy doesn't really forward to anything. Moreover, referring to the DOM binding proxies as "forwarding" to the C++ object is more confusing than clarifying IMO - the proxies implement all kinds of custom behavior.

So I'd prefer if we were just straightforward and said "BaseProxyHandler has no notion of a target - implement whatever custom behavior you wish".

>+ *
>+ * The way in which a proxy forwards its traps to the target depends on the
>+ * particular kind of target it supports. To allow a proxy's forwarding behavior
>+ * to be customized, each proxy contains a proxy handler, which is a C++ object
>+ * containing the actual forwarding code for each trap.
>+ *
>+ * To minimize code duplication, a set of abstract proxy handler classes is
>+ * provided, from which other proxy handlers may inherit. These abstract classes
>+ * are organized in the following hierarchy:
>+ *
>+ * BaseProxyHandler
>+ * |
>+ * IndirectProxyHandler
>+ * |                    
>+ * DirectProxyHandler
>+ */
>+
>+/*
>+ * BaseProxyHandler is the most generic kind of proxy handler. It does not make
>+ * any assumptions about the nature of its target. This makes BaseProxyHandler
>+ * the appropriate class to inherit from if your target is a general C++ object
>+ * (as is the case for DOM bindings).
>+ */

I think it's important to note here the contract of BaseProxyHandler here: You must implement the fundamental traps, and the derived traps are implemented in terms of the fundamental ones (but you can of course override them if you wish).


>-    virtual bool getOwnPropertyNames(JSContext *cx, JSObject *proxy, AutoIdVector &props) = 0;
>+    virtual bool getOwnPropertyNames(JSContext *cx, JSObject *proxy,
>+                                     AutoIdVector &props) = 0;

Nit - sprinkling in whitespace changes like this makes patches more of a pain to review. It's nicer to keep them as a separate patch, where you can say "this patch just changes whitespace" so that the reviewer knows there's no need to check it carefully. :-)

> 
>+/*
>+ * IndirectProxyHandler assumes that the target is a JSObject. Moreover, it
>+ * distinguishes between fundamental traps and derived traps

This distinction exists in BaseProxyHandler as well, no?

> Fundamental traps
>+ * are forwarded directly to the target, whereas derived traps are defined in
>+ * terms of fundamental traps. This is useful if you're creating a security
>+ * wrapper that restricts access to certain traps, and you only want to think in
>+ * terms of fundamental traps.

Except that our security wrappers are actually Direct. :-) I think we should just say "This allows consumers to define custom behavior without implementing the entire gamut of proxy traps."

>+ */
> class JS_PUBLIC_API(IndirectProxyHandler) : public BaseProxyHandler {
>   public:
>     explicit IndirectProxyHandler(void *family);
> 
>     /* ES5 Harmony fundamental proxy traps. */
>     virtual bool getPropertyDescriptor(JSContext *cx, JSObject *proxy, jsid id,
>                                        bool set,
>                                        PropertyDescriptor *desc) MOZ_OVERRIDE;
>@@ -96,18 +136,16 @@ class JS_PUBLIC_API(IndirectProxyHandler
>                                 PropertyDescriptor *desc) MOZ_OVERRIDE;
>     virtual bool getOwnPropertyNames(JSContext *cx, JSObject *proxy,
>                                      AutoIdVector &props) MOZ_OVERRIDE;
>     virtual bool delete_(JSContext *cx, JSObject *proxy, jsid id,
>                          bool *bp) MOZ_OVERRIDE;
>     virtual bool enumerate(JSContext *cx, JSObject *proxy,
>                            AutoIdVector &props) MOZ_OVERRIDE;
> 
>-    /* Derived traps are implemented in terms of fundamental traps */
>-
>     /* Spidermonkey extensions. */
>     virtual bool call(JSContext *cx, JSObject *proxy, unsigned argc,
>                       Value *vp) MOZ_OVERRIDE;
>     virtual bool construct(JSContext *cx, JSObject *proxy, unsigned argc,
>                            Value *argv, Value *rval) MOZ_OVERRIDE;
>     virtual bool nativeCall(JSContext *cx, JSObject *proxy, Class *clasp,
>                             Native native, CallArgs args) MOZ_OVERRIDE;
>     virtual bool hasInstance(JSContext *cx, JSObject *proxy, const Value *vp,
>@@ -121,16 +159,22 @@ class JS_PUBLIC_API(IndirectProxyHandler
>     virtual bool regexp_toShared(JSContext *cx, JSObject *proxy,
>                                  RegExpGuard *g) MOZ_OVERRIDE;
>     virtual bool defaultValue(JSContext *cx, JSObject *obj, JSType hint,
>                               Value *vp) MOZ_OVERRIDE;
>     virtual bool iteratorNext(JSContext *cx, JSObject *proxy,
>                               Value *vp) MOZ_OVERRIDE;
> };
> 
>+/*
>+ * DirectProxyHandler overrides the derived traps of IndirectProxyHandler so
>+ * that they are forwarded directly to the target as well. This makes sense if
>+ * you're creating a wrapper that should behave as transparently as possible,
>+ * forwarding all traps directly to the target (such as CrossCompartmentWrapper)

needs a period.

Looks good otherwise. r- just because I want to look at the updated comments. But it should be fine. :-)
Attachment #637944 - Flags: review?(bobbyholley+bmo) → review-
Confused -- this landed after an r-?

Dave
Part 5 is what landed, no?
(In reply to Dave Herman [:dherman] from comment #106)
> Confused -- this landed after an r-?
> 
> Dave

Ryan is correct. Part 5 landed. Part 6 is still pending r+
My bad, shutting up now. :)

Dave
Also, part 6 breaks a browser build.
Blocks: 770140
Attachment #637944 - Attachment is obsolete: true
Attachment #638762 - Flags: review?(bobbyholley+bmo)
Comment on attachment 638762 [details] [diff] [review]
Part 6 - Rename AbstractWrapper to IndirectWrapper + adding comments


>+ * A proxy is a JSObject that implements generic behavior by providing custom
>+ * implementations for each object trap. The implementation for each trap is
>+ * provided by a C++ object stored on the proxy, known as its handler.
>+ *
>+ * A major use case for proxies is to forward each trap to another object,
>+ * known as its target. The target can be an arbitrary C++ object. Not every
>+ * proxy has the notion of a target, however.

Referring to things like NodeLists as "forwarding to a target", as well as the concept to "forwarding to an arbitrary c++ object", doesn't really make sense. "Forwarding" only makes sense for JS Objects IMO. Please remove that part.
> 
>+/*
>+ * IndirectProxyHandler assumes that a target exists. Moreover, it assumes the
>+ * target is a JSObject.

This part should be changed per the above comment.

Looks great otherwise! r=bholley with those fixes.
Attachment #638762 - Flags: review?(bobbyholley+bmo) → review+
Had some issues with my repo, so I hope I didn't mess this one up:
http://hg.mozilla.org/integration/mozilla-inbound/rev/20b0bce4c165
This bug, or something which was merged to mozilla-central in the same push, caused a permanent orange on Linux32 mochitest-chrome for builds without frame pointers (example log: <https://tbpl.mozilla.org/php/getParsedLog.php?id=13227357&tree=Profiling&full=1>).  This is important because framepointers are not enabled on Aurora, so this permaorange will appear there on the next uplift.  I backed out this patch as part of the rest of the js and xpconnect patches in the same merge push.  Please test this patch locally (and push to the try server if needed by taking out the --enable-profiling command from the Linux32 mozconfig) and reland when you make sure that it's not the cause behind the perma-orange.  I apologize in advance for the inconvenience in case this patch is not at fault.

Backout changeset:
https://hg.mozilla.org/mozilla-central/rev/bdf2bb0c66fd
In order to test whether this patch is at fault, push it to try with --enable-profiling removed from browser/config/mozconfigs/linux32/nightly, and if you get a green Linux Moth run, you're good to go!

And in turn you can reproduce the crash on try with your patch, I'd suggest you keep the possibility of having hit a compiler bug in mind.  :-)
Also, note that these backouts indeed made the Linux32 Moth runs on the profiling branch green again, so we can be fairly certain that one of the backed out patches was at fault.
Currently, IndirectProxyHandler stores the target object in its private slot. ScriptedProxyHandler, which inherits from IndirectProxyHandler, treats this target object as its handler object. In effect, for scripted proxies the target and the handler object are the same thing, so that if the handler object does not explicitly override a trap, the trap is forwarded to the object directly. This is messy, but it allowed us to inherit ScriptedProxyHandler from IndirectProxyHandler without breaking any existing code.

For direct proxies, the target and the handler object will have to become two separate objects. The slot in which to store the target is already fixed by IndirectProxyHandler. The obvious place to store the handler is therefore the first extra slot of a scripted proxy. This patch makes a first step towards that by making sure that the target/handler is stored in both the private slot and the extra slot. Note that they are still the same object at this point, but that will change as soon as the new proxy constructors land.
Attachment #639371 - Flags: review?(jorendorff)
By the way, I was thinking: maybe it is a good idea to keep ScriptedProxyHandler in its current form, and introduce a new class DirectScriptedProxyhandler for direct proxies. This way, we should be able to introduce the new direct proxies syntax without removing support for the existing scripted proxies (sans fix trap, but nobody was using that anyway). Deprecating and then slowly phasing the old proxies out over time would be preferable over throwing them away without warning, imho.
Comment on attachment 639371 [details] [diff] [review]
Part 7 - Store handler object in extra slot for scripted proxies

Canceled this in light of our decision to add a new class ScriptedDirectProxyhandler for direct proxies.
Attachment #639371 - Flags: review?(jorendorff)
This patch renames the class ScriptedProxyHandler to IndirectScriptedProxyHandler, introduces a new class, ScriptedDirectProxyHandler, and implements the trap getOwnPropertyDescriptor on it (together with some auxiliary functions that are necessary for its implementation).

The implementation of the trap and the auxiliary functions is pretty much the same as it was in my original patch, barring some minor changes to make everything compile. It's possible that there have been subtle changes in semantics in the meantime though, so this patch warrants close examination.

Luckily, it's quite heavily commented, so the intended behavior should be pretty clear :-)
Attachment #639371 - Attachment is obsolete: true
Attachment #640061 - Flags: review?(jorendorff)
By the way, it looks to me like our custom implementation of GetOwnPropertyDescriptor in jsproxy.cpp is superfluous. This function already exists in jsobj.cpp, and afaict has the same semantics. I can file a followup patch that fixes this.
(In reply to Ehsan Akhgari [:ehsan] from comment #116)
> In order to test whether this patch is at fault, push it to try with
> --enable-profiling removed from browser/config/mozconfigs/linux32/nightly,
> and if you get a green Linux Moth run, you're good to go!
> 
> And in turn you can reproduce the crash on try with your patch, I'd suggest
> you keep the possibility of having hit a compiler bug in mind.  :-)

Patch looks good to me. Here's the corresponding try run:
https://tbpl.mozilla.org/?tree=Try&rev=6c6ee7f7ecb8
(In reply to Eddy Bruel [:ejpbruel] from comment #123)
> (In reply to Ehsan Akhgari [:ehsan] from comment #116)
> > In order to test whether this patch is at fault, push it to try with
> > --enable-profiling removed from browser/config/mozconfigs/linux32/nightly,
> > and if you get a green Linux Moth run, you're good to go!
> > 
> > And in turn you can reproduce the crash on try with your patch, I'd suggest
> > you keep the possibility of having hit a compiler bug in mind.  :-)
> 
> Patch looks good to me. Here's the corresponding try run:
> https://tbpl.mozilla.org/?tree=Try&rev=6c6ee7f7ecb8

Um, you didn't need to include that change when pushing to inbound...
(In reply to :Ms2ger from comment #125)
> (In reply to Eddy Bruel [:ejpbruel] from comment #123)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #116)
> > > In order to test whether this patch is at fault, push it to try with
> > > --enable-profiling removed from browser/config/mozconfigs/linux32/nightly,
> > > and if you get a green Linux Moth run, you're good to go!
> > > 
> > > And in turn you can reproduce the crash on try with your patch, I'd suggest
> > > you keep the possibility of having hit a compiler bug in mind.  :-)
> > 
> > Patch looks good to me. Here's the corresponding try run:
> > https://tbpl.mozilla.org/?tree=Try&rev=6c6ee7f7ecb8
> 
> Um, you didn't need to include that change when pushing to inbound...

D'oh!
Rebased the patch on a recent pull and made sure it builds so jorendorff can review. Haven't double checked yet. Hope everything still checks out.
Attachment #640061 - Attachment is obsolete: true
Attachment #640061 - Flags: review?(jorendorff)
Attachment #644517 - Flags: review?(jorendorff)
Attachment #644517 - Attachment is patch: true
Comment on attachment 644517 [details] [diff] [review]
Part 7 - Implement getOwnPropertyDescriptorTrap

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

The general approach looks good, but there are some bugs, and I think we can get a lot closer to what the spec says regarding descriptor objects. (Much more about that below.)

::: js/src/js.msg
@@ +351,5 @@
>  MSG_DEF(JSMSG_FUNCTION_ARGUMENTS_AND_REST, 298, 0, JSEXN_ERR, "the 'arguments' property of a function with a rest parameter may not be used")
>  MSG_DEF(JSMSG_REST_WITH_DEFAULT,      299, 0, JSEXN_SYNTAXERR, "rest parameter may not have a default")
>  MSG_DEF(JSMSG_NONDEFAULT_FORMAL_AFTER_DEFAULT, 300, 0, JSEXN_SYNTAXERR, "parameter(s) with default followed by parameter without default")
>  MSG_DEF(JSMSG_YIELD_IN_DEFAULT,       301, 0, JSEXN_SYNTAXERR, "yield in default expression")
> +MSG_DEF(JSMSG_CANT_REPORT_NC_AS_NE,   302, 0, JSEXN_TYPEERR, "can't report a non-configurable own property as non-existent")

I think adding the word "proxy" at the beginning of each of these error messages would help people figure out what's going on.

I understand these messages come right out of the spec, but they're bound to be confusing. These errors are thrown at kind of a weird time, since the code that messed up (the proxy handler trap) is no longer on the stack at the time of the error -- so the url/line number information will blame the victim.

::: js/src/jsproxy.cpp
@@ +988,5 @@
>  }
>  
>  bool
> +ScriptedIndirectProxyHandler::nativeCall(JSContext *cx, IsAcceptableThis test,
> +                                         NativeImpl impl, CallArgs args)

Nit: While you're in here, put all this on one line if it'll fit. Lines go to 99 columns in js/src.

@@ +1007,5 @@
>  }
>  
>  bool
> +ScriptedIndirectProxyHandler::defaultValue(JSContext *cx, JSObject *proxy,
> +                                           JSType hint, Value *vp)

Ditto.

@@ +1018,5 @@
>  }
>  
> +ScriptedIndirectProxyHandler ScriptedIndirectProxyHandler::singleton;
> +
> +class ScriptedDirectProxyHandler : public DirectProxyHandler {

If you are seized with the desire to put all the scriptable proxy stuff in a separate C++ file, do it! Perhaps js/src/builtin/Proxy.{h,cpp}.

@@ +1050,5 @@
> + *
> + * In particular, the specification assumes that properties are represented as
> + * JavaScript objects, which allows for the possibility of missing fields. To
> + * keep track of missing fields, PropDesc contains several bit flags, which are
> + * absent in PropertyDescriptor.

All this is basically correct. In particular the fact that BaseProxyHandler::defineProperty takes a PropertyDescriptor argument rather than a PropDesc leads to some bugs that you can observe using Object.defineProperty.

However there is some sense in using a different type (not PropDesc) for descriptors of actual properties that exist. Certain combinations are impossible in actual properties; for example, a properties can't have a [[Value]] field but no [[Writable]] field. PropertyDescriptor is not so bad for something like that. It can't represent generic descriptors. But that's a feature: objects can't have "generic properties" in ES5; all properties are either data or accessor.

I think the spec will end up calling the latter "complete" property descriptors.

@@ +1052,5 @@
> + * JavaScript objects, which allows for the possibility of missing fields. To
> + * keep track of missing fields, PropDesc contains several bit flags, which are
> + * absent in PropertyDescriptor.
> + *
> + * 

Delete both of these lines with nothing but * on them.

@@ +1063,5 @@
> +    if (!desc->obj) 
> +        return false;
> +
> +    /*
> +     * 2. If both Desc.[[Value]] and Desc.[[Set]] are absent, then return false.

[[Writable]], not [[Set]].

I don't think it helps to quote every line of the spec. Here's what I would write:

/* ES5 8.10.1 */
static inline bool
IsAccessorDescriptor(const PropertyDescriptor *desc)
{
    return desc->obj && (desc->attrs & (JSPROP_GETTER | JSPROP_SETTER));
}

/*
 * ES5 8.10.2
 * If desc->obj is NULL, we treat it as "undefined" and return false;
 * otherwise it is a data descriptor iff it isn't an accessor descriptor.
 */
static inline bool
IsDataDescriptor(const PropertyDescriptor *desc)
{
    return desc->obj && !(desc->attrs & (JSPROP_GETTER | JSPROP_SETTER));
}

/*
 * ES5 8.10.3
 * PropertyDescriptor can't represent generic descriptors.
 */
static inline bool
IsGenericDescriptor(const PropertyDescriptor *desc)
{
    return false;
}

@@ +1108,5 @@
> +     */
> +
> +    /*
> +     * Due to the way we've devined IsDataDescriptor (see above), this test
> +     * always fails, and IsGenericDescriptor always returns true.

I think the test always passes, so this returns false.

@@ +1120,5 @@
> +
> +/* Aux 5. ValidateProperty(O, P, Desc) */
> +static inline bool
> +ValidateProperty(JSContext *cx, HandleObject proxy, jsid id,
> +                 PropertyDescriptor *desc, bool *bp)

My reading of the spec is that the 'desc' argument should be a JSObject* rather than a PropertyDescriptor*. It is kind of hard to tell, but

  - TrapGetOwnProperty calls it, passing desc, which is the result of a call to
    NormalizeAndCompletePropertyDescriptor, which returns an Object;

  - TrapDefineOwnProperty calls it, passing normalizedDesc, which is the result of
    a call to NormalizePropertyDescriptor, which returns an Object;

  - there aren't any other callers.

But if so, I think the spec ought to be rewritten in terms of [[Get]] calls.  It might be a good idea to ask the spec author about this (Tom Van Cutsem).

I didn't review ValidateProperty closely because of this.

@@ +1124,5 @@
> +                 PropertyDescriptor *desc, bool *bp)
> +{
> +    /*
> +     * 1. Let current be the result of calling the [[GetOwnProperty]] internal
> +     *    method of O with property name P.

I don't so much mind quoting chapter and verse here since this stuff is so long, complicated, and generally inscrutable to the uninitiated. If you like you could save a lot of lines by using // comments throughout the body of the function, rather than /**/. Optional but recommended!

@@ +1275,5 @@
> +        return true;
> +    }
> +
> +    /* 3. Return the boolean negation of desc.[[Configurable]] */
> +    *bp = desc.attrs & JSPROP_PERMANENT;

Again, quoting every step of the proposed standard obscures the meaning rather than illuminating.  It is enough to make clear at the top that this is an implementation of a specific section of the spec.

As above, removing a few comments leads to dramatically shorter code:

    *bp = desc.obj && (desc.attrs & JSPROP_PERMANENT);
    return true;

@@ +1319,5 @@
> +         * a. Return the result of calling the built-in function
> +         *    Reflect.getOwnPropertyDescriptor(target, P)
> +         */
> +        return DirectProxyHandler::getOwnPropertyDescriptor(cx, proxy, id, set,
> +                                                            desc);

Nit: This probably fits on one line.

@@ +1343,5 @@
> +
> +    /*
> +     * Due to how PropertyDescriptors are represented, we can neither normalize
> +     * or complete them, so we are forced to simply convert trapResult to a
> +     * PropertyDescriptor.

I don't understand. Why not implement NormalizeAndCompletePropertyDescriptor as specified in Aux.4, and ToCompletePropertyDescriptor as specified in Aux.1 -- operating on actual objects?  After all, rval is an actual JS value here, not a PropertyDescriptor.

Then convert the object back to a PropertyDescriptor in step 3 of [[GetOwnProperty]] (which happens after TrapGetOwnProperty returns).

I think this is the main problem with the patch as it stands.

Suggest actually writing a helper function called TrapGetOwnProperty and calling from here -- and also from Object.getOwnPropertyDescriptor, just like the spec says.

@@ +1359,5 @@
> +        if (!IsSealed(cx, target, id, &sealed)) 
> +            return false;
> +        if (sealed) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                                 JSMSG_CANT_REPORT_NC_AS_NE);

Style nit: one line again (also in other similar places).

@@ +1370,5 @@
> +         */
> +        if (!target->isExtensible()) {
> +            bool found;
> +            if (!getOwnPropertyDescriptor(cx, proxy, id, false, desc))
> +                return false;

I think this is operating on the wrong object (proxy; should be target). Also, doesn't this overwrite *desc?

@@ +1373,5 @@
> +            if (!getOwnPropertyDescriptor(cx, proxy, id, false, desc))
> +                return false;
> +            if (!desc->obj)
> +                return false;
> +            if (found) {

You pointed out that this doesn't make much sense as written and it should just say,

    if (desc->obj) {

A test should be added for each bug you fix in here -- this stuff is tricky to get right and those tests will be valuable in maintaining it.

@@ +1386,5 @@
> +        }
> +    }
> +    
> +    /* 8. Let isFixed be the result of calling target.[[HasOwn]].P */
> +    bool found;

Consider calling this isFixed to match the spec. Future people reading this code will thank you.

@@ +1388,5 @@
> +    
> +    /* 8. Let isFixed be the result of calling target.[[HasOwn]].P */
> +    bool found;
> +    if (!getOwnPropertyDescriptor(cx, proxy, id, false, desc))
> +        return false;

Again, I think this should be operating on target, not proxy, and it should avoid overwriting *desc.

@@ +1427,5 @@
> +        }
> +    }
> +
> +    /* 11. If desc.[[Configurable]] is false and isFixed is false, */
> +    if (desc->attrs & JSPROP_PERMANENT && !found) {

Style nit: Overparenthesize please:

    if ((desc->attrs & JSPROP_PERMANENT) && !found) {
Attachment #644517 - Flags: review?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #129)
> @@ +1120,5 @@
> > +
> > +/* Aux 5. ValidateProperty(O, P, Desc) */
> > +static inline bool
> > +ValidateProperty(JSContext *cx, HandleObject proxy, jsid id,
> > +                 PropertyDescriptor *desc, bool *bp)
> 
> My reading of the spec is that the 'desc' argument should be a JSObject*
> rather than a PropertyDescriptor*. It is kind of hard to tell, but
> 
>   - TrapGetOwnProperty calls it, passing desc, which is the result of a call
> to
>     NormalizeAndCompletePropertyDescriptor, which returns an Object;
> 
>   - TrapDefineOwnProperty calls it, passing normalizedDesc, which is the
> result of
>     a call to NormalizePropertyDescriptor, which returns an Object;
> 
>   - there aren't any other callers.
> 
> But if so, I think the spec ought to be rewritten in terms of [[Get]] calls.
> It might be a good idea to ask the spec author about this (Tom Van Cutsem).
> 
> I didn't review ValidateProperty closely because of this.

You're right, there was an inconsistency in the spec: ValidateProperty did expect an internal property descriptor, and callers were providing a JSObject. I fixed the spec by having the callers pass ToPropertyDescriptor(descObject) instead of descObject directly. I chose this solution in order not to further complicate the spec for ValidateProperty. In principle, even if we would turn the descriptor argument into a JSObject, we can count on the fact that it's been properly normalized. Still, ValidateProperty is currently a direct transliteration of [[DefineOwnProperty]], and I thought it a good idea to keep it that way. I'm hopeful that an implementation doesn't actually need to allocate a temporary internal descriptor just to call ValidateProperty.
Attached patch Part 7 - Dead code removal (obsolete) — Splinter Review
Given the large amount of comments from jorendorff, I'm breaking the previous patch up into a series of smaller ones and will try to get r+ for those individually.

This is the first patch in that series. All it does is some dead code removal. Afaict, the AutoPendingProxyOperation struct was used by the now-removed fix trap to ensure that fix would not be called from within another pending proxy operation. It's now no longer necessary.
Attachment #644517 - Attachment is obsolete: true
Attachment #648498 - Flags: review?(jorendorff)
This name change is needed because we want to keep ScriptedProxyHandler around, so we can gracefully deprecate it, and we want to avoid name conflicts with the new ScriptedDirectProxyHandlers.
Attachment #648499 - Flags: review?(jorendorff)
Attachment #648500 - Flags: review?(jorendorff)
Attached patch Part 10 - Add Proxy constructor (obsolete) — Splinter Review
This patch adds a new proxy constructor. Note that for compatibility reasons, this will still create the old scripted proxies:
Proxy.create(...)
Proxy.createFunction(...)

Whilst this will now create direct proxies (as per spec);
new Proxy(...)
The remaining parts of the original patch require some more work. Hopefully I can land those tomorrow.
Comment on attachment 648498 [details] [diff] [review]
Part 7 - Dead code removal

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

Also remove pendingProxyOperation from JSRuntime and struct js::PendingProxyOperation from jscntxt.h. Right? (Maybe it's in a later patch; I haven't looked yet...)

::: js/src/jsproxy.cpp
@@ -791,3 @@
>  
>      /* Spidermonkey extensions. */
> -    virtual bool nativeCall(JSContext *cx, IsAcceptableThis test, NativeImpl impl, CallArgs args) MOZ_OVERRIDE;

This is the only line here that is over the 99 column limit. Please revert all the other whitespace changes here.

Retain MOZ_OVERRIDE. Adding MOZ_OVERRIDE to all the other methods (they are in fact overrides) is optional.
Attachment #648498 - Flags: review?(jorendorff) → review+
Attachment #648499 - Flags: review?(jorendorff) → review+
Comment on attachment 648500 [details] [diff] [review]
Part 9 - Add stub implementation of ScriptedDirectProxyHandler

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

So far so good, but is there a reason to check this in yet? I'd just as soon leave it in your mq stack for now.

Style nit: Please fill up to 99 columns.

It'd be great to move all the "scripted proxy" implementation into builtin/Proxy.{h,cpp}, leaving the core infrastructure in {jsproxy,jswrapper}.{h,cpp}. Please do it!
Attachment #648500 - Flags: review?(jorendorff) → feedback+
Comment on attachment 648516 [details] [diff] [review]
Part 10 - Add Proxy constructor

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

::: js/src/jsproxy.cpp
@@ +1786,1 @@
>      NULL,                    /* construct   */

lol

@@ +1944,5 @@
>      return obj;
>  }
>  
>  static JSBool
> +proxy(JSContext *cx, unsigned argc, jsval *vp)

Prevailing style suggests "Proxy" here, though it doesn't matter much.

@@ +1951,5 @@
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
> +                             "Proxy", "0", "s");
> +        return false;
> +    }
> +    JSObject *target = NonNullObject(cx, vp[2]);

Use CallArgs! It's nice!

@@ +1963,5 @@
> +                                     ObjectValue(*target), proto, proto->getParent(),
> +                                     target, target);
> +    if (!proxy)
> +        return false;
> +    SetProxyExtra(proxy, 0, ObjectOrNullValue(handler)),     

Nit: trailing whitespace on the last line quoted here.
Attachment #648516 - Flags: feedback+
(In reply to Jason Orendorff [:jorendorff] from comment #137)
> Comment on attachment 648500 [details] [diff] [review]
> Part 9 - Add stub implementation of ScriptedDirectProxyHandler
> 
> Review of attachment 648500 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So far so good, but is there a reason to check this in yet? I'd just as soon
> leave it in your mq stack for now.
> 
> Style nit: Please fill up to 99 columns.
> 
> It'd be great to move all the "scripted proxy" implementation into
> builtin/Proxy.{h,cpp}, leaving the core infrastructure in
> {jsproxy,jswrapper}.{h,cpp}. Please do it!

The proxy code is still in flux. Landing these patches early ensures that other people working on the proxy code have to make sure their patches don't break mine (the reverse is also true). In contrast, I see no advantage to *not* landing these patches early. Direct proxies don't affect any existing code, so why not?

If you feel strongly about moving scripted proxies to built-in, thats fine by me. I will do so in a subsequent patch.
(In reply to Jason Orendorff [:jorendorff] from comment #138)
> Comment on attachment 648516 [details] [diff] [review]
>
> >  static JSBool
> > +proxy(JSContext *cx, unsigned argc, jsval *vp)
> 
> Prevailing style suggests "Proxy" here, though it doesn't matter much.
>

I tried that, but "Proxy" is already taken by the Proxy class itself.
(In reply to Jason Orendorff [:jorendorff] from comment #129)
> Comment on attachment 644517 [details] [diff] [review]
> Part 7 - Implement getOwnPropertyDescriptorTrap
> 
> Review of attachment 644517 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +1343,5 @@
> > +
> > +    /*
> > +     * Due to how PropertyDescriptors are represented, we can neither normalize
> > +     * or complete them, so we are forced to simply convert trapResult to a
> > +     * PropertyDescriptor.
> 
> I don't understand. Why not implement NormalizeAndCompletePropertyDescriptor
> as specified in Aux.4, and ToCompletePropertyDescriptor as specified in
> Aux.1 -- operating on actual objects?  After all, rval is an actual JS value
> here, not a PropertyDescriptor.
> 
> Then convert the object back to a PropertyDescriptor in step 3 of
> [[GetOwnProperty]] (which happens after TrapGetOwnProperty returns).
> 
> I think this is the main problem with the patch as it stands.
> 
> Suggest actually writing a helper function called TrapGetOwnProperty and
> calling from here -- and also from Object.getOwnPropertyDescriptor, just
> like the spec says.
> 
The proxy code uses PropertyDescriptor to represent internal property descriptors. PropertyDescriptor, unlike PropDesc, cannot represent absent fields.

According to the proposed spec, ToCompletePropertyDescriptor takes a JSObject, and converts it to a PropertyDescriptor by calling ToPropertyDescriptor (PropDesc::initialize). After that, it is supposed to fill in default values for absent data or accessor fields, depending on the type of the descriptor.

However, since PropertyDescriptor cannot represent absent fields, we are forced to fill in default values for all absent fields anyway, regardless of the type of the descriptor. This is exactly what ParsePropertyDescriptor seems to do already, so as far as ToCompletePropertyDescriptor is concerned, I still think my comment was correct (maybe we should just rename ParsePropertyDescriptor?)

As far as NormalizedAndCompletePropertyDescriptor is concerned, however, I think you are right, since it converts the descriptor back to a JSObject before step 5. I don't think I read that correctly.
After thinking about it some more, I came up with the following implementations for auxiliary functions 1-4. Following your suggestion, they now operate on JSObjects, rather than PropertyDescriptors.

It was unclear to me whether the statement: "If the value of an attribute field of desc, considered as a data descriptor, is absent, set it to its default value." includes the enumerable and configurable properties. I assumed it doesn't.

According to the comment on vm/ObjectImpl.h:176, PropDesc::makeObject is used to implement FromPropertyDescriptor as specified in (8.10.4). However, it actually behaves more like FromGenericPropertyDescriptor as specified in (Aux. 2). The former uses the descriptor type to decide which properties to set on the object, whereas the latter decides this on a per attribute basis.

Assuming the above observation is correct, I've implemented FromGenericPropertyDescriptor in terms of PropDesc::makeObject, and replaced any reference in the spec to ToPropertyDescriptor with FromGenericPropertyDescriptor (since under the current implementation, their semantics are equivalent). Please let me know if you agree with this approach.

Note that NormalizeProperty and NormalizeAndCompleteProperty share the same implementation. This saves some code duplication, but it might be less clear what's going on to the reader. Would like your opinion on this.
Attachment #649129 - Flags: review?(jorendorff)
By the way, jorendorff, does feedback+ imply positive review? Or just positive feedback? :-P

I've addressed your comments for those patches, should I put them up for review again?
(In reply to Eddy Bruel [:ejpbruel] from comment #139)
> If you feel strongly about moving scripted proxies to built-in, thats fine
> by me. I will do so in a subsequent patch.

Nah, if I feel that strongly about it I'll just write the patch myself.
(In reply to Eddy Bruel [:ejpbruel] from comment #143)
> By the way, jorendorff, does feedback+ imply positive review? Or just
> positive feedback? :-P

Sorry I wasn't clear about this. I marked feedback+ on Part 9 and Part 10 because I agree with the approach, but I don't think they should land yet. In particular the stub implementation shouldn't land because (a) it currently asserts with JS_NOT_REACHED, and with the constructor it's easy for scripts to trigger that; (b) it's something we emphatically do not want to ship in that state, since nothing works.

Let's just get everything tested, reviewed, and landed.

> I've addressed your comments for those patches, should I put them up for
> review again?

Yes, definitely get a complete stack posted for review in this bug as soon as you can!
Comment on attachment 649129 [details] [diff] [review]
Part 11 - Auxiliary functions 1-4

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

So far so good.

::: js/src/jsproxy.cpp
@@ +1082,5 @@
> + * that for Aux.4 NormalizeAndCompletePropertyDescriptor (see below). The
> + * argument complete is used to distinguish between the two.
> + */
> +static inline bool
> +NormalizePropertyDescriptor(JSContext *cx, Value *vp, bool complete = false)

This shouldn't be inline, right? In fact I don't think any of them need an explicit 'inline'. Since they're static, the C++ compiler will inline them if it makes sense.

@@ +1092,5 @@
> +    /* Aux.3 steps 1-2 / Aux.4 steps 2-3 */
> +    AutoPropDescArrayRooter descs(cx);
> +    PropDesc *desc = descs.append();
> +    if (!desc || !desc->initialize(cx, *vp))
> +        return false;

Aux.4 step 3 uses ToCompletePropertyDescriptor, which I think means there should be another line or two of code here:

    if (complete)
        desc->complete(cx);

::: js/src/vm/ObjectImpl.h
@@ +169,5 @@
>       * themselves callable.)
>       */
> +    bool initialize(JSContext *cx, const Value &v, bool checkAccessors = true);
> +
> +    void complete(JSContext *cx);

Needs a comment.
Attachment #649129 - Flags: review?(jorendorff) → feedback+
Blocks: 601379
Attached patch Part 7 - Dead code removal (obsolete) — Splinter Review
Attachment #648498 - Attachment is obsolete: true
Attachment #650290 - Flags: review?(jorendorff)
Attachment #648499 - Attachment is obsolete: true
Attachment #650292 - Flags: review?(jorendorff)
Attachment #648500 - Attachment is obsolete: true
Attachment #650293 - Flags: review?(jorendorff)
Attachment #648516 - Attachment is obsolete: true
Attachment #650294 - Flags: review?(jorendorff)
Attachment #649129 - Attachment is obsolete: true
Attachment #650295 - Flags: review?(jorendorff)
Attachment #650296 - Flags: review?(jorendorff)
Attachment #650297 - Flags: review?(jorendorff)
Attachment #650298 - Flags: review?(jorendorff)
Attachment #650299 - Flags: review?(jorendorff)
Attachment #650300 - Flags: review?(jorendorff)
Attached patch Part 17 - Implement delete trap (obsolete) — Splinter Review
Attachment #650302 - Flags: review?(jorendorff)
Attachment #650303 - Flags: review?(jorendorff)
Attached patch Part 19 - Implement has trap (obsolete) — Splinter Review
Attachment #650305 - Flags: review?(jorendorff)
Attached patch Part 20 - Implement hasOwn trap (obsolete) — Splinter Review
Attachment #650306 - Flags: review?(jorendorff)
Attached patch Part 21 - Implement get trap (obsolete) — Splinter Review
Attachment #650307 - Flags: review?(jorendorff)
Attached patch Part 22 - Implement set trap (obsolete) — Splinter Review
Attachment #650327 - Flags: review?(jorendorff)
Attached patch Part 23 - Implement keys trap (obsolete) — Splinter Review
Attachment #650330 - Flags: review?(jorendorff)
Attached patch Part 24 - Implement iterate trap (obsolete) — Splinter Review
Attachment #650331 - Flags: review?(jorendorff)
Comment on attachment 650293 [details] [diff] [review]
Part 9 - Add ScriptedDirectProxyHandler stub

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

::: js/src/jsproxy.cpp
@@ +1011,5 @@
>  
> +static JSObject *
> +GetDirectProxyHandlerObject(JSObject *proxy)
> +{
> +    return GetProxyExtra(proxy, 0).toObjectOrNull();

Is there a way to assert that proxy is the right kind of object?
Comment on attachment 650307 [details] [diff] [review]
Part 21 - Implement get trap

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

::: js/src/jsproxy.cpp
@@ +1962,5 @@
>      *bp = !!success;
>      return true;
>  }
>  
> +/* [[GetP](P, Receiver) */

]]
Comment on attachment 650327 [details] [diff] [review]
Part 22 - Implement set trap

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

::: js/src/jsproxy.cpp
@@ +2070,5 @@
> +
> +    /* step 6 */
> +    JSBool success;
> +    if (!JS_ValueToBoolean(cx, trapResult, &success))
> +        return false;

bool success = ToBoolean(trapResult);
Oh, and please fix all the trailing whitespace
Comment on attachment 650290 [details] [diff] [review]
Part 7 - Dead code removal

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

r=me.

::: js/src/jsproxy.cpp
@@ -1054,5 @@
>  Proxy::getPropertyDescriptor(JSContext *cx, JSObject *proxy, jsid id, bool set,
>                               PropertyDescriptor *desc)
>  {
>      JS_CHECK_RECURSION(cx, return false);
> -    AutoPendingProxyOperation pending(cx, proxy);

AutoPendingProxyOperation gc-roots 'proxy'. The PendingProxyOperation contains a RootedObject.

So instead of removing these please turn them into RootedObjects, unless you're quite sure the caller has rooted them. (Or, feel free to rewrite all this to use handles, if you understand that stuff and have time.)
Attachment #650290 - Flags: review?(jorendorff) → review+
Attachment #650292 - Flags: review?(jorendorff) → review+
Comment on attachment 650293 [details] [diff] [review]
Part 9 - Add ScriptedDirectProxyHandler stub

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

I'll go ahead and do all the work involved in reviewing these, but nothing gets r+ 'til I see some tests.

::: js/src/jsproxy.cpp
@@ +1038,5 @@
> +    virtual bool get(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, Value *vp);
> +    virtual bool set(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, bool strict,
> +                     Value *vp);
> +    virtual bool keys(JSContext *cx, JSObject *proxy, AutoIdVector &props);
> +    virtual bool iterate(JSContext *cx, JSObject *proxy, unsigned flags, Value *vp);

Feel free to add MOZ_OVERRIDE here for bonus warm fuzzies.
Attachment #650293 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650294 [details] [diff] [review]
Part 10 - Add direct proxy constructor

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

::: js/src/jsproxy.cpp
@@ +1911,5 @@
>      return obj;
>  }
>  
>  static JSBool
> +proxy(JSContext *cx, unsigned argc, jsval *vp)

OK, if Proxy is taken, consider Proxy_construct or Proxy::construct.

@@ +1924,5 @@
> +    if (!target)
> +        return false;
> +    JSObject *handler = NULL;
> +    if (args.length() > 1)
> +        handler = &args[2].toObject();

It's not OK to use .toObject() without first checking that .isObject(); also this should be args[1] not args[2].

However, it looks like the spec has changed so that a handler object is now required:

http://wiki.ecmascript.org/doku.php?id=harmony:proxies_spec#proxy_constructor_function
> If Type(T) is not Object, throw a TypeError exception
> If Type(H) is not Object, throw a TypeError exception

So you can require at least 2 arguments above. Don't forget to change the error message from ("Proxy", "0", "s") to ("Proxy", "1", "").
Attachment #650294 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650295 [details] [diff] [review]
Part 11 - Implement auxiliary functions 1-4

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

Yup, looks about right. A few bugs.

::: js/src/jsobj.cpp
@@ +1400,5 @@
> +        }
> +    }
> +    if (!hasEnumerable_) {
> +        hasEnumerable_ = true;
> +        attrs |= JSPROP_ENUMERATE;

The default value for [[Enumerable]] is false, so "&= ~" rather than "|=" here.

@@ +1404,5 @@
> +        attrs |= JSPROP_ENUMERATE;
> +    }
> +    if (!hasConfigurable_) {
> +        hasConfigurable_ = true;
> +        attrs &= ~JSPROP_PERMANENT;

The default value for [[Configurable]] is false, and JSPROP_PERMANENT is the opposite of configurable, so "|=" here.

The default values of attributes are defined in 8.6.1, table 7:
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-8.6.1

::: js/src/jsproxy.cpp
@@ +1058,5 @@
> +
> +    /* Aux.2 steps 3-9 */
> +    if (!desc->makeObject(cx))
> +        return false;
> +    *rval.address() = desc->pd();

I think you're meant to write:  rval.set(desc->pd());

Does that not work for Values?  I think it should.

@@ +1073,5 @@
> +static bool
> +NormalizePropertyDescriptor(JSContext *cx, MutableHandleValue vp, bool complete = false)
> +{
> +    /* Aux.4 step 1 */
> +    if (complete && vp.isUndefined())

You can't write vp.isUndefined(), can you? Should be vp->isUndefined(), right?

@@ +1079,5 @@
> +
> +    /* Aux.3 steps 1-2 / Aux.4 steps 2-3 */
> +    AutoPropDescArrayRooter descs(cx);
> +    PropDesc *desc = descs.append();
> +    if (!desc || !desc->initialize(cx, *vp.address()))

It's pretty gross that you can't just pass vp here. Or at worst vp.get(). Oh well.

@@ +1101,5 @@
> +    RootedObject descObj(cx, &vp.toObject());
> +
> +    /* Aux.3 steps 4-5 / Aux.4 steps 5-6 */
> +    AutoIdVector props(cx);
> +    if (!GetPropertyNames(cx, descObj, JSITER_OWNONLY, &props))

Wrong object here. We want to get properties off of the Attributes object--that is, the object that was passed in, in *vp.

OWNONLY isn't what the spec asks for here; it says:
    "step 4 is determined as if by performing a for-in loop"
and so on -- we want both own and inherited properties.

This whole aspect of the behavior is one we definitely have no tests for.

@@ +1111,5 @@
> +            JSAtom *atom = JSID_TO_ATOM(id);
> +            const JSAtomState &atomState = cx->runtime->atomState;
> +            if (atom == atomState.valueAtom || atom == atomState.writableAtom ||
> +                atom == atomState.getAtom || atom == atomState.setAtom ||
> +                atom == atomState.enumerableAtom || atom == atomState.configurableAtom) {

Funny style nit: '{' gets its own line when the condition spans multiple lines.

@@ +1117,5 @@
> +            }
> +        }
> +
> +        RootedValue v(cx);
> +        if (!descObj->getGeneric(cx, descObj, id, &v))

Again, we want to get properties off of the Attributes object, not descObj.

@@ +1119,5 @@
> +
> +        RootedValue v(cx);
> +        if (!descObj->getGeneric(cx, descObj, id, &v))
> +            return false;
> +        if (!JS_DefinePropertyById(cx, descObj, id, v, NULL, NULL, JSPROP_ENUMERATE))

Use descObj->defineGeneric() instead.
Attachment #650295 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650296 [details] [diff] [review]
Part 12 - Implement auxiliary functions 5-7

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

::: js/src/jsproxy.cpp
@@ +615,3 @@
>  GetFundamentalTrap(JSContext *cx, HandleObject handler, HandlePropertyName name, MutableHandleValue fvalp)
>  {
> +    if (!handler->getProperty(cx, name, fvalp))

You removed a JS_CHECK_RECURSION call here and in GetDerivedTrap. Why? Those are usually there as a result of having been burned.

@@ +620,5 @@
>      if (!js_IsCallable(fvalp)) {
>          JSAutoByteString bytes;
>          if (js_AtomToPrintableString(cx, name, &bytes))
>              JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_FUNCTION, bytes.ptr());
>          return false;

I don't think all this (pre-existing code) is necessary, since js::Invoke will detect this condition and throw a TypeError anyway.

If it is necessary to avoid a mind-bogglingly misleading error message, add a comment to that effect; otherwise delete it.

@@ +1127,5 @@
>  
> +static inline bool
> +IsDataDescriptor(const PropertyDescriptor &desc)
> +{
> +    return !desc.obj && !(desc.attrs & (JSPROP_GETTER | JSPROP_SETTER));

Seems like the first ! should be removed.

@@ +1133,5 @@
> +
> +static inline bool
> +IsAccessorDescriptor(const PropertyDescriptor &desc)
> +{
> +    return !desc.obj && desc.attrs & (JSPROP_GETTER | JSPROP_SETTER);

Same thing here.

@@ +1138,5 @@
> +}
> +
> +/* Aux.5 ValidateProperty(O, P, Desc) */
> +static bool
> +ValidateProperty(JSContext *cx, HandleObject obj, HandleId id, PropDesc *desc, bool *bp)

I have written a bunch of comments below about the code in this method, but can we common up some code rather than add 70 new lines? (I'm just asking in case you didn't consider it at all. If you decided against it, I certainly accept your judgement.)

@@ +1158,5 @@
> +    /* Aux.5 step 4 */
> +    if (!current.obj && extensible) {
> +        *bp = true;
> +        return true;
> +    }

Merge steps 3 and 4:

    // Aux.5 steps 3 and 4
    if (!current.obj) {
        *bp = extensible;
        return true;
    }

@@ +1162,5 @@
> +    }
> +
> +    /* Aux.5 step 5 */
> +    if (!desc->hasValue() && !desc->hasWritable() && !desc->hasGet() && !desc->hasSet() &&
> +        !desc->hasEnumerable() && !desc->hasConfigurable()) {

'{' gets its own line.

@@ +1172,5 @@
> +    if ((!desc->hasWritable() || desc->writable() == !(current.attrs & JSPROP_READONLY)) &&
> +        (!desc->hasGet() || desc->getter() == current.getter) &&
> +        (!desc->hasSet() || desc->setter() == current.setter) &&
> +        (!desc->hasEnumerable() || desc->enumerable() == current.attrs & JSPROP_ENUMERATE) &&
> +        (!desc->hasConfigurable() || desc->configurable() == !(current.attrs & JSPROP_PERMANENT))) {

And here.

This expression bothers me:
    desc->enumerable() == current.attrs & JSPROP_ENUMERATE

As it happens, JSPROP_ENUMERATE == 1, so it works, but I don't think you meant to depend on that. It would be better to write something like:

    desc->enumerable() == bool(current.attrs & JSPROP_ENUMERATE)

@@ +1194,5 @@
> +            return true;
> +        }
> +
> +        if (desc->hasEnumerable() &&
> +            desc->enumerable() != (current.attrs & JSPROP_ENUMERATE)) {

Some comments as above regarding '{' and explicitly casting the RHS here to type bool.

@@ +1215,5 @@
> +        }
> +
> +        *bp = true;
> +        return true;
> +    }

Again, simplify to something like

    if (IsDataDescriptor(current) != desc->isDataDescriptor()) {
        *bp = !(current.attrs & JSPROP_PERMANENT);
        return true;
    }

@@ +1218,5 @@
> +        return true;
> +    }
> +
> +    /* Aux.5 step 10 */
> +    if (IsDataDescriptor(current) && desc->isDataDescriptor()) {

If IsDataDescriptor(current) is true, then we're already guaranteed that desc->isDataDescriptor() is true, by step 9. Turn the check into an assertion.

@@ +1224,5 @@
> +            if (current.attrs & JSPROP_READONLY && desc->hasWritable() && desc->writable()) {
> +                *bp = false;
> +                return true;
> +            }
> +        

Nit: remove the space characters on this blank line.

To avoid duplication (which I admit is present in the spec, but still) I think the structure of this block should be:

    if ((current.attrs & JSPROP_PERMANENT) && (current.attrs & JSPROP_READONLY)) {
        if (desc->hasWritable() && desc->writable()) {
            ...
        }

        if (desc->hasValue()) {
            ...
        }
    }

@@ +1243,5 @@
> +        return true;
> +    }
> +
> +    /* Aux.5 step 11 */
> +    if (IsAccessorDescriptor(current) && desc->isAccessorDescriptor()) {

should read:

    // Aux.5 step 11
    JS_ASSERT(IsAccessorDescriptor(current));
    JS_ASSERT(desc->isAccessorDescriptor());

After that you could finish it like this:

    *bp = (!(current.attrs & JSOP_PERMANENT) ||
           ((!desc->hasSet() || desc->setter() == current.setter) &&
            (!desc->hasGet() || desc->getter() == current.getter)));
    return true;

but it's up to you whether that's a good idea.

@@ +1278,5 @@
> +    }
> +
> +    /* Aux.6 step 3 */
> +    *bp = desc.attrs & JSPROP_PERMANENT;
> +    return true;

Again:

    // Aux.6 steps 2 and 3.
    *bp = (desc.obj && (desc.attrs & JSPROP_PERMANENT));
    return true;

@@ +1300,5 @@
> +        JSAutoByteString bytes;
> +        if (js_AtomToPrintableString(cx, name, &bytes))
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_FUNCTION, bytes.ptr());
> +        return false;
> +    }

At least, common up this code with the stuff in GetFundamentalTrap; but I hope we can just drop it.
Attachment #650296 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650297 [details] [diff] [review]
Part 13 - Implement TrapOwnProperty

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

::: js/src/jsobj.cpp
@@ +1139,5 @@
> +    /*
> +     * FIXME: Once ScriptedIndirectProxies are removed, this code should call TrapGetOwnProperty
> +     * directly
> +     */
> +    if (obj->isProxy()) 

Nit: Delete the trailing space on this line.

I think there should just be an extra method, obj->isScriptedDirectProxy() or whatever, so that we can implement the spec behavior in places like this.

(However we talked about this on IRC and I forget what we concluded, so maybe I am missing something.)

::: js/src/jsproxy.cpp
@@ +1321,5 @@
> +/* TrapGetOwnProperty(O, P) */
> +static bool
> +TrapGetOwnProperty(JSContext *cx, HandleObject proxy, HandleId id, MutableHandleValue rval)
> +{
> +    /* step 1 */

In case I didn't mention it, I'd love to see // comments for all these. One final patch converting the whole file would be fine, and easy to review.

@@ +1357,5 @@
> +    if (!NormalizeAndCompletePropertyDescriptor(cx, &trapResult))
> +        return false;
> +
> +    /* step 7 */
> +    if (rval.isUndefined()) {

trapResult, not rval.

@@ +1369,5 @@
> +
> +        if (!target->isExtensible()) {
> +            bool found;
> +            if (!HasOwn(cx, target, id, &found))
> +                return false;

It's silly that the spec does IsSealed and then HasOwn here (which does target.[[GetOwnProperty]] twice, possibly getting different results), but ok.

@@ +1372,5 @@
> +            if (!HasOwn(cx, target, id, &found))
> +                return false;
> +            if (found) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_CANT_REPORT_E_AS_NE);
> +                return false; 

Trailing space.

@@ +1396,5 @@
> +
> +    /* step 10 */
> +    if (isFixed) {
> +        bool valid;
> +        if (!ValidateProperty(cx, target, id, desc, &valid))

Same silliness here with HasOwn followed by ValidateProperty. But ok.

@@ +1406,5 @@
> +        }
> +    }
> +
> +    /* step 11 */
> +    if (!desc->configurable() && !isFixed) {

Please notify tomvc that this step of the spec has an error, in that 'desc' there is an Object, not a Property Descriptor. (Your code here looks like the most reasonable interpretation, so that's fine.)

Also, this "if" could be spelled:
    } else if (!desc->configurable()) {

@@ +1452,5 @@
> +        return true;
> +    }
> +
> +    /* step 3-4 */
> +    return ParsePropertyDescriptorObject(cx, proxy, v, desc, true);

It seems silly again that TrapGetOwnProperty involves calling NormalizeAndCompletePropertyDescriptor, which converts the Object to a Property Descriptor and then back to an Object; and now here we're converting it to a Property Descriptor again.

But once again, nothing wrong with this code.
Attachment #650297 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650298 [details] [diff] [review]
Part 14 - Implement getPropertyDescriptor trap

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

::: js/src/jsproxy.cpp
@@ +1437,5 @@
> +    do {
> +        if (GetProxyHandler(proxy)->getOwnPropertyDescriptor(cx, proxy, id, set, desc))
> +            return true;
> +        proxy = proxy->getProto();
> +    } while (proxy);

This can't be right. For one thing, there's no guarantee that proxy->getProto() will return another proxy, so the second time through the loop, GetProxyHandler(proxy) is unsafe.

You're improvising here, right? There's no such thing in the spec as far as I can tell.

What I would improvise is something like this:

    JS_CHECK_RECURSION(...);
    Rooted<JSObject*> obj(cx, proxy);
    Rooted<jsid> idr(cx, id);
    if (!GetOwnPropertyDescriptor(cx, obj, idr, desc))
        return false;
    if (desc->obj)
        return true;
    obj = obj->getProto();
    if (!obj)
        return true;  // desc->obj is already null.
    return JS_GetPropertyDescriptorById(cx, obj, idr.get(), 0, desc);
Attachment #650298 - Flags: review?(jorendorff) → feedback-
Comment on attachment 650299 [details] [diff] [review]
Part 15 - Implement DefineOwnProperty

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

::: js/src/jsproxy.cpp
@@ +1430,5 @@
> +    /* step 3 */
> +    RootedValue trap(cx);
> +    if (!GetTrap(cx, handler, ATOM(defineProperty), &trap))
> +        return false;
> +    

Nit: remove spaces on line.

@@ +1455,5 @@
> +        StringValue(name),
> +        normalizedDesc
> +    };
> +    RootedValue trapResult(cx);
> +    if (!Invoke(cx, ObjectValue(*handler), trap, 2, argv, trapResult.address()))

argc is 3 here, right?

@@ +1457,5 @@
> +    };
> +    RootedValue trapResult(cx);
> +    if (!Invoke(cx, ObjectValue(*handler), trap, 2, argv, trapResult.address()))
> +        return false;
> +    

Nit: remove spaces on line.

@@ +1464,5 @@
> +    if (!JS_ValueToBoolean(cx, trapResult, &success))
> +        return false;
> +
> +    /* step 8 */
> +    if (success) {

Instead:
    // steps 7-8
    if (ToBoolean(trapResult)) {
        ...

@@ +1472,5 @@
> +
> +        if (!target->isExtensible() && !isFixed) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_CANT_DEFINE_NEW);
> +            return false;
> +        }       

Nit: remove trailing whitespace.

@@ +1481,5 @@
> +            return false;
> +
> +        if (isFixed) {
> +            bool valid;
> +            if (!ValidateProperty(cx, target, id, desc, &valid)) 

Nit: trailing whitespace.

@@ +1489,5 @@
> +                return false;
> +            }
> +        }
> +
> +        if (desc->configurable() && !isFixed) {

!desc->configurable()

@@ +1497,5 @@
> +
> +        vp.set(BooleanValue(true));
> +        return true;
> +    }
> +    

Nit: spaces on blank line.

@@ +1500,5 @@
> +    }
> +    
> +    /* step 9 */
> +
> +    /* FIXME: API doesn't allow us to throw errors */

The comment should say:  /* FIXME: API does not include a Throw parameter */

(Of course the API allows throwing errors; we've been doing it all over the place ;)
Attachment #650299 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650300 [details] [diff] [review]
Part 16 - Implement getOwnPropertyNames trap

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

Just about perfect.

::: js/src/jsproxy.cpp
@@ +1039,5 @@
>  };
>  
>  static int sScriptedDirectProxyHandlerFamily = 0;
>  
> +

Style nit: Delete second blank line.

@@ +1518,5 @@
> +}
> + 
> +static bool
> +ArrayToIdVector(JSContext *cx, HandleObject proxy, HandleObject target, HandleValue v,
> +                AutoIdVector &props, JSAtom *trapName)

Function needs a brief explanatory comment, citing which part of the spec these "step" comments refer to.

@@ +1558,5 @@
> +        if (!HasOwn(cx, target, id, &isFixed))
> +            return false;
> +
> +        /* step v */
> +        if (!target->isExtensible() && !isFixed) {

Ugh. The spec is silly here again; we shouldn't call HasOwn if isExtensible() is true, as it usually will be.

@@ +1599,5 @@
> +
> +        /* step ii */
> +        bool isFixed;
> +        if (!HasOwn(cx, target, id, &isFixed))
> +            return false;

Again, spec weirdness: IsSealed followed by HasOwn, triggering two calls to .[[GetOwnProperty]] where one would suffice. And HasOwn shouldn't be called if target->isExtensible() is true.

@@ +1705,5 @@
> +    Value argv[] = {
> +        ObjectValue(*target)
> +    };
> +    RootedValue trapResult(cx);
> +    if (!Invoke(cx, ObjectOrNullValue(handler), trap, 1, argv, trapResult.address()))

You could use ObjectValue, right?
Attachment #650300 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650302 [details] [diff] [review]
Part 17 - Implement delete trap

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

::: js/src/jsproxy.cpp
@@ +1741,5 @@
> +    if (trap.isUndefined()) 
> +        return DirectProxyHandler::delete_(cx, proxy_, id_, bp);
> +
> +    /* step 5 */
> +    JSString *name = ToString(cx, IdToValue(id));

Could you make a single IdToHandlerValue function that does this and produces a Value?

We'll be adding private names (objects that can also be used as property ids) eventually, and it'll be nice to have a single place to change.

@@ +1758,5 @@
> +    if (!JS_ValueToBoolean(cx, trapResult, &success))
> +        return false;
> +
> +    /* step 7 */
> +    if (success) {

Again
    // steps 6-7
    if (ToBoolean(trapResult)) {

@@ +1773,5 @@
> +    }
> +
> +    /* step 8 */
> +
> +    // FIXME: API doesn't allow us to throw errors

same comment change as before.
Attachment #650302 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650303 [details] [diff] [review]
Part 18 - Implement enumerate trap

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

::: js/src/jsproxy.cpp
@@ +1778,5 @@
>      *bp = false;
>      return true;
>  }
>  
> +/* 12.6.4 The for-in Statement, step 4 */

It's step 6, in the wiki proposal, as of right now.

@@ +1818,5 @@
> +        return false;
> +    }
> +
> +    /* steps g-m are shared */
> +    return ArrayToIdVector(cx, proxy, target, trapResult, props, ATOM(enumerate));

Please add a comment noting that the proposal requires interleaving execution of the for-in loop with trapResult array element accesses (basically, it requires this to produce an iterator object for the for-in machinery to consume, something the Handler API doesn't support yet) and file the follow-up bug to make it actually work that way.
Attachment #650303 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650305 [details] [diff] [review]
Part 19 - Implement has trap

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

Same comments about ToBoolean and IdToHandlerValue as above.

::: js/src/jsproxy.cpp
@@ +1826,2 @@
>  bool
>  ScriptedDirectProxyHandler::has(JSContext *cx, JSObject *proxy, jsid id, bool *bp)

Should be proxy_ and id_. It's really strange that this compiles; what's going on here?

@@ +1882,5 @@
> +        }
> +    }
> +
> +    /* step 8 */
> +    *bp = !!success;

You don't need !! to convert JSBool to bool anymore; we apparently silenced the MSVC warning about that.

(Also there will be no conversion, once you switch to ToBoolean.)
Attachment #650305 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650306 [details] [diff] [review]
Part 20 - Implement hasOwn trap

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

Same comments about using bool instead of JSBool and ToBoolean instead of JS_ValueToBoolean.

::: js/src/jsproxy.cpp
@@ +1921,5 @@
> +    if (!Invoke(cx, ObjectValue(*handler), trap, 2, argv, trapResult.address()))
> +        return false;
> +
> +    /* step 6 */
> +    JSBool success;

Spec silliness: the variable should be called "found", not "success". Same thing in [[HasProperty]].

(But don't change it; it's more important to match the spec than to match common sense.)

@@ +1947,5 @@
> +        }
> +    }
> +    
> +    /* step 8 */
> +    if (!target->isExtensible()) {

This needs to be "else if", not "if".

Although actually, this code is so similar to step 7b that they could be merged, if you want. One says "if (isFixed)" where the other says "if (!isFixed)", and the error messages are different, but otherwise they're identical.
Attachment #650306 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650307 [details] [diff] [review]
Part 21 - Implement get trap

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

::: js/src/jsproxy.cpp
@@ +2007,5 @@
> +    
> +    /* step 7 */
> +    if (desc.obj) {
> +        if (IsDataDescriptor(desc) && (desc.attrs & JSPROP_PERMANENT) &&
> +            (desc.attrs & JSPROP_READONLY)) {

Style nit: '{' on its own line; and split the condition to three lines, breaking the lines after each '&&'.

@@ +2021,5 @@
> +        if (IsAccessorDescriptor(desc) && (desc.attrs & JSPROP_PERMANENT)) {
> +            if (!(desc.attrs & JSPROP_GETTER)) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MUST_REPORT_UNDEFINED);
> +                return false;
> +            }

The part of step 7.b.i where it says "If trapResult is not undefined" is missing.
Attachment #650307 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650327 [details] [diff] [review]
Part 22 - Implement set trap

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

Same comment about ToBoolean.

::: js/src/jsproxy.cpp
@@ +2061,5 @@
> +    Value argv[] = {
> +        ObjectOrNullValue(target),
> +        StringValue(name),
> +        *vp,
> +        ObjectOrNullValue(receiver)

receiver can't be null, can it?
Attachment #650327 - Flags: review?(jorendorff) → feedback+
Attachment #650330 - Flags: review?(jorendorff) → feedback+
Comment on attachment 650331 [details] [diff] [review]
Part 24 - Implement iterate trap

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

This looks like the same patch as part 23.

I'm not sure our iterate() method is called in at all the same situations as the "iterate" trap in the proposal. In fact I'm pretty sure it's not.

However I also hope the "iterate" proposal will change. I think it's pretty clear that we *don't* want a separate hook just for for-of loops, since it works via properties and proxies already support intercepting properties. I'll go argue that with dherman now.

Therefore just forget about part 24 for now and let's focus on getting the other stuff under test so it can land.
Attachment #650331 - Flags: review?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #178)
> Comment on attachment 650302 [details] [diff] [review]
> Part 17 - Implement delete trap
> 
> Review of attachment 650302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsproxy.cpp
> @@ +1741,5 @@
> > +    if (trap.isUndefined()) 
> > +        return DirectProxyHandler::delete_(cx, proxy_, id_, bp);
> > +
> > +    /* step 5 */
> > +    JSString *name = ToString(cx, IdToValue(id));
> 
> Could you make a single IdToHandlerValue function that does this and
> produces a Value?
> 

I don't understand the question. IdToValue *is* a single function that returns a Value, is it not?
(In reply to Jason Orendorff [:jorendorff] from comment #179)
> Comment on attachment 650303 [details] [diff] [review]
> Part 18 - Implement enumerate trap
> 
> Review of attachment 650303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsproxy.cpp
> @@ +1778,5 @@
> >      *bp = false;
> >      return true;
> >  }
> >  
> > +/* 12.6.4 The for-in Statement, step 4 */
> 
> It's step 6, in the wiki proposal, as of right now.
> 
> @@ +1818,5 @@
> > +        return false;
> > +    }
> > +
> > +    /* steps g-m are shared */
> > +    return ArrayToIdVector(cx, proxy, target, trapResult, props, ATOM(enumerate));
> 
> Please add a comment noting that the proposal requires interleaving
> execution of the for-in loop with trapResult array element accesses
> (basically, it requires this to produce an iterator object for the for-in
> machinery to consume, something the Handler API doesn't support yet) and
> file the follow-up bug to make it actually work that way.

I think we have another bug here. The for-in statement doesn't actually call Proxy::enumerate, it calls Proxy::iterate (see 
jsiter.cpp:683). The only places from which Proxy::enumerate is called is from the debugger, and when creating a snapshot. I can't easily replace the call to Proxy::iterate with Proxy::enumerate either, since the latter doesn't return an iterator object yet. This also means that I can't write proper tests for either trap at the moment.

All this stuff is horribly broken, but I don't think it should keep us from landing. After all, the original scripted proxies are broken in the same way. I feel that we should land what we have, and then fix the iteration stuff in a follow-up bug.
Depends on: 783829
Attached patch Part 7 - Dead code removal (obsolete) — Splinter Review
Attachment #650290 - Attachment is obsolete: true
Attachment #653263 - Flags: review?(jorendorff)
Attachment #650292 - Attachment is obsolete: true
Attachment #653264 - Flags: review?(jorendorff)
Attachment #650293 - Attachment is obsolete: true
Attachment #653265 - Flags: review?(jorendorff)
Attachment #650294 - Attachment is obsolete: true
Attachment #653266 - Flags: review?(jorendorff)
Attachment #650295 - Attachment is obsolete: true
Attachment #653267 - Flags: review?(jorendorff)
Note that GetTrap is now gone, as with your suggestions applied to it it would be a vestigial function, simply calling getProperty on handler.
Attachment #650296 - Attachment is obsolete: true
Attachment #653268 - Flags: review?(jorendorff)
Attachment #650297 - Attachment is obsolete: true
Attachment #653271 - Flags: review?(jorendorff)
Attachment #650298 - Attachment is obsolete: true
Attachment #653273 - Flags: review?(jorendorff)
Attachment #650299 - Attachment is obsolete: true
Attachment #653274 - Flags: review?(jorendorff)
Attachment #650300 - Attachment is obsolete: true
Attachment #653275 - Flags: review?(jorendorff)
Attachment #650302 - Attachment is obsolete: true
Attachment #653276 - Flags: review?(jorendorff)
Attachment #650303 - Attachment is obsolete: true
Attachment #653277 - Flags: review?(jorendorff)
Attached patch Part 19 - Implement has trap (obsolete) — Splinter Review
Attachment #650305 - Attachment is obsolete: true
Attachment #653279 - Flags: review?(jorendorff)
Attached patch Part 20 - Implement hasOwn trap (obsolete) — Splinter Review
Attachment #650306 - Attachment is obsolete: true
Attachment #653285 - Flags: review?(jorendorff)
Attached patch Part 21 - Implement get trap (obsolete) — Splinter Review
Attachment #650307 - Attachment is obsolete: true
Attachment #653288 - Flags: review?(jorendorff)
(In reply to Eddy Bruel [:ejpbruel] from comment #195)
> Created attachment 653273 [details] [diff] [review]
> Part 14 - Implement getPropertyDescriptor trap
I'm arriving late to the discussion, sorry, but this trap is gone as part of the direct proxy new design. To a large extent, the removal of this trap (and getPropertyNames trap too) came in pair with the internal object specification refactoring:
http://wiki.ecmascript.org/doku.php?id=harmony:proto_climbing_refactoring
Attached patch Part 22 - Implement set trap (obsolete) — Splinter Review
Attachment #650327 - Attachment is obsolete: true
Attachment #653291 - Flags: review?(jorendorff)
Attached patch Part 23 - Implement keys trap (obsolete) — Splinter Review
Ignored the iterate trap for now by your suggestion
Attachment #650330 - Attachment is obsolete: true
Attachment #650331 - Attachment is obsolete: true
Attachment #653292 - Flags: review?(jorendorff)
Attached patch Part 24 - Implement apply trap (obsolete) — Splinter Review
Attachment #653293 - Flags: review?(jorendorff)
Attachment #653295 - Flags: review?(jorendorff)
(In reply to David Bruant from comment #203)
> (In reply to Eddy Bruel [:ejpbruel] from comment #195)
> > Created attachment 653273 [details] [diff] [review]
> > Part 14 - Implement getPropertyDescriptor trap
> I'm arriving late to the discussion, sorry, but this trap is gone as part of
> the direct proxy new design. To a large extent, the removal of this trap
> (and getPropertyNames trap too) came in pair with the internal object
> specification refactoring:
> http://wiki.ecmascript.org/doku.php?id=harmony:proto_climbing_refactoring

The name of that patch is misleading. It doesn't actually provide a scriptable getPropertyDescriptor trap for direct proxies. Instead, it implements ScriptedDirectProxyHandler::getPropertyDescriptor in terms of getOwnPropertyDescriptor and getProto, as per the proto climbing refactoring. We can't actually get rid of the getPropertyDescriptor trap on the handler API until the old scripted proxies are phased out, so this seemed like the most sane solution in the meantime.
(In reply to Eddy Bruel [:ejpbruel] from comment #185)
> > > +    /* step 5 */
> > > +    JSString *name = ToString(cx, IdToValue(id));
> > 
> > Could you make a single IdToHandlerValue function that does this and
> > produces a Value?
> > 
> 
> I don't understand the question. IdToValue *is* a single function that
> returns a Value, is it not?

Yes, but it doesn't do ToString!
Due to general incompetence, I forgot to add the tests to hg before creating the diffs for these patches, so I'm going to file them all once again :-(
Attachment #653263 - Attachment is obsolete: true
Attachment #653263 - Flags: review?(jorendorff)
Attachment #653366 - Flags: review?(jorendorff)
Attachment #653264 - Attachment is obsolete: true
Attachment #653264 - Flags: review?(jorendorff)
Attachment #653367 - Flags: review?(jorendorff)
Attachment #653265 - Attachment is obsolete: true
Attachment #653265 - Flags: review?(jorendorff)
Attachment #653368 - Flags: review?(jorendorff)
Attachment #653266 - Attachment is obsolete: true
Attachment #653266 - Flags: review?(jorendorff)
Attachment #653369 - Flags: review?(jorendorff)
Attachment #653267 - Attachment is obsolete: true
Attachment #653267 - Flags: review?(jorendorff)
Attachment #653370 - Flags: review?(jorendorff)
Attachment #653268 - Attachment is obsolete: true
Attachment #653268 - Flags: review?(jorendorff)
Attachment #653372 - Flags: review?(jorendorff)
Attachment #653271 - Attachment is obsolete: true
Attachment #653271 - Flags: review?(jorendorff)
Attachment #653375 - Flags: review?(jorendorff)
Attachment #653273 - Attachment is obsolete: true
Attachment #653273 - Flags: review?(jorendorff)
Attachment #653376 - Flags: review?(jorendorff)
Attachment #653274 - Attachment is obsolete: true
Attachment #653274 - Flags: review?(jorendorff)
Attachment #653378 - Flags: review?(jorendorff)
Attachment #653275 - Attachment is obsolete: true
Attachment #653275 - Flags: review?(jorendorff)
Attachment #653379 - Flags: review?(jorendorff)
Attachment #653276 - Attachment is obsolete: true
Attachment #653276 - Flags: review?(jorendorff)
Attachment #653380 - Flags: review?(jorendorff)
Attachment #653277 - Attachment is obsolete: true
Attachment #653277 - Flags: review?(jorendorff)
Attachment #653381 - Flags: review?(jorendorff)
Attachment #653279 - Attachment is obsolete: true
Attachment #653279 - Flags: review?(jorendorff)
Attachment #653382 - Flags: review?(jorendorff)
Attachment #653285 - Attachment is obsolete: true
Attachment #653285 - Flags: review?(jorendorff)
Attachment #653383 - Flags: review?(jorendorff)
Attachment #653288 - Attachment is obsolete: true
Attachment #653288 - Flags: review?(jorendorff)
Attachment #653384 - Flags: review?(jorendorff)
Attachment #653291 - Attachment is obsolete: true
Attachment #653291 - Flags: review?(jorendorff)
Attachment #653385 - Flags: review?(jorendorff)
Attachment #653292 - Attachment is obsolete: true
Attachment #653292 - Flags: review?(jorendorff)
Attachment #653386 - Flags: review?(jorendorff)
Attachment #653293 - Attachment is obsolete: true
Attachment #653293 - Flags: review?(jorendorff)
Attachment #653387 - Flags: review?(jorendorff)
Attachment #653295 - Attachment is obsolete: true
Attachment #653295 - Flags: review?(jorendorff)
Attachment #653388 - Flags: review?(jorendorff)
Comment on attachment 653366 [details] [diff] [review]
Part 7 - Dead code removal

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

Sorry I didn't get to this yesterday.
Attachment #653366 - Flags: review?(jorendorff) → review+
Attachment #653367 - Flags: review?(jorendorff) → review+
(Incidentally, those two patches already had r+, and you should feel free to set r+ yourself on new versions of already-reviewed js/src patches when you post them. It'll say "r=ejpbruel" instead of "r=jorendorff" in bugzilla, but we all know what it means. It's ok.)
Attachment #653368 - Flags: review?(jorendorff) → review+
Comment on attachment 653369 [details] [diff] [review]
Part 10 - Implement Proxy constructor

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

I still don't see any tests in here, so just clearing the review flag until that can be remedied.

::: js/src/jsproxy.cpp
@@ +2062,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() < 2) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
> +                             "Proxy", "0", "s");

You forgot to change the error message from ("Proxy", "0", "s") to ("Proxy", "1", "")!
Attachment #653369 - Flags: review?(jorendorff)
Comment on attachment 653370 [details] [diff] [review]
Part 11 - Implement auxiliary functions 1-4

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

Memo to myself: we definitely want some tests for Aux.3 steps 4-5.

::: js/src/jsproxy.cpp
@@ +1090,5 @@
> +{
> +    // Aux.4 step 1
> +    if (complete && vp.isUndefined())
> +        return true;
> +        

spaces on blank line
Attachment #653370 - Flags: review?(jorendorff) → review+
Comment on attachment 653372 [details] [diff] [review]
Part 12 - Implement auxiliary functions 5-7

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

::: js/src/jit-test/tests/basic/testDirectProxyValidateProperty.js
@@ +1,5 @@
> +/*
> + * Throw a TypeError if the current descriptor is non-configurable and the trap
> + * returns a configurable descriptor
> + */
> +var target = {};

Awesome. This is 9 or so separate tests. Please put them in separate files so that in the future when this test starts crashing (which is inevitable), some poor hacker can start out with a 22-line test case to fix, instead of 243 lines.

(Or common up code to make the test leaner. Copy and paste sucks.)

(Or do both!)

Definitely delete the trailing whitespace though.

Another thing you could test pretty easily while you're in here is the case where the trap returns undefined, but the property exists on the target and is non-configurable.

@@ +18,5 @@
> +} catch (e) {
> +    assertEq(String(e).indexOf('TypeError'), 0);
> +    caught = true;
> +}
> +assertEq(caught, true);

Use assertThrowsInstanceOf. And move the stuff that should not throw out of that test, to avoid false negatives if we start throwing in the wrong place.

load(libdir + "asserts.js");
...
var p = new Proxy(target, {
    ...
});
assertThrowsInstanceOf(function () {
    Object.getOwnPropertyDescriptor(p, 'foo');
}, TypeError);

::: js/src/jsproxy.cpp
@@ +1197,5 @@
> +            return true;
> +        }
> +
> +        if (desc->hasEnumerable() &&
> +            desc->enumerable() != (current.attrs & JSPROP_ENUMERATE)) {

You missed my earlier review comment about this line:
> Some comments as above regarding '{' and explicitly casting the RHS here to type bool.

@@ +1218,5 @@
> +
> +    // step 10
> +    if (IsDataDescriptor(current)) {
> +        JS_ASSERT(desc->isDataDescriptor()); // by step 9
> +        if (current.attrs & JSPROP_PERMANENT && current.attrs & JSPROP_READONLY) {

Style micro-nit: we over-parenthesize these, like so:
> if ((current.attrs & JSPROP_PERMANENT) && (current.attrs & JSPROP_READONLY)) {

@@ +1265,5 @@
> +    }
> +
> +    // step 3
> +    *bp = desc.attrs & JSPROP_PERMANENT;
> +    return true;

I had a review comment about this too; I wanted you to write it this way instead:

    // Aux.6 steps 2 and 3.
    *bp = (desc.obj && (desc.attrs & JSPROP_PERMANENT));
    return true;

but it's ok if you really prefer it like it is.
Attachment #653372 - Flags: review?(jorendorff) → review+
Comment on attachment 653375 [details] [diff] [review]
Part 13 - Implement getOwnPropertyDescriptor trap

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

::: js/src/jit-test/tests/basic/testDirectProxyGetOwnPropertyDescriptor.js
@@ +1,1 @@
> +// Forward to the target if the trap is not defined

Same deal with splitting tests up and using asserts.js.

BTW, you should mkdir jit-test/tests/proxy for sure.

@@ +51,5 @@
> +try {
> +    Object.getOwnPropertyDescriptor(new Proxy(target, {
> +        getOwnPropertyDescriptor: function (target, name) {
> +            return undefined;
> +        }

Oh, I see you do have a test for this! Never mind then.

@@ +151,5 @@
> +}), 'foo');
> +assertEq(desc.value, 'baz');
> +assertEq(desc.writable, false);
> +assertEq(desc.enumerable, true);
> +assertEq(desc.configurable, true);

Could also test that the object returned by Object.getOwnPropertyDescriptor is not the same object that the trap returned: the proxy machinery makes a copy of it.

Could also test that missing property descriptor properties get filled in with the defaults (what happens if the trap returns {}).

::: js/src/jsobj.cpp
@@ +1155,5 @@
>  bool
>  GetOwnPropertyDescriptor(JSContext *cx, HandleObject obj, HandleId id, PropertyDescriptor *desc)
>  {
> +    // FIXME: Call TrapGetOwnProperty directly once ScriptedIndirectProxies is removed
> +    if (obj->isProxy()) 

trailing whitespace

::: js/src/jsproxy.cpp
@@ +1335,5 @@
> +            if (!HasOwn(cx, target, id, &found))
> +                return false;
> +            if (found) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_CANT_REPORT_E_AS_NE);
> +                return false; 

and here
Attachment #653375 - Flags: review?(jorendorff) → review+
Comment on attachment 653376 [details] [diff] [review]
Part 14 - Implement getPropertyDescriptor trap

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

::: js/src/jsproxy.cpp
@@ +1398,3 @@
>                                                    bool set, PropertyDescriptor *desc)
>  {
> +    Rooted<JSObject*> proxy(cx, proxy_);

No JS_CHECK_RECURSION?

As long as there's a simple reason for not having it, that's fine, but it's easy to make long proto chains happen, and I wouldn't be surprised if the recursive path here chewed up rather a lot of C++ stack.
Attachment #653376 - Flags: review?(jorendorff) → review+
Comment on attachment 653378 [details] [diff] [review]
Part 15 - Implement defineProperty trap

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

No tests?

Also: delete trailing whitespace.
Attachment #653378 - Flags: review?(jorendorff)
Comment on attachment 653379 [details] [diff] [review]
Part 16 - Implement getOwnPropertyNames trap

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

::: js/src/jit-test/tests/basic/testDirectProxyGetOwnPropertyNames.js
@@ +35,5 @@
> +        called = true;
> +        return [];
> +    }
> +};
> +Object.getOwnPropertyNames(new Proxy(target, handler));

Might as well assert something about the return value here, e.g. that .length == 0.

::: js/src/jsproxy.cpp
@@ +1477,5 @@
> +                         NullPtr(), bytes.ptr());
> +}
> +
> +static bool
> +ArrayToIdVector(JSContext *cx, HandleObject proxy, HandleObject target, HandleValue v,

Still needs a one-line comment.

@@ +1505,5 @@
> +        for (uint32_t j = 0; j < i; ++j) {
> +            if (props[j] == id) {
> +                ReportInvalidTrapResult(cx, proxy, trapName);
> +                return false;
> +            } 

delete trailing whitespace
Attachment #653379 - Flags: review?(jorendorff) → review+
Comment on attachment 653380 [details] [diff] [review]
Part 17 - Implement deleteProperty trap

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

delete trailing whitespace

::: js/src/jsproxy.cpp
@@ +1699,5 @@
> +    if (trap.isUndefined()) 
> +        return DirectProxyHandler::delete_(cx, proxy_, id_, bp);
> +
> +    // step 5
> +    JSString *name = ToString(cx, IdToValue(id));

Comment 209 still applies.
Attachment #653380 - Flags: review?(jorendorff) → review+
Comment on attachment 653381 [details] [diff] [review]
Part 18 - Implement enumerate trap

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

delete trailing whitespace

Throw in a FIXME comment pointing to the follow-up bug(s) please.
Attachment #653381 - Flags: review?(jorendorff) → review+
Comment on attachment 653382 [details] [diff] [review]
Part 19 - Implement has trap

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

::: js/src/jit-test/tests/basic/testDirectProxyHas.js
@@ +8,5 @@
> +        configurable: true
> +    }
> +}), {});
> +assertEq('foo' in proxy, true);
> +assertEq('bar' in proxy, true);

Might as well also:
  assertEq('baz' in proxy, false);
Attachment #653382 - Flags: review?(jorendorff) → review+
Comment on attachment 653383 [details] [diff] [review]
Part 20 - Implement hasOwn trap

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

delete trailing whitespace
Attachment #653383 - Flags: review?(jorendorff) → review+
Comment on attachment 653384 [details] [diff] [review]
Part 21 - Implement get trap

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

OK, I have to drive home, but I will try to get through the rest of these tonight.

delete trailing whitespace

::: js/src/jit-test/tests/basic/testDirectProxyGet.js
@@ +1,4 @@
> +// Forward to the target if the trap is not defined
> +assertEq(new Proxy({
> +    foo: 'bar'
> +}, {})['foo'], 'bar');

At least one or two of these should test p.foo as well as p['foo']; they are of course equivalent.

In fact they compile to identical bytecode so it seems a little superstitious to write p['foo'] in the first place.

It might be worth throwing in a test for p[s], where s is a variable/expression with the value 'foo'.

@@ +65,5 @@
> +} catch (e) {
> +    assertEq(String(e).indexOf('TypeError'), 0);
> +    caught = true;
> +}
> +assertEq(caught, true);

Might as well also test that returning undefined from the trap *does* work.
Attachment #653384 - Flags: review?(jorendorff) → review+
Comment on attachment 653385 [details] [diff] [review]
Part 22 - Implement set trap

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

delete trailing whitespace in test

More test coverage is needed here. Like maybe:
- default behavior:
  - when the target has a getter
  - when the target is also a proxy
  - when the target has a proxy as its prototype
  - when the target property doesn't exist and target is non-extensible, in strict and non-strict code
  - when the target property is non-writable, strict and non-strict
- check the receiver argument when the proxy is not the receiver:
    var handler = {
        set: function (t, n, v, receiver) {
            assertEq(receiver, q);
        }
    };
    var p = new Proxy(target, handler);
    var q = Object.create(p);
    q.x = v;  // handler.set(target, 'x', v, q); <--- q, not p
  If we can't do this right yet, file the follow-up bug.
- .set return value is handled correctly, strict and non-strict
  - even when it is not a primitive boolean

r=me with moar tests.

::: js/src/jit-test/tests/basic/testDirectProxySet.js
@@ +1,4 @@
> +// Forward to the target if the trap is not defined
> +assertEq(new Proxy({
> +    foo: 'bar'
> +}, {})['foo'] = 'baz', 'baz');

This will pass no matter what Proxy.[[Set]] does, because x.a = v evaluates to v:

js> var x = Object.freeze({a: 1});
js> x.a = 12;
12                      <--- even though x.a is non-writable!
js> x.a;
1

Fortunately "writing it out" does not really make it any longer.

var target = {foo: 'bar'};
new Proxy(target, {}).foo = 'baz';
assertEq(target.foo, 'baz');
Attachment #653385 - Flags: review?(jorendorff) → review+
Comment on attachment 653386 [details] [diff] [review]
Part 23 - Implement keys trap

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

Please notify tomvc that step 2.L of the proposed algorithm for Object.keys and Object.getOwnPropertyNames has us calling "the built-in Object.getOwnPropertyDescriptor", which makes no sense; I'm pretty sure it should be calling the builtin Object.keys or Object.getOwnPropertyNames, recursively.

Things to consider testing:
- when the trap returns:
  - some arraylike object other than an array
  - some totally non-array-like object
  - an object with a .length getter (the getter is called)
  - another proxy
  - an array with holes in it (I think a hole counts as "undefined")
  - an array of objects (their .toString methods are called)
- the array returned by Object.keys is not the object returned from the trap (it's a copy)
- default behavior, when the target is:
  - an array (the result should still be an array of strings, ["0", "1", "2"]; and no "length")
  - another proxy
- Object.keys(Object.create(proxy)) shouldn't call any proxy traps

r=me with moar tests and the other comments addressed.

::: js/src/jsproxy.cpp
@@ +2056,5 @@
>      *vp = BooleanValue(success);
>      return true;
>  }
>  
> +// 15.2.3.14 Object.keys (O), step 2 

delete trailing whitespace

@@ +2097,5 @@
> +        return false;
> +    }
> +
> +    // steps g-n are shared
> +    return ArrayToIdVector(cx, proxy, target, trapResult, props, ATOM(keys));

ArrayToIdVector checks that all non-configurable properties are listed, even non-enumerable ones, which we would expect Object.keys to ignore. Here's the test for this. I didn't check, but I expect it'll fail.

var target = Object.create(null, {x: {value: 0}});
var proxy = Proxy(target, {
    keys: function () { return []; }
});
assertEq(Object.keys(proxy).length, 0);  // shouldn't throw complaining about x

@@ +2103,5 @@
>  
>  bool
>  ScriptedDirectProxyHandler::iterate(JSContext *cx, JSObject *proxy, unsigned flags, Value *vp)
>  {
> +    // FIXME: Provide a proper implementation for this trap

Cite a follow-up bug number in this comment.
Attachment #653386 - Flags: review?(jorendorff) → review+
Comment on attachment 653387 [details] [diff] [review]
Part 24 - Implement apply trap

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

Things to test:
- default behavior:
  - when the target is not callable
  - when the target is a callable proxy
  - when the target function has default arguments
  - when the target function has a rest param
  - when the target function uses `arguments`
- if the target is not callable, proxy() is a TypeError and the apply trap is not called
- [].sort(f) works when f is a proxy of a function
- the receiver argument:
  - when calling the proxy directly:
    proxy(2, 3);  // receiver === undefined
  - when the proxy is a method of some other object:
    var obj = {method: proxy};
    obj.method(2, 3);   // receiver === obj
- typeof proxy === "function" if the target is a function.

r=me with moar tests.

::: js/src/jit-test/tests/basic/testDirectProxyApply.js
@@ +29,5 @@
> +}, {
> +    apply: function (target, receiver, args) {
> +        return (function (x, y) {
> +            return x * y;
> +        }).apply(receiver, args);

Seems unnecessarily fancy; it could just return args[0] * args[1].

::: js/src/jsproxy.cpp
@@ +2118,5 @@
> +    // step 1
> +    RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
> +
> +    // step 2
> +    RootedObject target(cx, GetProxyTargetObject(proxy));

Note that the proposal says this hook only exists on the proxy if the target is callable.

So in theory, we shouldn't get here unless the target is callable.

In practice, I don't think we're actually going to implement things that way, so we *will* get here even if target isn't callable. That means that to follow the spec correctly, we have to add a check here, and if target isn't callable, throw a TypeError here.

Note that just to make things extra exciting, it looks like JSObject::isCallable will be wrong in the case of direct proxies. I think that should just be fixed, and hope it's not perf-hot.
Attachment #653387 - Flags: review?(jorendorff) → review+
Comment on attachment 653388 [details] [diff] [review]
Part 25 - Implement construct trap

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

To test:
- new p() when p's target isn't callable (throws TypeError without trying the .construct hook)
- no-arg new:
  var result = new p;  // where p is a Proxy
- using new with a proxy that's a property of some other object:
  var obj = {p: proxy};
  var result = new obj.p;  // should be exactly the same as `new proxy`
- returning primitives from a construct hook
  - what happens when JS_New is invoked on such a proxy?

I'm a little worried about the primitive-returning case, since it's a longstanding invariant that new-expressions produce objects (reflected in JS_New's return type). In fact, ask bhackett how this change affects type inference.

r=me with that.

::: js/src/jit-test/tests/basic/testDirectProxyConstruct.js
@@ +2,5 @@
> +var target = function (x, y) {
> +    this.foo = x + y;
> +}
> +var obj = new (new Proxy(target, {}))(2, 3);
> +obj.foo = 5;

Also:
assertEq(Object.getPrototypeOf(obj), target.prototype);

@@ +18,5 @@
> +        assertEq(args[0], 2);
> +        assertEq(args[1], 3);
> +    }
> +}
> +new (new Proxy(target, handler))(2, 3);

Check that the result of this new-expression === undefined.

@@ +22,5 @@
> +new (new Proxy(target, handler))(2, 3);
> +
> +// Return the trap result
> +var obj = new (new Proxy(function (x, y) {
> +    this.foo = x * y;

In the Apply tests, the two functions here behaved differently; one used + and the other *. So that you could tell the difference, I figured.

@@ +29,5 @@
> +        return (function (x, y) {
> +            return {
> +                foo: x * y
> +            };
> +        }).apply(null, args);

As before, this could just return {foo: x * y}.
Attachment #653388 - Flags: review?(jorendorff) → review+
jorendorff, thanks for that thorough review of not only the code, but also the spec. I fixed the bugs in TrapGetOwnProperty step 11 and Object.getOwnPropertyNames step 2.l.

I'll try to refactor the spec to remove the redundancy introduced by the IsSealed helper.

Note also that at the July TC39 meeting, we decided that Object.{isExtensible,isSealed,isFrozen} will trap. Will add spec for that soon and report back.
Attachment #653369 - Attachment is obsolete: true
Attachment #655082 - Flags: review?(jorendorff)
Attachment #653378 - Attachment is obsolete: true
Attachment #655083 - Flags: review?(jorendorff)
Attachment #655082 - Flags: review?(jorendorff) → review+
Comment on attachment 655083 [details] [diff] [review]
Part 15 - Implement defineProperty trap

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

r=me with these notes addressed.

::: js/src/jit-test/tests/basic/testDirectProxyDefineProperty.js
@@ +1,1 @@
> +// Forward to the target if the trap is not defined

As with all the others, this should be several files under jit-test/tests/proxy.

@@ +59,5 @@
> +    caught = true;
> +}
> +assertEq(caught, true);
> +
> +// Throw a TypeError if the trap defines a new non-configurable property

"if the trap claims to have defined a non-configurable property, but it doesn't exist on the target"

Please add tests for the following 2 variations on this test as well:

- Throw a TypeError if the trap claims to have defined a non-configurable property, but the property on the target is configurable
  (just like this test, but in the Object.defineProperty trap, add these lines:)
    desc.configurable = true;
    Object.defineProperty(target, name, desc);

- Throw a TypeError if the trap claims to have defined a non-configurable property, but the property on the target is incompatible
  (just like this test, but in the Object.defineProperty trap, add these lines:)
    desc.value = 42;
    Object.defineProperty(target, name, desc);

@@ +82,5 @@
> +    defineProperty: function (target, name, desc) {
> +        desc.value = 'baz';
> +        desc.writable = false;
> +        desc.enumerable = true;
> +        Object.defineProperty(target, name, desc);

Check that desc is not actually the descriptor object that was passed to Object.defineProperty (it should be a copy of it).
Attachment #655083 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #239)
> Comment on attachment 653381 [details] [diff] [review]
> Part 18 - Implement enumerate trap
> 
> Review of attachment 653381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> delete trailing whitespace
> 
> Throw in a FIXME comment pointing to the follow-up bug(s) please.

There already is such a comment. It is a the end of ScriptedDirectProxyHandler::enumerate
(In reply to Jason Orendorff [:jorendorff] from comment #245)
> Comment on attachment 653387 [details] [diff] [review]
> Part 24 - Implement apply trap
> 
> Review of attachment 653387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Things to test:
> - default behavior:
>   - when the target is not callable
>   - when the target is a callable proxy
>   - when the target function has default arguments
>   - when the target function has a rest param
>   - when the target function uses `arguments`
> - if the target is not callable, proxy() is a TypeError and the apply trap
> is not called
> - [].sort(f) works when f is a proxy of a function
> - the receiver argument:
>   - when calling the proxy directly:
>     proxy(2, 3);  // receiver === undefined
>   - when the proxy is a method of some other object:
>     var obj = {method: proxy};
>     obj.method(2, 3);   // receiver === obj
> - typeof proxy === "function" if the target is a function.
> 
> r=me with moar tests.
> 
> ::: js/src/jit-test/tests/basic/testDirectProxyApply.js
> @@ +29,5 @@
> > +}, {
> > +    apply: function (target, receiver, args) {
> > +        return (function (x, y) {
> > +            return x * y;
> > +        }).apply(receiver, args);
> 
> Seems unnecessarily fancy; it could just return args[0] * args[1].
> 
> ::: js/src/jsproxy.cpp
> @@ +2118,5 @@
> > +    // step 1
> > +    RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
> > +
> > +    // step 2
> > +    RootedObject target(cx, GetProxyTargetObject(proxy));
> 
> Note that the proposal says this hook only exists on the proxy if the target
> is callable.
> 
> So in theory, we shouldn't get here unless the target is callable.
> 
> In practice, I don't think we're actually going to implement things that
> way, so we *will* get here even if target isn't callable. That means that to
> follow the spec correctly, we have to add a check here, and if target isn't
> callable, throw a TypeError here.
> 
> Note that just to make things extra exciting, it looks like
> JSObject::isCallable will be wrong in the case of direct proxies. I think
> that should just be fixed, and hope it's not perf-hot.

Currently, we *do* implement it this way, because the old scripted proxies did so too. That is, proxies created with a non-callable target object get a different class that does not include the call and construct trap (see NewProxyObject). Can we just leave a TODO comment in the traps that says we need to add a check if this is changed
(In reply to Jason Orendorff [:jorendorff] from comment #246)
> I'm a little worried about the primitive-returning case, since it's a
> longstanding invariant that new-expressions produce objects (reflected in
> JS_New's return type). In fact, ask bhackett how this change affects type
> inference.

TI shouldn't care, it already assumes nothing about the values produced by native calls, whether 'new' or not, and the result of invoking 'new' on a proxy will be monitored and the possible result types updated on each execution.
Well, here goes nothing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05adc6145143
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/05adc6145143
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla15 → mozilla18
Documentation: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Proxy
Old API documentation moved to : https://developer.mozilla.org/en-US/docs/JavaScript/Old_Proxy_API

I'm marking dev-doc-complete. Ping me if you consider there are major things to change in the doc.
Depends on: 788172
Depends on: 789893
No longer blocks: 601379
Depends on: 601379
Depends on: 601329
Depends on: 793160
I did a revision of the proxy spec based on jorendorff's comments, and some other pending changes. Here's a summary of what has changed recently on <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_spec>:

- The "iterate" trap has been removed in favor of "iterator" being defined simply as a unique name property.
- The "enumerate" trap now returns an iterator rather than an array of strings.
- Object.{isExtensible, isSealed, isFrozen} and Object.getPrototypeOf now trap.
- refactored the TrapGetOwnProperty, [[HasProperty]], [[HasOwnProperty]], Object.getOwnPropertyNames, Object.keys and [[Enumerate]] algorithms to remove redundant calls to [[HasOwnProperty]] (introduced by the IsSealed helper), and removed Aux 6. IsSealed altogether.

- In TrapGetOwnProperty, the check in step 11.a. was strengthened to include an OR test for targetDesc.[[Configurable]] === true. This is crucially important for the invariants associated with non-configurability. See <https://mail.mozilla.org/pipermail/es-discuss/2012-September/025033.html> for details. Test case:

var target = {};
Object.defineProperty(target, 'x', {value:1,configurable:true});
var proxy = Proxy(target, {
  getOwnPropertyDescriptor: function(target, name) {
    return { value: 2, configurable: false };
  }
});
Object.getOwnPropertyDescriptor(proxy, 'x') // should throw TypeError
Depends on: 789897
Depends on: 753402
Depends on: 795830
Depends on: 795894
I've filed a couple of followup bugs.

(In reply to Tom Van Cutsem from comment #257)
> - The "iterate" trap has been removed in favor of "iterator" being defined
> simply as a unique name property.
bug 795885

> - The "enumerate" trap now returns an iterator rather than an array of
> strings.
bug 795894

> - Object.{isExtensible, isSealed, isFrozen}
bug 795830

> Object.getPrototypeOf trap.
Bug 795904
 
> - In TrapGetOwnProperty, the check in step 11.a. was strengthened to include
> an OR test for targetDesc.[[Configurable]] === true. This is crucially
> important for the invariants associated with non-configurability. See
> <https://mail.mozilla.org/pipermail/es-discuss/2012-September/025033.html>
> for details. (...)
Bug 795903
Depends on: 795904
Depends on: 795903
Depends on: 798299
New significant changes in the direct proxies spec:

- In TrapDefineOwnProperty, the check in step 8.e.i. was strengthened to include an OR test for targetDesc.[[Configurable]] === true. This is important for the invariants associated with non-configurability. See <https://mail.mozilla.org/pipermail/es-discuss/2012-October/025555.html> for details. Test case:

var t = {a:1};
var p = new Proxy(t, {
  defineProperty: function(target, name, desc){
    return true;
  }
});

Object.defineProperty(p, 'a', {configurable: false}); // should throw TypeError

- Aux.5 ValidateProperty was renamed to IsCompatibleDescriptor and its signature was changed, to prevent redundant calls to target.[[GetOwnProperty]].
Depends on: 806299
A further Proxy spec refactoring after discussion on es-discuss (see <https://mail.mozilla.org/pipermail/es-discuss/2012-November/026098.html>):

Aux.3 NormalizePropertyDescriptor and Aux.4 NormalizeAndCompletePropertyDescriptor are now inlined.
The new behavior is identical except that:
- both TrapGetOwnProperty and TrapDefineOwnProperty do one less needless
conversion from Object to PropDesc
- both TrapGetOwnProperty and TrapDefineOwnProperty can now do all
invariant checks against an internal PropDesc instead of against a
normalized property descriptor Object
- in TrapGetOwnProperty, we can defer conversion from PropDesc to Object,
and copying of custom attributes, to the last possible moment, only after
all the invariant checks succeed.

Raw changes:
<http://wiki.ecmascript.org/doku.php?id=harmony:proxies_spec&rev=1349982638&do=diff>
Depends on: 814892
Depends on: 817401
Depends on: 787710
Depends on: 876114
Depends on: 878605
Depends on: 887212
Depends on: 934223
Depends on: 937689
Depends on: 945566
Depends on: 793210
Depends on: 979671
You need to log in before you can comment on or make changes to this bug.