In xpcshell: var s = Components.utils.Sandbox("http://example.com/"); Components.utils.evalInSandbox("this.__defineSetter__('k',function(){});",s); Components.utils.evalInSandbox("this.watch('k', Math.pow)", s) Components.utils.evalInSandbox("function k(){}", s) Components.utils.evalInSandbox("k = 3;", s) Result: Crash [@ js::CallJSPropertyOpSetter]

Reproduces in JS shell, too, sort of: jimb@fyodor:~/moz/tm/js/src$ obj~/js js> this.__defineSetter__('k', function(){}); js> this.watch('k', Math.pow); js> function k(){} js> k = 3 Segmentation fault

For some reason, this only reproduces when each statement is entered into the shell directly; it does not reproduce if one puts the lines from the prior comment into a file and runs the file with 'js -f'. The crash comes when the watch code attempts to call the handler; while the js::Shape's setter is a JSFunction (that is, the 'setterObj' branch of the the union), js_watch_set is attempting to use it as a C++ function pointer (the 'rawSetter' branch of the union), because the 'JSPROP_SETTER' bit of the shape's attrs has been cleared by the 'function k(){}'. I'm working on understanding how that function definition is supposed to work in this circumstances --- in particular, why JSObject::putProperty has successfully carried across the setter pointer, but not the JSPROP_SETTER bit.

Note bug 577325.

(In reply to comment #4) > Note bug 577325. Thanks for the heads-up! I honestly hadn't wondered whether it was correct for the function definition to invoke the setter...

So, to summarize: First, we create an accessor property with a JavaScript setter. That is, the property's JSPROP_SETTER attribute is set, indicating that it is js::Shape::setterObj (a JSObject *) that is live, not js::Shape::rawSetter (a JSPropertyOp) --- the two share a union in js::Shape. Then, we place a watchpoint on that property. We do this by establishing a setter on the shape that implements the watchpoint behavior. However, placing a watchpoint must preserve the property's JSPROP_SETTER attribute, so we actually have two kinds of watchpoint setters: js_watch_set, which is a JSPropertyOp and is used for data properties; and JavaScript native-code functions whose underlying C++ function is js_watch_set_wrapper. In either case, the original setter (whether JSPropertyOp or JSObject *) gets saved in the watchpoint data structure, where js_watch_set (to which js_watch_set_wrapper defers) will find it and call it. Critically, js_watch_set uses the *shape*'s JSPROP_SETTER attribute to decide how to interpret the *watchpoint's* saved setter. Finally, the test case defines a function whose name is that property. As per ES5 + July 31 2010 errata, this is meant to *replace* the existing setter property. However, when we do this, 1) we fail to clear the setter that was moved to the watchpoint structure, even though the watched property is no longer a setter, and to make matters worse, 2) we do clear the property's JSPROP_SETTER attribute, causing js_watch_set to treat the JSObject * as if it were a JSPropertyOp. Jumping to the address of a JSObject as if it were executable code causes the segmentation fault. I think the right place for the fix is NormalizeGetterAndSetter. I'm working on this now. Hopefully I'll be able to put together a regression test that can be run from a file in the ordinary JS shell.

In trying to make this reproducible via a script loaded into the shell, I've found filed bug Bug 604781 - Watchpoints set on setters do not always trigger.

Here's a patch that adds a test case that detects the bug in the JS shell. Fix forthcoming.

Improved comments.

Latest rev of patch; will r? if tests look good.

Separated for ease of review.

This patch makes the JSWatchPoint type visible in the new header jsdbgapiinlines.h, whereas it had previously been private to jsdbgapi.cpp. However, the actual intra-SM inter-file watchpoint API gets simpler (see the changes to jsscope.cpp), so I think this is a case where modularity is improved via inline functions, whose definitions require us to expose more in the 'inlines' header.

TEST-UNEXPECTED-FAIL | file:///home/jimb/moz/tm/js/src/tests/jsreftest.html?test=js1_5/extensions/regress-488995.js | Exited with code 1 during test run

Reproduced in shell; test case attached.

This gets more and more interesting; filed bug 605596.

Another: bug 605631.

This includes: - a test showing how adding and deleting watchpoints can lose a property's JSPropertyOp setter; and - tests for watchpoints on properties that change from setters to value properties and vice versa.

Separated for ease of review.

Latest patch; different approach from previous. Many of the watchpoint bugs have to do with wp->setter and wp->shape getting out of sync. The new function js_UpdateWatchpointsForShape takes care of bringing all relevant watchpoints fully up to date. It is also used by the watchpoint creation code. This patch makes the JSWatchPoint type visible in the new header jsdbgapiinlines.h, whereas it had previously been private to jsdbgapi.cpp. However, the actual intra-SM inter-file watchpoint API gets simpler (see the declarations removed from jsdbgapi.h, and the changes to jsscope.cpp), so I think this is a case where modularity is improved via inline functions, whose definitions require us to expose more in the 'inlines' header.

That's my current queue; under test.

Found another bug. Fixing a regression I found will probably fix this, too.

This includes: - a test showing how adding and deleting watchpoints can lose a property's JSPropertyOp setter; - tests for watchpoints on properties that change from setters to value properties and vice versa; and - tests for watchpoints set on inherited setter properties.

Separated for ease of review.

Many of the watchpoint bugs have to do with wp->setter and wp->shape getting out of sync. The new function js_UpdateWatchpointsForShape takes care of bringing all relevant watchpoints fully up to date. It is also used by the watchpoint creation code. This patch makes the JSWatchPoint type visible in the new header jsdbgapiinlines.h, whereas it had previously been private to jsdbgapi.cpp. However, the actual intra-SM inter-file watchpoint API gets simpler (see the declarations removed from jsdbgapi.h, and the changes to jsscope.cpp), so I think this is a case where modularity is improved via inline functions, whose definitions require us to expose more in the 'inlines' header.

Okay, that's the patch queue, ready for review. No regressions in shell tests, jit tests, or jstestbrowser.

Comment on attachment 485141 [details] [diff] [review] Watchpoint tests. watch-replaced-setter.js might as well test for correctness too (and not just test that we don't segfault). The comments in watch-replaced-setter.js make it sound like we have a bug: >+ * [...] However, the >+ * watchpoint is still holding the original JavaScript setter value, and >+ * will use the shape's attributes to interpret it. >+ * >+ * We ought to clear the watchpoint's setter: [...] Of course we *do* have a bug, currently, but to avoid confusion when someone reads this test years hence, the comment should just say what needs to happen for correctness.

Comment on attachment 485143 [details] [diff] [review] Add js_UpdateWatchpointsForShape, to correctly update watchpoints after shape changes. OK. So this won't ding performance because we were already paying for it in NormalizeGetterAndSetter, is that the theory? It should be fine. Fingers crossed. >+static inline bool >+js_UpdateWatchpointsForShape(JSContext *cx, JSObject *obj, const js::Shape >*new_shape) >+{ >+ if (JS_CLIST_IS_EMPTY(&cx->runtime->watchPointList)) >+ return true; >+ >+ JSWatchPoint *wp = js_FindWatchPoint(cx->runtime, obj, new_shape->id); >+ if (!wp) >+ return true; >+ >+ return js_UpdateWatchpointShape(cx, wp, new_shape); >+} Style nit: There are several counterexamples in jsdbgapi code, but the prevailing style is still camelCaps: newShape, not new_shape. Only the case where watchPointList is empty is hot. It seems to me the rest shouldn't be inlined; this should read: return JS_CLIST_IS_EMPTY(&cx->runtime->watchPointList) || ReallyUpdateBlahBlah(cx, obj, newShape); >@@ -861,6 +866,10 @@ JSObject::putProperty(JSContext *cx, jsi > */ > if (shape->matchesParamsAfterId(getter, setter, slot, attrs, flags, >shortid)) { > METER(redundantPuts); >+ if (!js_UpdateWatchpointsForShape(cx, this, shape)) { >+ METER(wrapWatchFails); >+ return NULL; >+ } > return shape; > } Why is this one necessary? Isn't this the case where we haven't mutated shape after all? I think an update is necessary in changeProperty too, when inDictionaryMode(). r=me with that. Brendan might want to take a look too.

(In reply to comment #27) > Comment on attachment 485141 [details] [diff] [review] > Watchpoint tests. > > watch-replaced-setter.js might as well test for correctness too (and not just > test that we don't segfault). ... > Of course we *do* have a bug, currently, but to avoid confusion when someone > reads this test years hence, the comment should just say what needs to happen > for correctness. Yes --- I wrote these before I started debugging; I should have gone over them to make them make sense post-fix.

(In reply to comment #28) > OK. So this won't ding performance because we were already paying for it in > NormalizeGetterAndSetter, is that the theory? It should be fine. Fingers > crossed. There should be no change in behavior in the no-watchpoints case, so I would be very surprised if this affects performance. > Style nit: There are several counterexamples in jsdbgapi code, but the > prevailing style is still camelCaps: newShape, not new_shape. Will fix, thanks. > Only the case where watchPointList is empty is hot. It seems to me the rest > shouldn't be inlined; this should read: Sure, I can remove the watchpoint search from the inlined portion. > Why is this one necessary? Isn't this the case where we haven't mutated shape > after all? Oh --- indeed it is! Awesome. > I think an update is necessary in changeProperty too, when inDictionaryMode(). I will check.

This revision includes a test for the JSObject::changeProperty case that was missing previously.

This is a revision of the earlier patch that adds a call to js_UpdateWatchpointsForShape to JSObject::changeProperty, which I had missed, and moves code out of the fast path exposed in jsdbgapiinlines.h. As a result, a lot more is kept private to the watchpoint implementation. I believe this addresses all review comments.