1085464 - Crash [@ js::GeneratorObject::suspend] or Assertion failure: isObject(), at dist/include/js/Value.h

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f() {
    with(Proxy.createFunction(function() {
        return {
            get: function() {},
            has: function() {
                return wrap
            }
        }
    }(), (function() {}))) {
        yield
    }
}
for (i in f()) {}

asserts js debug shell on m-c changeset 33c0181c4a25 with --ion-eager --no-threads at Assertion failure: isObject(), at dist/include/js/Value.h and crashes js opt shell at js::GeneratorObject::suspend.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b56d94c7261a
user:        Jan de Mooij
date:        Fri Oct 17 10:19:40 2014 +0200
summary:     Bug 987560 - Greatly refactor generator implementation. Patch mostly written by Andy Wingo. r=wingo

Jan, is bug 987560 a possible regressor?

Setting s-s and assuming sec-critical because a weird memory address 0x7ffffffe seems to be accessed.

Comment 1

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

Attachment: debug and opt stacks

Partial opt stack:

(lldb) bt 3
* thread #1: tid = 0x1ff895, 0x0000000100436dd1 js-dbgDisabled-opt-64-dm-nsprBuild-darwin-33c0181c4a25`js::GeneratorObject::suspend(cx=0x000000010160ebe0, fp=0x00000001018418e0, pc=0x00000001016319d0, vp=0x0000000101841960, nvalues=0, suspendKind=NORMAL, obj=<unavailable>) + 97 at GeneratorObject.cpp:71, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x48)
  * frame #0: 0x0000000100436dd1 js-dbgDisabled-opt-64-dm-nsprBuild-darwin-33c0181c4a25`js::GeneratorObject::suspend(cx=0x000000010160ebe0, fp=0x00000001018418e0, pc=0x00000001016319d0, vp=0x0000000101841960, nvalues=0, suspendKind=NORMAL, obj=<unavailable>) + 97 at GeneratorObject.cpp:71
    frame #1: 0x000000010044a162 js-dbgDisabled-opt-64-dm-nsprBuild-darwin-33c0181c4a25`Interpret(JSContext*, js::RunState&) [inlined] js::InterpreterRegs::fp(this=0x000000010160ebe0, this=0x000000010160ebe0, this=<unavailable>, cx=0x000000010160ebe0, root=0x000000010160ebf8, dummy=<unavailable>, fp=<unavailable>, pc=<unavailable>, vp=<unavailable>, nvalues=<unavailable>) const + 52626 at GeneratorObject.h:67
    frame #2: 0x000000010044a157 js-dbgDisabled-opt-64-dm-nsprBuild-darwin-33c0181c4a25`Interpret(cx=0x000000010160ebe0, state=0x00007fff5fbff020) + 52615 at Interpreter.cpp:3400
(lldb)

===

Partial debug stack:

(lldb) bt 3
* thread #1: tid = 0x1ffa89, 0x000000010001e282 js-dbg-opt-64-dm-nsprBuild-darwin-33c0181c4a25`JS::Value::toObject() const [inlined] JSVAL_IS_OBJECT_IMPL(jsval_layout) + 28 at Value.h:792, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010001e282 js-dbg-opt-64-dm-nsprBuild-darwin-33c0181c4a25`JS::Value::toObject() const [inlined] JSVAL_IS_OBJECT_IMPL(jsval_layout) + 28 at Value.h:792
    frame #1: 0x000000010001e266 js-dbg-opt-64-dm-nsprBuild-darwin-33c0181c4a25`JS::Value::toObject() const [inlined] JS::Value::isObject(this=<unavailable>) const at Value.h:1133
    frame #2: 0x000000010001e266 js-dbg-opt-64-dm-nsprBuild-darwin-33c0181c4a25`JS::Value::toObject(this=<unavailable>) const + 182 at Value.h:1228
(lldb)

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

As discussed on IRC, the problem was that we emitted a JSOP_NAME for the .generator lookup inside a with-statement. Proxies can then intercept it and do weird things.

This patch makes sure we emit JSOP_GETALIASEDVAR and adds an assert to make it easier to catch similar bugs.

Comment 3

[Andy Wingo [:wingo]]

Comment on attachment 8508707 [details] [diff] [review]
Patch

Review of attachment 8508707 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with the extra test.  In the future if more dot-variables are added (a candidate would be .iterator for the iterator in for-of loops), then we should generalize this concept to something like "known lexicals": internally allocated variables which must be lexically bound.  As is the single check is fine.

Thanks for hunting this down, Jan!

::: js/src/jit-test/tests/basic/bug1085464.js
@@ +11,5 @@
> +        }
> +    }
> +    with ({}) {
> +        yield eval("3");
> +    }

Should probably add a test for objects like { ".generator": 100 }.

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ed2bd18a68

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5ed2bd18a68

Comment 6

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.