1165794 - Assertion failure: propdef->pn_u.binary.right->pn_u.name.funbox->function()->isMethod(), at frontend/BytecodeEmitter.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

makeFinalizeObserver("nursery").bind({
    get
    7()
    eval()
})

asserts js debug shell on m-c changeset 35918b0441b4 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: propdef->pn_u.binary.right->pn_u.name.funbox->function()->isMethod(), at frontend/BytecodeEmitter.cpp.

Configure options:

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

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 35918b0441b4

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2fd7da3aa49a
user:        Tom Schuster
date:        Fri May 15 20:53:03 2015 +0200
summary:     Bug 1059908 - Merge FunctionType and FunctionSyntaxKind. r=efaust

Tom, is bug 1059908 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x12b4d8, 0x000000010017eb65 js-dbg-64-dm-nsprBuild-darwin-35918b0441b4`js::frontend::BytecodeEmitter::emitPropertyList(this=<unavailable>, pn=<unavailable>, objp=<unavailable>, type=<unavailable>) + 2101 at BytecodeEmitter.cpp:6769, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010017eb65 js-dbg-64-dm-nsprBuild-darwin-35918b0441b4`js::frontend::BytecodeEmitter::emitPropertyList(this=<unavailable>, pn=<unavailable>, objp=<unavailable>, type=<unavailable>) + 2101 at BytecodeEmitter.cpp:6769
    frame #1: 0x000000010017ed11 js-dbg-64-dm-nsprBuild-darwin-35918b0441b4`js::frontend::BytecodeEmitter::emitObject(this=0x00007fff5fbfdee8, pn=0x00000001028b6170) + 321 at BytecodeEmitter.cpp:6845
    frame #2: 0x00000001001629c3 js-dbg-64-dm-nsprBuild-darwin-35918b0441b4`js::frontend::BytecodeEmitter::emitTree(this=0x00007fff5fbfdee8, pn=0x00000001028b6170) + 1459 at BytecodeEmitter.cpp:7587
    frame #3: 0x000000010017cf44 js-dbg-64-dm-nsprBuild-darwin-35918b0441b4`js::frontend::BytecodeEmitter::emitCallOrNew(this=0x00007fff5fbfdee8, pn=0x00000001028b6138) + 1156 at BytecodeEmitter.cpp:6429
    frame #4: 0x0000000100162653 js-dbg-64-dm-nsprBuild-darwin-35918b0441b4`js::frontend::BytecodeEmitter::emitTree(this=0x00007fff5fbfdee8, pn=0x00000001028b6138) + 579 at BytecodeEmitter.cpp:7498
(lldb)

Comment 2

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

Setting [fuzzblocker] because this seems to occur quite often in jsfunfuzz.

Comment 3

[Eric Faust [:efaust]]

Yeah, I think it's likely both that bug 1059908 is the regressor, and that the assertion is just bogus, and should be updated to isMethod() || isGetter() || isSetter(). I will let Tom investigate fully, though.

Comment 4

[Tom S [:evilpie]]

Indeed, I will now follow your suggestions and add some method like "isMethodKind" or similar. I haven't come up with a good name for this yet.

Comment 5

[Tom S [:evilpie]]

Oo. I forgot to give getters/setters an extended allocation kind and somehow this didn't trigger any test failures.

Comment 6

[Tom S [:evilpie]]

Attachment: Correctly handle getter/setters like methods everywhere

I am not sure about the change ion IonMonkey. At least by our definition of lambdas in jsfun.h, methods aren't even lambdas.

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8607105 [details] [diff] [review]
Correctly handle getter/setters like methods everywhere

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

Nice. Thanks for fixing this.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +282,5 @@
>          return nullptr;
>  
>      bool savedCallerFun = evalCaller && evalCaller->functionOrCallerFunction();
> +    bool allowSuperProperty = savedCallerFun &&
> +                              evalCaller->functionOrCallerFunction()->allowSuperProperty();

I have an r+ patch in bug 1164777 that reworks all this stuff. I should land that today or tomorrow. When it lands, we just want to make a similar change inside GlobalSharedContext::allowSuperProperty() in the obvious place.

::: js/src/jit-test/tests/parser/home-object-getter.js
@@ +1,4 @@
> +var o = {get a() {
> +    return eval("5");
> +}}
> +assertEq(o.a, 5);

This test doesn't actually test the home object. We just asserted when we noticed we should have had one. How do we not have jstests in ecma_6/Class that handle this? Please add one that uses |super.prop| and |super[expr]| if we don't, somehow.

::: js/src/jit/CodeGenerator.cpp
@@ +1712,5 @@
>  
>      emitLambdaInit(output, scopeChain, info);
>  
>      if (info.flags & JSFunction::EXTENDED) {
> +        MOZ_ASSERT(info.fun->allowSuperProperty());

This is fine. Even though they are not lambdas, we emit JSOP_LAMBDA to clone them from the script when they are created, which will wind us up here.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/282554b1f983

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/282554b1f983