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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Tom Van Cutsem, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

6 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
(Reporter)

Updated

6 years ago
Priority: -- → P2

Comment 1

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

Comment 2

6 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;

Comment 3

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

Comment 4

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

Comment 5

6 years ago
A try run of a patch that implemented comment 4 showed green for all jsreftests, fwiw.
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.)

Updated

6 years ago
Blocks: 546590
Blocks: 694103
No longer blocks: 694103

Comment 7

6 years ago
Bobby has discovered this same issue independently six months later, it turns out.
Depends on: 702491

Comment 8

5 years ago
I ran the test in comment 0 and it logs 'true' twice in the latests nightly. Certainly fixed alongside with 702491
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.