600067 - fun->u.i.names is incorrect when a local function shadows an argument

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Reduced from test case in bug 597408

// vim: set ts=4 sw=4 tw=99 et:
X = function (a) {
   function b() {
      print(a);
   }
   function a() {
   }
   print(a);
}

Assertion failure: localKind == JSLOCAL_VAR || localKind == JSLOCAL_CONST, at ../jsemit.cpp:4541

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Err, sorry, actual test case is:

YCore[JY5] = function (a) {
  var W = _R,
      Z = YCore.Threads,
      V = U[AE7],
      Q = U[NV5];

  function a() {
  }
};

The problem is we duplicate "a" in the property tree. If there are <=5 local variables, it works, because the linear search along the ancestor line hits the local variable "a" first.

If in dictionary mode, it hits the argument "a" first.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

b7+ to fix Yahoo Mail!

Comment 3

[Nochum Sossonko [:Natch]]

(In reply to comment #1)
> The problem is we duplicate "a" in the property tree. If there are <=5 local
> variables, it works, because the linear search along the ancestor line hits the
> local variable "a" first.

This was very frustrating at first since I couldn't tell exactly what was triggering the error (it wouldn't happen if I removed random functions).

The interesting thing though is, if I'd replace those letter-number combinations (which are really just shorthands for strings), the error would say: "void(0) is undefined" which was really comical if you ask me :)

Comment 4

[Jeff Walden [:Waldo]]

A little more copy-pastable:

[jwalden@find-waldo-now tests]$ echo '
function s(a)
{
  var b, c, d, e;
  function a() { }
}' | ../dbg/js -j
Assertion failure: localKind == JSLOCAL_VAR || localKind == JSLOCAL_CONST, at ../jsemit.cpp:4541
Aborted (core dumped)

Comment 5

[Jesse Ruderman]

The first bad revision is:
changeset:   3dc4daa9621b
user:        Brendan Eich
date:        Mon Sep 13 18:44:34 2010 -0700
summary:     JSFunction::addLocal never calls Shape::maybeHash (bug 595918, r=jorendorff).

Comment 6

[Jesse Ruderman]

jsfunfuzz can now hit this bug. It was missing the ability to create a large number of local variables, and it tended not to create functions declaration statements with aliasing/shadowing names.

Comment 7

[Brendan Eich [:brendan]]

Attachment: minimal fix

Comment 8

[Jeff Walden [:Waldo]]

How many places is js::PropertyTable used to store multiple shapes with the same id?  It seems very non-obvious to me that it would ever contain more than a single shape for an id.  Should some other structure more supportive of duplicate entries be used instead?

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> How many places is js::PropertyTable used to store multiple shapes with the
> same id?

No places, with this bug fixed.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: with test (plane landed)

Comment 11

[Jeff Walden [:Waldo]]

Mega-nit: making the argument not start off the alphabet makes it easier to see just how many local variables are necessary to trigger the behavior (so I found when I was typing it out locally doing my own investigation):

function s(e)
{
  var a, b, c, d;
  function e() { }
}

Might be worth making that tweak to the test before pushing, really doesn't matter if it doesn't happen tho.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 479607 [details] [diff] [review]
with test (plane landed)

r=me for the minimal fix.

It seems like it would be better for the compiler not to create property lineages with duplicate ids. I imagine that would be more involved.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> Comment on attachment 479607 [details] [diff] [review]
> with test (plane landed)
> 
> r=me for the minimal fix.
> 
> It seems like it would be better for the compiler not to create property
> lineages with duplicate ids. I imagine that would be more involved.

Something has to, as in non-strict ES, duplicate parameters are allowed and they need to be decompiled as such, even though the last one wins so one could avoid allocating slots and shapes for all but the last.

In the pre-Shape code there was a fair amount of specialized code just to keep track of duplicates in a JSLocalNameMap. With shapes this simplifies to linking dups in reverse source order from fun->u.i.names (skipping upvars and vars to get to args) up to the first parameter.

Similarly for function binding trumping formal parameter, we could remove the param and append the duplicate-id  function as local var shape, but that is more code statically and more cycles than just allowing the duplication but keeping the last-one-wins rule when hashing.

/be

Comment 14

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/cdac8d48fcca

/be

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to comment #13)
> > It seems like it would be better for the compiler not to create property
> > lineages with duplicate ids. [...]
> 
> Something has to, as in non-strict ES, duplicate parameters are allowed and
> they need to be decompiled as such[...].

This is true.

decompiler_TCO++;

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/cdac8d48fcca

Comment 17

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-600067.js.