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

RESOLVED FIXED in Firefox 41

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gkw, Assigned: evilpie)

Tracking

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

Trunk
mozilla41
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

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

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 = '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)
(Reporter)

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.
(Assignee)

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)
(Assignee)

Updated

4 years ago
Assignee: nobody → evilpies
(Assignee)

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.
(Assignee)

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)
(Assignee)

Updated

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(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.
Attachment #8607105 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/282554b1f983
Status: NEW → RESOLVED
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.