1100623 - TypeError reports different, incorrect identifier names depending on unrelated JS declarations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: test.html

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

Comment 1

[Alice0775 White]

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

Comment 2

[Chris Peterson [:cpeterson]]

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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

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 4

[Jan de Mooij [:jandem]]

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 5

[Luke Wagner [:luke]]

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.

Comment 6

[Jan de Mooij [:jandem]]

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

[Luke Wagner [:luke]]

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.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Tracking (cf comment #2)

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/beb6d3e078d0

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/beb6d3e078d0