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

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: decoder, Assigned: arai)

Tracking

(Blocks: 1 bug, 4 keywords)

52 Branch
mozilla53
x86_64
Linux
assertion, jsbugmon, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 wontfix, firefox53 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(4 attachments)

(Reporter)

Description

a year ago
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

Updated

a year ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

a year ago
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.
Arai-san, is bug 1184922 a likely regressor?
Blocks: 1184922
Flags: needinfo?(arai.unmht)
(Assignee)

Updated

a year ago
Flags: needinfo?(arai.unmht) → needinfo?(shu)

Comment 3

a year ago
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.
Flags: needinfo?(shu)
(Assignee)

Updated

a year ago
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
(Assignee)

Comment 4

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

bug 1204028 patch should be rebased onto this.
Blocks: 1204028
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e77d5713b88c429d2ce41d3a2d8eb5d754f1fd6e
(Assignee)

Comment 6

a year ago
Created attachment 8818503 [details] [diff] [review]
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.
Attachment #8818503 - Flags: review?(shu)
(Assignee)

Comment 7

a year ago
Created attachment 8818504 [details] [diff] [review]
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.
Attachment #8818504 - Flags: review?(shu)

Comment 8

a year ago
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
Attachment #8818503 - Flags: review?(shu) → review+

Updated

a year ago
Attachment #8818504 - Flags: review?(shu) → review+
(Assignee)

Comment 9

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

Comment 10

a year ago
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

a year ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/06f2e1a6a1a2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4681df104175
IIUC, 52 is also affected? Please request Aurora approval on this when you get a chance, assuming that's true.
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → affected
Flags: needinfo?(arai.unmht)
Flags: in-testsuite+
Version: Trunk → 52 Branch
(Assignee)

Comment 14

a year ago
Created attachment 8822598 [details] [diff] [review]
(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
Flags: needinfo?(arai.unmht)
Attachment #8822598 - Flags: review+
Attachment #8822598 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 15

a year ago
Created attachment 8822599 [details] [diff] [review]
(mozilla-aurora) Part 2: Disallow emitting ParseNode twice (backout 908dce87d771). r=shu
Attachment #8822599 - Flags: review+
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.
Attachment #8822598 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
status-firefox52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.