Closed Bug 631305 Opened 9 years ago Closed 9 years ago

Deleted watchpoints don't always come back when assigned

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 file)

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.
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).
Group: core-security
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.
Attached patch v1Splinter Review
This was easy pickings as I was cleaning up watchpoint badness for blockers. I'd like to land it.
Assignee: general → jorendorff
Attachment #510464 - Flags: review?(brendan)
Attachment #510464 - Flags: approval2.0?
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
Attachment #510464 - Flags: review?(brendan) → review+
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);
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
(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.
Attachment #510464 - Flags: approval2.0? → approval2.0+
Blocks: 627984
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.
Depends on: 631723
http://hg.mozilla.org/tracemonkey/rev/80914cc8ceda
Whiteboard: [fixed-in-tracemonkey]
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.