Assertion failure: CheckVarNameConflict(cx, lexicalScope, dn), at vm/Interpreter-inl.h

RESOLVED FIXED in Firefox 47

Status

()

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

People

(Reporter: gkw, Assigned: shu)

Tracking

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

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

Firefox Tracking Flags

(firefox45 affected, firefox47 fixed)

Details

(Whiteboard: [jsbugmon:update,ignore])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
let x;
try {} catch (x) {
    with({}) {
        var x;
    }
}

asserts js debug shell on m-c changeset bc74dbdea094 with --fuzzing-safe --no-threads --no-ion --no-baseline at Assertion failure: CheckVarNameConflict(cx, lexicalScope, dn), at vm/Interpreter-inl.h

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 --disable-threadsafe --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r bc74dbdea094

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ac0aa2c21379
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:30 2015 -0700
summary:     Bug 589199 - Implement all-or-nothing redeclaration checks for global and eval scripts. (r=efaust)

Shu-yu, is bug 589199 a likely regressor?
Flags: needinfo?(shu)
(Reporter)

Comment 1

4 years ago
Posted file stack
(lldb) bt 5
* thread #1: tid = 0x17b966, 0x00000001006c49f8 js-dbg-64-dm-darwin-bc74dbdea094`js::DefVarOperation(cx=<unavailable>, varobj=<unavailable>, dn=<unavailable>, attrs=5) + 920 at Interpreter-inl.h:398, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001006c49f8 js-dbg-64-dm-darwin-bc74dbdea094`js::DefVarOperation(cx=<unavailable>, varobj=<unavailable>, dn=<unavailable>, attrs=5) + 920 at Interpreter-inl.h:398
    frame #1: 0x00000001006907b1 js-dbg-64-dm-darwin-bc74dbdea094`Interpret(cx=0x0000000102d69400, state=0x00007fff5fbfeed8) + 64721 at Interpreter.cpp:3134
    frame #2: 0x0000000100680a5c js-dbg-64-dm-darwin-bc74dbdea094`js::RunScript(cx=0x0000000102d69400, state=0x00007fff5fbfeed8) + 412 at Interpreter.cpp:341
    frame #3: 0x0000000100698889 js-dbg-64-dm-darwin-bc74dbdea094`js::ExecuteKernel(cx=0x0000000102d69400, script=<unavailable>, scopeChainArg=0x0000000102e5e070, thisv=0x00007fff5fbfefe0, newTargetValue=0x00007fff5fbfefc8, type=EXECUTE_GLOBAL, evalInFrame=<unavailable>, result=0x0000000000000000) + 1337 at Interpreter.cpp:603
    frame #4: 0x0000000100698cc3 js-dbg-64-dm-darwin-bc74dbdea094`js::Execute(cx=0x0000000102d69400, script=<unavailable>, scopeChainArg=<unavailable>, rval=0x0000000000000000) + 547 at Interpreter.cpp:639
(lldb)
(Assignee)

Comment 2

4 years ago
Posted patch Implement ES6 Annex B.3.5. (obsolete) — Splinter Review
In fixing this fuzz bug I decided to also just implement B.3.5. The implementation is _so_ gross. Please let me know if you have better ideas.
Attachment #8688282 - Flags: review?(jorendorff)
(Assignee)

Updated

4 years ago
Flags: needinfo?(shu)
Comment on attachment 8688282 [details] [diff] [review]
Implement ES6 Annex B.3.5.

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

Aside from the content of the patch: I can't figure out why this is in an annex. Any ideas?

The spec says it also affects function declarations, but I don't see anything about those in the patch. If this patch fixes functions too, please add tests; if it doesn't and they are not to spec, add tests anyway to make sure we don't assert and die, and file a follow-up bug.

::: js/src/frontend/Parser.cpp
@@ +188,4 @@
>      else
>          MOZ_ASSERT(!decls_.lookupFirst(name));
>  
> +

Style micro-nit: extra blank line here

@@ +270,5 @@
>              dn->pn_dflags |= PND_CLOSED;
> +
> +        // See note in Parser::bindVar about ES6 Annex B.3.5.
> +        if (declaringVarInCatchBody) {
> +            if (!decls_.addInverseShadow(name, dn))

name suggestions: addClownShoes, addOverThereNotHere, addUniqueAtBodyLevel

@@ +3597,5 @@
>      }
> +
> +    // Even if the binding doesn't appear in any blocks, it might still be a
> +    // body-level lexical.
> +    return pc->isBodyLevelLexicallyDeclaredName(atom);

While you're here, please comment this function OuterLet().

@@ +3665,5 @@
> +            pc->declaringVarInCatchBody = false;
> +            if (!rv)
> +                return false;
> +            if (data->declarationsNode())
> +                parser->handler.addList(data->declarationsNode(), synthesizedVarName);

I don't understand how this works. Doesn't this mean that the synthesized node is added to the variables node *in addition to* the one created in Parser::variables(), not *instead of*?

Also this is just a little bit ugly

::: js/src/frontend/Parser.h
@@ +266,5 @@
> +
> +    // Set when declaring a 'var' binding despite a shadowing lexical binding
> +    // of the same name already existing as a catch parameter. Covered by ES6
> +    // Annex B.3.5.
> +    bool declaringVarInCatchBody:1;

Isn't this just a boolean parameter to define()?

Please implement it that way instead if possible.

::: js/src/tests/ecma_6/LexicalEnvironment/var-in-catch-body-annex-b.js
@@ +20,5 @@
> +  try { throw 8; } catch (x) {
> +    var x = 42;
> +    log += x;
> +  }
> +  log += x;

This test should distinguish whether the x being accessed on this last line is global or function-local (i.e. that the `var` isn't being ignored altogether).

@@ +31,5 @@
> +} catch (e) {
> +  log += "e";
> +}
> +
> +reportCompare(log, "e420e");

assertThrowsInstanceOf(()=>Function(...), SyntaxError) instead of logging?

Please add tests checking:

* that `catch (x) { for (var x in obj); }` is allowed
* and `for (var x;;)`
* but the corresponding for-of loop is not.
* when the var-statement and/or the catch clause use destructuring.
* that `catch (x) { let x; if (0) { var x; } }` is a SyntaxError
* that `catch (x) { try {} catch (y) { var x; } }` is allowed in a function, iff there's no `let x` at function scope

Better add a Reflect.parse test too :(
Attachment #8688282 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 4

4 years ago
Assigning to Shu-yu since he has a patch.
Assignee: nobody → shu
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #3)

> * that `catch (x) { for (var x in obj); }` is allowed

This pointed out a bug. Because I synthesize a second 'var' name in the declaration list, the AST for the 'var x' LHS no longer looks like a single declaration (i.e., it is !isValidForStatementLHS), and errors out.

Looks like I have to bite the bullet and redo how we emit declarations in the frontend so I don't need the "synthesize a new name for global and eval scripts" trick.
(Assignee)

Comment 6

4 years ago
I had also missed that Annex B.3.5 does not apply to for-of loop heads. I'm blocking this bug on Waldo's for head parser refactor.
Depends on: 1233249
(Assignee)

Comment 7

3 years ago
Applied all the comments. Could use another look.
Attachment #8704454 - Flags: review?(jorendorff)
(Assignee)

Updated

3 years ago
Attachment #8688282 - Attachment is obsolete: true
Comment on attachment 8704454 [details] [diff] [review]
Implement ES6 Annex B.3.5.

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

r=me.

::: js/src/frontend/Parser.cpp
@@ +3914,5 @@
> +            //
> +            // try {} catch (e) { var e = 42; }
> +            //
> +            // While a new var 'e' is declared, the initializer '= 42' needs
> +            // to is assigned to the lexically bound catch parameter

"needs to be assigned"

@@ +3975,5 @@
>                                     JSMSG_REDECLARED_CATCH_IDENTIFIER, bytes.ptr())
>                    : parser->report(reporter, false, pn, JSMSG_REDECLARED_VAR,
>                                     Definition::kindString(dn_kind), bytes.ptr())))
>              {
> +                 return false;

Micro-nit: I think this is indented an extra space.

::: js/src/tests/ecma_6/LexicalEnvironment/var-in-catch-body-annex-b.js
@@ +2,5 @@
> +
> +var log = "";
> +
> +try {
> +  evaluate(`

The test needs to bail out if evaluate isn't defined, right?

@@ +64,5 @@
> +} catch (e) {
> +  log += "e";
> +}
> +
> +assertEq(log, "e42local-xe");

I find this kind of testing pretty hard to follow, unless you're actually testing the particular path taken through some code.

This assertion, I guess, is testing that exactly one of the two previous tests results in an exception, but we don't care which? Why not just use assertThrowsInstanceOf(fn, SyntaxError)? It's defined in ../shell.js.
Attachment #8704454 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 9

3 years ago
So we can't report early errors early enough due to lazy parsing. What's our stance on this?

js> function f() { try {} catch (e) { for (var e of {}); } }
js> f()
typein:2:43 SyntaxError: redeclaration of identifier 'e' in catch:
typein:2:43 () { try {} catch (e) { for (var e of {}); } }
typein:2:43 .................................^
Stack:
  @typein:3:1
(Assignee)

Comment 10

3 years ago
(In reply to Shu-yu Guo [:shu] from comment #9)
> So we can't report early errors early enough due to lazy parsing. What's our
> stance on this?
> 
> js> function f() { try {} catch (e) { for (var e of {}); } }
> js> f()
> typein:2:43 SyntaxError: redeclaration of identifier 'e' in catch:
> typein:2:43 () { try {} catch (e) { for (var e of {}); } }
> typein:2:43 .................................^
> Stack:
>   @typein:3:1

Our stance is: have to abort lazy parse when parsing for-of with var inside catch blocks. Just ugh.
(Assignee)

Comment 11

3 years ago
Just when I thought I was done. Still needs to handle direct evals introducing var bindings inside of catch blocks.
(Assignee)

Comment 12

3 years ago
Will land without direct eval support first, then deal with direct evals later.

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9aab45404d97
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 15

3 years ago
Attachment #8717194 - Flags: review?(jorendorff)
(Assignee)

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

3 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]

Comment 16

3 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 815d689a6e1e).
Comment on attachment 8717194 [details] [diff] [review]
Implement ES6 Annex B.3.5 for direct eval.

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

r=me with one question

::: js/src/vm/ScopeObject.h
@@ +276,5 @@
> +                         PrivateUint32Value(offset << LocalOffsetShift));
> +    }
> +
> +    void setLocalOffsetToInvalid() {
> +        setLocalOffset(LOCALNO_LIMIT - 1);

Is this value really invalid for a local offset? What code checks this and rejects it or asserts that this value isn't there?

If this set-it-to-something-invalid thing is worth doing at all, it's worth using a real invalid value. Sorry if I'm missing the point here...
Attachment #8717194 - Flags: review?(jorendorff) → review+

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee1963e67cdd
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Reporter)

Updated

3 years ago
Depends on: 1250192
You need to log in before you can comment on or make changes to this bug.