602139 - Crash [@ js::CallJSPropertyOpSetter]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: stack trace

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]

Comment 1

[Jesse Ruderman]

Attachment: mac crash report

Comment 2

[Jim Blandy :jimb]

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

Comment 3

[Jim Blandy :jimb]

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.

Comment 4

[Jeff Walden [:Waldo]]

Note bug 577325.

Comment 5

[Jim Blandy :jimb]

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

Comment 6

[Jim Blandy :jimb]

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.

Comment 7

[Jim Blandy :jimb]

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.

Comment 8

[Jim Blandy :jimb]

Attachment: Test for crash while assigning to value property, formerly a watched setter.

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

Comment 9

[Jim Blandy :jimb]

Attachment: Test for crash while assigning to value property, formerly a watched setter.

Improved comments.

Comment 10

[Jim Blandy :jimb]

Attachment: Test for crash while assigning to value property, formerly a watched setter.

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

Comment 11

[Jim Blandy :jimb]

Attachment: Trivial cleanups to watchpoint implementation.

Separated for ease of review.

Comment 12

[Jim Blandy :jimb]

Attachment: Have a single function both save watched setters and create wrappers for them.

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.

Comment 13

[Jim Blandy :jimb]

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

Comment 14

[Jim Blandy :jimb]

Attachment: Test watchpoints on value properties changed to setters.

Reproduced in shell; test case attached.

Comment 15

[Jim Blandy :jimb]

This gets more and more interesting; filed bug 605596.

Comment 16

[Jim Blandy :jimb]

Another: bug 605631.

Comment 17

[Jim Blandy :jimb]

Attachment: Watchpoint tests.

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.

Comment 18

[Jim Blandy :jimb]

Attachment: Trivial cleanups to watchpoint implementation.

Separated for ease of review.

Comment 19

[Jim Blandy :jimb]

Attachment: Add js_UpdateWatchpointsForShape, to correctly update watchpoints after shape changes.

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.

Comment 20

[Jim Blandy :jimb]

That's my current queue; under test.

Comment 22

[Jim Blandy :jimb]

Attachment: Test watchpoints set on inherited setter properties.

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

Comment 23

[Jim Blandy :jimb]

Attachment: Watchpoint tests.

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.

Comment 24

[Jim Blandy :jimb]

Attachment: Trivial cleanups to watchpoint implementation.

Separated for ease of review.

Comment 25

[Jim Blandy :jimb]

Attachment: Add js_UpdateWatchpointsForShape, to correctly update watchpoints after shape changes.

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.

Comment 26

[Jim Blandy :jimb]

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

Comment 27

[Jason Orendorff [:jorendorff]]

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 28

[Jason Orendorff [:jorendorff]]

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.

Comment 29

[Jim Blandy :jimb]

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

Comment 30

[Jim Blandy :jimb]

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

Comment 31

[Jim Blandy :jimb]

Attachment: Watchpoint tests, revised.

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

Comment 32

[Jim Blandy :jimb]

Attachment: Add js_UpdateWatchpointsForShape, to correctly update watchpoints after shape changes. (revised)

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.

Comment 33

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/4e9b098ccf87

Comment 34

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c56444630bb0