Object.defineProperty does not update existing watchpoints. This means it can make watchpoint records stale. In particular wp->shape can become stale. This is dangerous because wp->shape is a weak reference and it can get GC'd. My patch in bug 627984 originally was going to add two assertions that I had to disable. This test flunks the first assertion (that on entry to js_watch_set, wp->shape is correct). var o = {}; o.watch("p", function () {}); Object.defineProperty(o, "p", {enumerable: false}); o.p = 0; This variant flunks the second assertion (that after calling the watchpoint handler, wp->shape is correct). var o = {}; function f() { Object.defineProperty(o, "p", {enumerable: false}); } o.watch("p", f); o.p = 0; Marking s-g again since this is a potential GC bug. I think it could be exploited to expose uncloned functions pretty easily, but although that's something we try not to do, it's not instantly catastrophic as far as I know. Should probably softblock.

Correction. wp->shape isn't a weak pointer; it is traced. So this isn't a GC bug. I'll open this bug tomorrow unless anyone thinks it is exploitable.

This might be a Windows-only thing. What I'm seeing in the debugger is that WrapWatchedSetter does return &js_watch_set; /* & to silence schoolmarmish MSVC */ and about twenty lines away, js::IsWatchedProperty does return shape->setterOp() == js_watch_set; and the two addresses of js_watch_set being used in those two functions are different. Tomorrow I'll do a clean build and see if this goes away, and if not, patch it somehow.

OK. Comment 2 seems to be a bug in the MSVC debugger. The actual bug symptoms are the same on Mac as on Windows, and there's a non-supernatural explanation: The test sets the watchpoint first. Then we call Object.defineProperty. Since the only thing we tell Object.defineProperty about the new property is {enumerable: true}, it retains everything else (getter, setter, value, all other attributes) from the existing property. Note in particular that when Object.defineProperty asks the system what o.p's setter is, the shape says it's js_watch_set. So obj_defineProperty ends up calling js_DefineProperty with setter=js_watch_set. js_UpdateWatchpointsForShape is called at the right time, so it has the opportunity to update wp->shape. But when it sees that the new shape's setter is js_watch_set, it figures the caller must have known it was defining a watchpoint and so there's nothing to update. Alas there isn't some abstraction layer above which you don't have to worry about watchpoints. They don't hide from anyone. JS_GetPropertyAttrsGetterAndSetter sees watchpoints. Even Object.getOwnPropertyDescriptor sees watchpoints on accessor properties. More Monday.

This comment in js_SlowPathUpdateWatchpointsForShape makes it totally clear what the busted assumption is: > /* > * The watchpoint code uses the normal property-modification functions to >install its > * own watchpoint-aware shapes. Those functions report those changes back >to the > * watchpoint code, just as they do user-level changes. So if this change >is > * installing a watchpoint-aware shape, it's something we asked for >ourselves, and can > * proceed without interference. > */ > if (IsWatchedProperty(cx, newShape)) > return newShape;

This is not a blocker, but ideally I'd like to get all the watchpoint assertions fixed for FF4, since assertions are bad and these bugs are going to be one big blur in my head a month from now.

Comment on attachment 512176 [details] [diff] [review] v1 Sorry, triaged this out of review sight, looks good. Is it still wanted or are you redoing watchpoints? /be

This bug and bug 605596 have an r+ patch. Is it planned to fix this bug or just to replace with what happens in bug 638053 ?

jorendorff: is your r+'d patch (from 2011) for Object.defineProperty watchpoints still relevant?

super dead