Closed Bug 602139 Opened 9 years ago Closed 9 years ago

Crash [@ js::CallJSPropertyOpSetter]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: jimb)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical][fixed-in-tracemonkey])

Crash Data

Attachments

(7 files, 10 obsolete files)

4.92 KB, text/plain
Details
26.44 KB, text/plain
Details
6.03 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.82 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
14.81 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
9.02 KB, patch
Details | Diff | Splinter Review
17.42 KB, patch
Details | Diff | Splinter Review
Attached file 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]
Attached file mac crash report
Whiteboard: [sg:critical]
Assignee: general → jimb
Assignee: jimb → jimb
blocking2.0: --- → final+
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.
OS: Linux → Windows CE
OS: Windows CE → Linux
(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.
Attachment #483679 - Attachment is obsolete: true
Latest rev of patch; will r? if tests look good.
Attachment #483681 - Attachment is obsolete: true
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.
Attached patch Watchpoint tests. (obsolete) — Splinter Review
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.
Attachment #483865 - Attachment is obsolete: true
Attachment #484044 - Attachment is obsolete: true
Separated for ease of review.
Attachment #483866 - Attachment is obsolete: true
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.
Attachment #483867 - Attachment is obsolete: true
That's my current queue; under test.
Duplicate of this bug: 605631
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.
Attachment #484845 - Attachment is obsolete: true
Attachment #484939 - Attachment is obsolete: true
Separated for ease of review.
Attachment #484847 - Attachment is obsolete: true
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.
Attachment #484848 - Attachment is obsolete: true
Okay, that's the patch queue, ready for review. No regressions in shell tests, jit tests, or jstestbrowser.
Status: NEW → ASSIGNED
Attachment #485141 - Flags: review?(jorendorff)
Attachment #485142 - Flags: review?(jorendorff)
Attachment #485143 - Flags: review?(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.
Attachment #485141 - Flags: review?(jorendorff) → review+
Attachment #485142 - Flags: review?(jorendorff) → review+
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.
Attachment #485143 - Flags: review?(jorendorff) → review+
(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.
Blocks: 609924
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.
http://hg.mozilla.org/tracemonkey/rev/4e9b098ccf87
Whiteboard: [sg:critical] → [sg:critical][fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/c56444630bb0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Crash Signature: [@ js::CallJSPropertyOpSetter]
Group: core-security
You need to log in before you can comment on or make changes to this bug.