604781 - Watchpoints set on setters do not always trigger; and, zombie setters!

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Depending on the details of how the property is defined, watchpoints on setter properties may not trigger. In particular, a watchpoint set on p.x does trigger:

var p = { set x(v) { setter(v); } };

But a watchpoint set on o.x does not:

var o = Object.defineProperty({}, 'x', { set:setter, enumerable:true, configurable:true });

The two should be identical, except for the specific setter function.

Comment 1

[Jim Blandy :jimb]

Attachment: Test case.

Comment 2

[Jim Blandy :jimb]

<jorendorff> jimb: In the second case, the property so defined is
             JSPROP_READONLY  [14:45]
<jorendorff> jimb: which i think is nonsense, and we rightly ignore it in most
             places, but perhaps not in the watch_setter

Comment 3

[Jim Blandy :jimb]

Here's another lovely bit of behavior:

function watcher(id,old,newval) { print("assignment watched"); return newval; }
var o = { set x(v) { print("setting x"); } };
o.watch('x', watcher);
Object.defineProperty(o, 'x', { value:3,
                                writable:true,
                                enumerable:true,
                                configurable:true });
o.x = 3;

Both the watchpoint and the setter run!

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v1

This fix applies on top of bug 637985. Will request review once that one is ready.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v2

Unbitrotted. Let's get this in.

Comment 6

[Jason Orendorff [:jorendorff]]

Incidentally the tests pass even without the change to obj_watch, I think because that property is not JSPROP_READONLY anymore; but there is no point having a line of code in obj_watch that doesn't do anything.

Comment 7

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/502b21011c3e

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/502b21011c3e