636697 - Crash [@ js_watch_set]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

o0 = Iterator.prototype
o7 = {}
function f0(o) {
    delete o.p0
    try {
        o.__proto__ = o7
    } catch (e) {}
    o.p0 = function () {}
    o.watch('p0', function () {})
}
for (i = 0; i < 9; i++) {
    o7.__defineSetter__('p0', function () {});
    f0(o0);
    f0(o7)
}

crashes js debug and opt shell on TM changeset 9d8a96a61d71 without -m nor -j at js_watch_set, and also crashes Firefox 4 Beta 12 on Windows 7 at bp-d5ac9a6e-dc83-480e-a670-034ed2110225

Nominating and s-s just-in-case, assuming [sg:critical?] until further diagnosed.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Might be null-deref, tested on TM changeset 725b8cce5c72 in Linux 32-bit opt shell:

(gdb) x/i $pc
=> 0x808e605 <_ZL20js_watch_set_wrapperP9JSContextjPN2js5ValueE+341>:	movzbl 0x1d(%edx),%eax
(gdb) x/b $edx
0x0:	Cannot access memory at address 0x0
(gdb) x/b $eax
0x838dee0:	0x00

Valgrind with --smc-check=full from the same shell:

==16207== Invalid read of size 1
==16207==    at 0x808E605: js_watch_set_wrapper(JSContext*, unsigned int, js::Value*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CAA28: js::Invoke(JSContext*, js::CallArgs const&, unsigned int) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CB677: js::ExternalInvoke(JSContext*, js::Value const&, js::Value const&, unsigned int, js::Value*, js::Value*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CB731: js::ExternalGetOrSet(JSContext*, JSObject*, int, js::Value const&, JSAccessMode, unsigned int, js::Value*, js::Value*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80F5DA9: js::Shape::set(JSContext*, JSObject*, bool, js::Value*) const (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80EA0C0: js_SetPropertyHelper(JSContext*, JSObject*, int, unsigned int, js::Value*, int) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x82808DC: js::Interpret(JSContext*, JSStackFrame*, unsigned int, JSInterpMode) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CA80F: js::RunScript(JSContext*, JSScript*, JSStackFrame*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CBE98: js::Execute(JSContext*, JSObject*, JSScript*, JSStackFrame*, unsigned int, js::Value*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x805B6B9: JS_ExecuteScript (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x804F957: Process(JSContext*, JSObject*, char*, int) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80508CB: Shell(JSContext*, int, char**, char**) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==  Address 0x1d is not stack'd, malloc'd or (recently) free'd

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   62375:589bb166be02
user:        Jason Orendorff
date:        Tue Feb 08 16:09:33 2011 -0600
summary:     Bug 627984 - Tighten up assertions in JSObject::methodReadBarrier. r=brendan.

Comment 3

[David Mandelin [:dmandelin]]

It is an NPE.

Comment 4

[Jason Orendorff [:jorendorff]]

Reduced:

var a = {set p(x) {}};
a.watch('p', function () {});
var b = Object.create(a);
b.watch('p', function () {});
delete b.p;
b.p = 0;

It seems there's just be a big hole in watchpoints where deleted accessor properties are concerned, because:

  var hit = 0;
  var a = {set p(x) { hit++; }};
  a.watch('p', function (a, b, c) {});
  var b = Object.create(a);
  b.p = 0;                                // <=== setter is skipped!
  assertEq(hit, 1);                       // FAIL

Note that I'm not complaining that a's watch-handler is skipped. That's by design. But the watchpoint on a.p shouldn't cause the *setter* to be skipped!

I'm not going to try to fix that hole. But I will fix the null crash.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v1

With this patch, we behave the same in the case where b.p is watched and deleted as in the case where there is no b.p.

Easily triggered null crashes are bad, so I'd like to get approval and land this.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v2 - Same as v1 but with the test attached, d'oh

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #6)
> Created attachment 515758 [details] [diff] [review]
> v2 - Same as v1 but with the test attached, d'oh

With this patch, the following testcase seems to hang continuously. Intended?

o9 = /a/gi;
o59 = /abc*def/.__proto__;
function f17(o) {
    delete o.toString;
}
function f26(o) {
    var _var_ = o;
    o.toString = function () {};
    try {
        _var_.__defineSetter__('toString', function (v) {});
        o.watch('toString', function () {});
    } catch (e) {}
}
for (var i = 0; i < 15; i++)
{
    f26(o59);
    f26(o9);
    f17(o9);
}

Comment 8

[Jason Orendorff [:jorendorff]]

Not intended. Looking.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: v3 - same as v2, but drop the watchpoint properly

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 515949 [details] [diff] [review]
v3 - same as v2, but drop the watchpoint properly

>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp
>--- a/js/src/jsdbgapi.cpp
>+++ b/js/src/jsdbgapi.cpp
>@@ -713,23 +713,32 @@ js_watch_set(JSContext *cx, JSObject *ob
>     for (JSWatchPoint *wp = (JSWatchPoint *)rt->watchPointList.next;
>          &wp->links != &rt->watchPointList;
>          wp = (JSWatchPoint *)wp->links.next) {
>         const Shape *shape = wp->shape;
>         if (wp->object == obj && SHAPE_USERID(shape) == id && !(wp->flags & JSWP_HELD)) {
>             wp->flags |= JSWP_HELD;
>             DBG_UNLOCK(rt);
> 
>+            bool ok;
>             jsid propid = shape->id;
>             shape = obj->nativeLookup(propid);
>+            if (!shape) {
>+                /*
>+                 * This happens if the watched property has been deleted, but a
>+                 * prototype has a watched accessor property with the same
>+                 * name. See bug 636697.
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

D'oh! Suggest "of the same name.\n" to make it fit.

r=me with that, thanks.

/be

Comment 11

[Andreas Gal :gal]

Backed out. Breaks Linux builds.

http://hg.mozilla.org/mozilla-central/rev/99e112c8616d

Revoking approval as per discussion during the triage meeting (anything that wasn't ready to go in right there got dropped, this clearly wasn't).

Comment 12

[Andreas Gal :gal]

(its already .x, so we should be good without renom I think)

Comment 13

[Jason Orendorff [:jorendorff]]

Yes.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: v4

Comment 15

[Jason Orendorff [:jorendorff]]

Fixed the warnings and relanded in tracemonkey only.

http://hg.mozilla.org/tracemonkey/rev/1d7f280804a1

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Original landing: http://hg.mozilla.org/mozilla-central/rev/8b3a403a6d26
Backout: http://hg.mozilla.org/mozilla-central/rev/99e112c8616d
Relanding: http://hg.mozilla.org/mozilla-central/rev/1d7f280804a1

Comment 17

[Daniel Veditz [:dveditz]]

Assuming this doesn't affect 1.9.2.x since it's a recent regression.

Comment 18

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-636697.js.

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.