In the attached test.html file, the expected error is "TypeError: baz is null", but starting in Nightly 36, the console reports "TypeError: foo is null" or "TypeError: bar is null" depending on seemingly unrelated declarations of variable `one` and functions `foo` and `bar` in the same <script> block: * The expected error is "TypeError: baz is null", but test.html reports "TypeError: bar is null". * If I change `const one = 1` to `var one = 1`, then "TypeError: bar is null" becomes "TypeError: foo is null". * If I remove the IIFE, then "TypeError: bar is null" goes away and we get the expected "TypeError: baz is null". * If I change bar() to return 1 instead of calling foo(), then "TypeError: bar is null" goes away and we get the expected "TypeError: baz is null". * If I change `baz.value` to just `baz`, then "TypeError: bar is null" goes away and we get the expected "TypeError: baz is null". Jan: could this be a regression from bug 1086842? I bisected this regression to Nightly 36 build 2014-10-30. There aren't many suspicious changesets in this pushlog. (I haven't bisected a narrower regression range on mozilla-inbound because of some mozregression.py bugs.) https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7e3c85754d32&tochange=80e18ff7c7b2

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f7c01f0ac64b&tochange=75de7e0fe086 Triggered by: 75de7e0fe086 Jan de Mooij — Bug 1090491 - Don't allocate stack slots for aliased locals. r=luke

Thanks, Alice! [Tracking Requested - why for this release]: This bug is a SpiderMonkey regression that causes JS error messages to refer to the wrong identifier names.

The problem was that the expression decompiler did FillBindingVector and then used the local slot as index into this vector. This no longer works since only unaliased locals have slots assigned to them. I changed it to use BindingIter. I think this is OK as the expression decompiler is not perf-critical. Furthermore, I also killed FillBindingVector; most places where it was used can just use BindingIter directly to avoid an unnecessary copy/allocation.

Comment on attachment 8524607 [details] [diff] [review] Patch Review of attachment 8524607 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsopcode.cpp @@ +1462,5 @@ > * recursively on the operands' pcs. The result is a depth-first traversal of > * the expression tree. > * > */ > + Oops will remove the newline I added here.

Comment on attachment 8524607 [details] [diff] [review] Patch Review of attachment 8524607 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfun.cpp @@ +1078,4 @@ > return nullptr; > + if (i == unsigned(fun->nargs() - 1) && fun->hasRest() && !out.append("...")) > + return nullptr; > + if (!out.append(bi->name())) Hah, much nicer. ::: js/src/jsopcode.cpp @@ +1673,5 @@ > + MOZ_ASSERT(slot < script->bindings.numArgs()); > + > + for (BindingIter bi(script); bi; bi++) { > + MOZ_ASSERT(bi->kind() == Binding::ARGUMENT); > + if (bi.frameIndex() == slot) I wonder if we could avoid embedding the implicit assumption that arguments go before vars in all these different places (or is that already a widespread assumption?). Two alternatives would be: (1) explicitly adding a (bi->kind() == ARGUMENT) guard, (2) adding UnaliasedFormalIter/UnaliasedLocalIter that were refinements of BindingIter. Also: it seems like frameIndex should only be called on unaliased bindings.

(In reply to Luke Wagner [:luke] from comment #5) > I wonder if we could avoid embedding the implicit assumption that arguments > go before vars in all these different places (or is that already a > widespread assumption?). I think so; it's also documented (BindingIter comment): * Iterator over a script's bindings (formals and variables). * The order of iteration is: * - first, formal arguments, from index 0 to numArgs * - next, variables, from index 0 to numLocals > Two alternatives would be: (1) explicitly adding a > (bi->kind() == ARGUMENT) guard, (2) adding > UnaliasedFormalIter/UnaliasedLocalIter that were refinements of BindingIter. Some places will need to see both unaliased and aliased formals, so we'd need FormalIter as well... > Also: it seems like frameIndex should only be called on unaliased bindings. I can change it to argIndex() but for arguments they do exactly the same thing.

Comment on attachment 8524607 [details] [diff] [review] Patch From irc: good points, r+ with change to argIndex and a beefy comment explaining why argIndex is valid for aliased and unaliased args.

Tracking (cf comment #2)