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?

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

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.

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 :(

Assigning to Shu-yu since he has a patch.

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

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.

Applied all the comments. Could use another look.

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.

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

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

Just when I thought I was done. Still needs to handle direct evals introducing var bindings inside of catch blocks.

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

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...