Tom Van Cutsem Reporter
	Description • 14 years ago

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

	David Bruant
	Comment 1 • 14 years ago

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.

	Josh Matthews [:jdm]
	Comment 2 • 13 years ago

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;

	Josh Matthews [:jdm]
	Comment 3 • 13 years ago

By which I mean that PropDesc::Initialize sets JSPROP_READONLY, and is called from ParsePropertyDescriptorObject.

	Josh Matthews [:jdm]
	Comment 4 • 13 years ago

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.

	Josh Matthews [:jdm]
	Comment 5 • 13 years ago

A try run of a patch that implemented comment 4 showed green for all jsreftests, fwiw.

	Jeff Walden [:Waldo]
	Comment 6 • 13 years ago

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

	Josh Matthews [:jdm]
	Comment 7 • 13 years ago

Bobby has discovered this same issue independently six months later, it turns out.

	David Bruant
	Comment 8 • 13 years ago

I ran the test in comment 0 and it logs 'true' twice in the latests nightly. Certainly fixed alongside with 702491