631305 - Deleted watchpoints don't always come back when assigned

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Watchpoints are supposed to survive property deletion. If a property is watched, then deleted, then assigned to, the watchpoint should fire.

First test:

var o = {a:1, b:2};
o.watch("p", function() { return 13; });
delete o.p;         // delete property, but watchpoint still exists
//delete o.a;
o.p = 0;            // should call watchpoint handler but doesn't
assertEq(o.p, 13);  // FAILS

If I uncomment the `delete o.a` line, the test passes. So it's something to do with shape, or OWN_SHAPE, or dictionary mode.

Second test:

var n = 0;
var a = [];
for (var i = 0; i < 20; i++)
    a[i] = {};
a[18].watch("p", function () { n++; });
delete a[18].p;
for (var i = 0; i < 20; i++)
    a[i].p = 0;
assertEq(n, 1);

As a quick hack, I think setting a breakpoint should set the target object's OWN_SHAPE flag. Then shape would cover (a) deleted watchpoints and (b) the underlying setters concealed from js::Shape by the presence of a watchpoint. Both seem highly desirable.

I'm curious, though, to know how this code manages to miss js_UpdateWatchpointsForShape as it stands. Both JSObject::addProperty and JSObject::putProperty look right. It isn't a JIT issue; both tests fail in the interpreter.

Comment 1

[Jason Orendorff [:jorendorff]]

Er, I'm pretty sure this isn't security-sensitive. The watchpoint machinery seems to be robust against the potential problems here (like calling a watchpoint on the wrong object).

Comment 2

[Jason Orendorff [:jorendorff]]

The first test in comment 0 is detecting bug 631723.

I'll leave this open for the bug in the second test, which is rather different--it is a minor hole in the shape guarantees.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

This was easy pickings as I was cleaning up watchpoint badness for blockers. I'd like to land it.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 510464 [details] [diff] [review]
v1

>+    /*
>+     * Ensure that an object with watchpoints never has the same shape as an
>+     * object without them, even if the watched properties are deleted.
>+     */
>+    obj->watchpointOwnShapeChange(cx);

Do this only if !(attrs & JSPROP_SETTER) -- if there's a user-defined setter, the object has been shaped accordingly and changing to the js_watch_set_wrapper native function object setter created by WrapWatchedSetter will reshape again. No need to go to own-shape mode in this scenario. A test to cover this would be swell.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

I don't agree with the reasoning behind comment 4; the trouble arises when the property is deleted: two objects that differ only in deleted watchpoints can end up with the same shape.

Here's the test case for that. I'm surprised to note that it fails, apparently because properties defined this way can't be watched. The a[18].watch("p", ...) call silently fails. I'll look into it.


var desc = {configurable: true, get: function () { return 0; }, set: function () {}};
var a = [];
for (var i = 0; i < 20; i++) {
    a[i] = {};
    Object.defineProperty(a[i], "p", desc);
}
var n = 0;
a[18].watch("p", function () { n++; });
for (var i = 0; i < 20; i++) {
    delete a[i].p;
    a[i].p = 0;
}
assertEq(n, 1);

Comment 6

[Brendan Eich [:brendan]]

Aha, delete on the last property added does not switch to dictionary mode and ensure unique shape ever after. Want to land the patch as-is?

/be

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to comment #5)
> Here's the test case for that. I'm surprised to note that it fails, apparently
> because properties defined this way can't be watched. The a[18].watch("p", ...)
> call silently fails. I'll look into it.

Looked into it, filed bug 632391.

(In reply to comment #6)
> Want to land the patch as-is?

Yes. Looking for approval.

Comment 8

[Jason Orendorff [:jorendorff]]

Made this block bug 627984, as my reasoning in patching that bug relies on this stuff being correct.

I'll land this when bug 631723 gets review.

Comment 9

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/80914cc8ceda

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/80914cc8ceda