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

RESOLVED FIXED in Firefox 41



4 years ago
4 years ago


(Reporter: gkw, Assigned: evilpie)


(Blocks 1 bug, {assertion, regression, testcase})

Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)


(Whiteboard: [fuzzblocker][jsbugmon:update])


(2 attachments)



4 years ago

asserts js debug shell on m-c changeset 35918b0441b4 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: propdef->pn_u.binary.right->>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/ -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:
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?
Flags: needinfo?(evilpies)

Comment 1

4 years ago
Posted file 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 = '', 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

Comment 2

4 years ago
Setting [fuzzblocker] because this seems to occur quite often in jsfunfuzz.
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
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

4 years ago
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.
Flags: needinfo?(evilpies)


4 years ago
Assignee: nobody → evilpies

Comment 5

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

Comment 6

4 years ago
I am not sure about the change ion IonMonkey. At least by our definition of lambdas in jsfun.h, methods aren't even lambdas.
Attachment #8607105 - Flags: review?(efaustbmo)


4 years ago
Attachment #8607105 - Attachment is patch: true
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(>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.
Attachment #8607105 - Flags: review?(efaustbmo) → review+
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.