645726 - [Harmony Proxies] derived set trap doesn't set accessor properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Tom Van Cutsem]

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]

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]]

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]]

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

Comment 4

[Josh Matthews [:jdm]]

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]]

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

Comment 6

[Jeff Walden [:Waldo]]

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]]

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

Comment 8

[David Bruant]

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