Closed Bug 1100623 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- unaffected
firefox36 + fixed

People

(Reporter: cpeterson, Assigned: jandem)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file 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
Flags: needinfo?(jdemooij)
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.
Assignee: nobody → jdemooij
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
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.
Attachment #8524607 - Flags: review?(luke)
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.
Attachment #8524607 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/beb6d3e078d0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.