1322314 - Assertion failure: &env->as<LexicalEnvironmentObject>().scope() == si.scope(), at js/src/vm/Stack.cpp:130

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 8103c612b79c (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe):

function ax(f) f();
function test_one(pattern, val) {
    eval(`
      function* g${0}
       (${pattern}) {}
        [g${0}(${"[]"})]
    `);
}
function test(expr) {
    pattern = `[a=${expr}]`;
    test_one(pattern);
}
test(`class E {[ ax(() => TypeError)]() {}}`);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
AssertScopeMatchesEnvironment (scope=<optimized out>, originalEnv=<optimized out>) at js/src/vm/Stack.cpp:130
#0  AssertScopeMatchesEnvironment (scope=<optimized out>, originalEnv=<optimized out>) at js/src/vm/Stack.cpp:130
#1  0x0000000000c0effc in js::InterpreterFrame::prologue (this=0x7ffff0227378, cx=0x7ffff695f000) at js/src/vm/Stack.cpp:230
#2  0x0000000000b54d04 in Interpret (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:1775
#3  0x0000000000b629c5 in js::RunScript (cx=cx@entry=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:403
#4  0x0000000000b62d59 in js::InternalCallOrConstruct (cx=0x7ffff695f000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:475
#5  0x0000000000b5598e in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:508
#6  Interpret (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:2919
#7  0x0000000000b629c5 in js::RunScript (cx=cx@entry=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:403
#8  0x0000000000b62d59 in js::InternalCallOrConstruct (cx=0x7ffff695f000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:475
#9  0x0000000000b5598e in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:508
#10 Interpret (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:2919
#11 0x0000000000b629c5 in js::RunScript (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:403
[...]
#25 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7944
rax	0x2029520	33723680
rbx	0x123def8	19128056
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffacd0	140737488334032
rsp	0x7fffffffac60	140737488333920
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff0227378	140737222177656
r13	0x7fffffffad20	140737488334112
r14	0x2002d80	33566080
r15	0x7ffff07004a0	140737227261088
rip	0xc0e50f <AssertScopeMatchesEnvironment(js::Scope*, JSObject*)+1951>
=> 0xc0e50f <AssertScopeMatchesEnvironment(js::Scope*, JSObject*)+1951>:	movl   $0x0,0x0
   0xc0e51a <AssertScopeMatchesEnvironment(js::Scope*, JSObject*)+1962>:	ud2

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9716bcfed35d
user:        Tooru Fujisawa
date:        Tue Sep 27 13:57:00 2016 +0900
summary:     Bug 1184922 - Part 1: Do not call iter.next() if the previous iter.next().done was true in array destructuring. r=shu

This iteration took 236.049 seconds to run.

Comment 2

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

Arai-san, is bug 1184922 a likely regressor?

Comment 3

[Shu-yu Guo [:shu]]

arai and I looked at this together in person. The bug is this:

For destructuring defaults we emit the RHS twice. The RHS in this case is a class expression, which introduces a new LexicalScope when emitted. Because we emit the RHS twice, we create 2 distinct LexicalScopes. Let's call these lexicalScope1 and lexicalScope2. The class expression contains a nested function. The function, on the other hand, is emitted exactly once, and thus has as its enclosing scope either one of lexicalScope1 or lexicalScope2. During execution, the LexicalEnvironmentObject that's pushed by bytecode, depending on the branch taken, is tied to lexicalScope1 or lexicalScope2. So, depending on which branch we take during execution of the destructuring default, we will hit the scope/env-chain mismatch.

Possible fixes:

1. Go back to the approach of emitting destructuring default RHS exactly once.
2. Special-case class expressions in destructuring default RHS, since it can introduce new lexical scopes.
3. Change the scope/env-chain matching logic to check for "deep" (i.e. bindings and their locations) equality instead of pointer equality.

Option 1 or 3 are the most sensible. I'm not sure if option 3 gets us into trouble elsewhere.

Comment 4

[Tooru Fujisawa [:arai]]

try is running:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5c612160c38aa645c6c05ce5d31a4bb1679bde8

bug 1204028 patch should be rebased onto this.

Comment 5

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e77d5713b88c429d2ce41d3a2d8eb5d754f1fd6e

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Do not emit ParseNode twice in BytecodeEmitter::emitDestructuringOpsArray.

Now each destructuring element is handled in each loop, and each loop doesn't check the next element's type etc.  it only checks if there's next element, to whether put [[Done]] on the stack or not.

To simplify the stack operation, removed needToPopIterator. now it's popped after the loop in any case.

Now destructuring LHS and default's RHS is not emitted twice, and it's always outside of local branch, so emitDestructuringLHSInBranch is unused and removed.

Also, modified the stack comment.
There seems to be 2 purpose for "?" in the stack comment, one for optional, other for boolean.
I think we should stop using "?" for boolean, and also maybe use "?" prefix for optional value.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Disallow emitting ParseNode twice (backout 908dce87d771).

Also, backed out the change to make it possible to emit ParseNode twice.
Now it will hit assertion failure when emitting object including function twice.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8818503 [details] [diff] [review]
Part 1: Do not emit ParseNode twice in BytecodeEmitter::emitDestructuringOpsArray.

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

Looks good. There's a few places where you emit JSOP_TRUE or JSOP_FALSE because the original 'done' value is consumed by the branch. Would it save some code here to always DUP the 'done' value? Up to you -- just wondering.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4800,5 @@
> +    //
> +    //   d = value;
> +
> +    // Use an iterator to destructure the RHS, instead of index lookup. We
> +    //  must leave the *original* value on the stack.

Nit: extra space before 'must'

@@ +4815,5 @@
>              IfThenElseEmitter ifThenElse(this);
>              if (!isHead) {
>                  // If spread is not the first element of the pattern,
>                  // iterator can already be completed.
> +                //                                                // ... OBJ ITER DONE

Nit: extra // in front

@@ +4866,5 @@
> +        IfThenElseEmitter ifAlreadyDone(this);
> +        if (!isHead) {
> +            // If this element is not the first element of the pattern,
> +            // iterator can already be completed.
> +            //                                                    // ... OBJ ITER DONE

Nit: extra // in front

Comment 9

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to Shu-yu Guo [:shu] from comment #8)
> Comment on attachment 8818503 [details] [diff] [review]
> Part 1: Do not emit ParseNode twice in
> BytecodeEmitter::emitDestructuringOpsArray.
> 
> Review of attachment 8818503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. There's a few places where you emit JSOP_TRUE or JSOP_FALSE
> because the original 'done' value is consumed by the branch. Would it save
> some code here to always DUP the 'done' value? Up to you -- just wondering.

Yeah, apparently DUP-ing it before the branch is simpler.
changed it and now each branch has same pushed() count regardless of hasNext.

Comment 10

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4681df10417515f060f2b48eeeddf96c16828dcf
Bug 1322314 - Part 1: Do not emit ParseNode twice in BytecodeEmitter::emitDestructuringOpsArray. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/06f2e1a6a1a29e58b211e94b93e813c39b7d150e
Bug 1322314 - Part 2: Disallow emitting ParseNode twice (backout 908dce87d771). r=shu

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06f2e1a6a1a2

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4681df104175

Comment 13

[Ryan VanderMeulen [:RyanVM]]

IIUC, 52 is also affected? Please request Aurora approval on this when you get a chance, assuming that's true.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora) Part 1: Do not emit ParseNode twice in BytecodeEmitter::emitDestructuringOpsArray. r=shu

Thanks :)

Approval Request Comment
> [Feature/Bug causing the regression]

bug 1184922

> [User impact if declined]

No user impact.
It helps debug build to avoid hitting assertion failure.
The assertion failure hit by this can be ignored, since the situation satisfied underlying requirement that is checked by the assertion.

> [Is this code covered by automated tests?]

Yes

> [Has the fix been verified in Nightly?]

Yes

> [Needs manual test from QE? If yes, steps to reproduce]

No

> [List of other uplifts needed for the feature/fix]

None

> [Is the change risky?]

Yes.

> [Why is the change risky/not risky?]

It introduces different codepath for emitter and also in generated code.
automated tests would catch most issues if any though.

> [String changes made/needed]

None

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora) Part 2: Disallow emitting ParseNode twice (backout 908dce87d771). r=shu

Comment 16

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8822598 [details] [diff] [review]
(mozilla-aurora) Part 1: Do not emit ParseNode twice in BytecodeEmitter::emitDestructuringOpsArray. r=shu

As this only affects debug builds, and considering the risk involved in the fix, I think I'd rather let this ride the trains.