Deleted watchpoints don't always come back when assigned

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
Created attachment 510464 [details] [diff] [review]
v1

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+
(Assignee)

Comment 5

7 years ago
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
(Assignee)

Comment 7

7 years ago
(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.

Updated

7 years ago
Attachment #510464 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Blocks: 627984
(Assignee)

Comment 8

7 years ago
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.
(Assignee)

Updated

7 years ago
Depends on: 631723
(Assignee)

Comment 9

7 years ago
http://hg.mozilla.org/tracemonkey/rev/80914cc8ceda
Whiteboard: [fixed-in-tracemonkey]
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.