Last Comment Bug 645726 - [Harmony Proxies] derived set trap doesn't set accessor properties
: [Harmony Proxies] derived set trap doesn't set accessor properties
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: ---
Assigned To: general
:
:
Mentors:
Depends on: 702491
Blocks: harmony:proxies
  Show dependency treegraph
 
Reported: 2011-03-28 09:47 PDT by Tom Van Cutsem
Modified: 2011-12-17 11:02 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Tom Van Cutsem 2011-03-28 09:47:37 PDT
If a handler does not implement a "set" trap, the default implementation should fall back on the "get{Own}PropertyDescriptor" trap. This happens, but for accessors this default implementation does not call the setter.

Tested on the tracemonkey trunk.

Minimal example included below:


var a = 2;
var target = {
  get accessorProp() { return a; },
  set accessorProp(v) { a = v; },
};

var handler = {
  getOwnPropertyDescriptor: function(name) {
    return Object.getOwnPropertyDescriptor(target, name);
  },
  getPropertyDescriptor: function(name) {
    var desc = Object.getOwnPropertyDescriptor(target, name);
    var parent = Object.getPrototypeOf(target);
    while (desc === undefined && parent !== null) {
      desc = Object.getOwnPropertyDescriptor(parent, name);
      parent = Object.getPrototypeOf(parent);
    }
    return desc;
  }
};

var proxy = Proxy.create(handler, Object.getPrototypeOf(target));
         
print(proxy.accessorProp === 2); // true
proxy.accessorProp = 3;
print(proxy.accessorProp === 3); // false, expected true
Comment 1 David Bruant 2011-03-28 11:14:30 PDT
Noticing the same here in FF4 Web Console. I've been playing with the code and I arrive to the same conclusion.

For this particular test case, the getPropertyDescriptor could be reduced to:
---
getPropertyDescriptor: function(name) {
    return Object.getOwnPropertyDescriptor(target, name);
  }
---
The bug appears the same way with this getPropertyDescriptor trap.
Comment 2 Josh Matthews [:jdm] 2011-04-16 08:49:21 PDT
We correctly retrieve the property descriptor in JSProxyHandler::set, but then bail because the property is apparently readonly (as set unconditionally by ParsePropertyDescriptorObject.

>    if (!getOwnPropertyDescriptor(cx, proxy, id, true, &desc))
>        return false;
>    /* The control-flow here differs from ::get() because of the fall-through case below. */
>    if (desc.obj) {
>        if (desc.attrs & JSPROP_READONLY)
>            return true;
Comment 3 Josh Matthews [:jdm] 2011-04-16 08:52:43 PDT
By which I mean that PropDesc::Initialize sets JSPROP_READONLY, and is called from ParsePropertyDescriptorObject.
Comment 4 Josh Matthews [:jdm] 2011-04-16 09:16:22 PDT
Should we be removing JSPROP_READONLY when there is a setter present in PropDesc::Initialize?  I have a very poor grasp of the spec and how this ties in with non-proxies, but it seems strange to me that there would be a property with a setter that cannot be called.
Comment 5 Josh Matthews [:jdm] 2011-04-16 11:26:32 PDT
A try run of a patch that implemented comment 4 showed green for all jsreftests, fwiw.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-28 12:10:01 PDT
The readonly/writable concept doesn't apply to accessor properties, and eventually I would like to assert that any query of a property's attributes never queries writable() if the property is an accessor property.  But we're not there yet.

What you should be doing, and what any spec should be doing, is switching based on the type of the property.  It should handle accessor descriptors one way.  And it should handle data descriptors another way.  (And don't ask me what it should do for PropertyOp-based descriptors, which are neither fish nor fowl, at least not consistently.)
Comment 7 Josh Matthews [:jdm] 2011-11-14 22:36:08 PST
Bobby has discovered this same issue independently six months later, it turns out.
Comment 8 David Bruant 2011-12-17 11:02:34 PST
I ran the test in comment 0 and it logs 'true' twice in the latests nightly. Certainly fixed alongside with 702491

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