668438 - Crash [@ MakePlaceholder] or [@ LeaveFunction] or "Assertion failure: !p,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

let(x = (x((
  function() {
    return {
      e: function() {
        e in x
      }
    }
  }))
for


asserts js debug shell on TM changeset c2e5e424e6c3 without any CLI arguments at Assertion failure: !p, tested on 64-bit Mac.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   71814:756abc094eb1
user:        Jim Blandy
date:        Wed Jun 29 02:11:08 2011 -0700
summary:     Bug 627859: Use the standard placeholder-making function when re-scoping variable references in generator 'yield' expressions. r=brendan

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function w() {}(function() {
    Reflect.parse("\
        for(let x=x(function(){\
            return{e:function(){for(e in x){}}}\
        }(x)for each(l in{})\
    ")
})()

crashes js opt shell at MakePlaceholder or LeaveFunction and asserts similarly.

Comment 2

[Jim Blandy :jimb]

I can reproduce this.

Comment 3

[Jim Blandy :jimb]

So, there's some very odd stuff going on when we parse this:

let(x = (x((
  function() {
    return {
      e: function() {
        e in x
      }
    }
  }))
for

There are three uses of x here:

- The first is represented by a definition. Good.

- The second is a placeholder. I guess the scope of a let binding doesn't include its initializer, so that's fine.

- The third is parsed as a use of the first, which makes no sense whatsoever.

However, my refactoring isn't the cause of this: it only causes it to be noticed, because it changes a call to lexdeps->put in transplant into a call to lexdeps->add in MakePlaceholder, and lexdeps->add doesn't expect to replace an existing entry.

I have a patch which simply restores the previous behavior, but this should be looked at.

Comment 4

[Jim Blandy :jimb]

Attachment: Let MakePlaceholder's callers put the placeholder in the lexdeps table, as that needs to be done differently in different cases.

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 543471 [details] [diff] [review]
Let MakePlaceholder's callers put the placeholder in the lexdeps table, as that needs to be done differently in different cases.

Let's get this into aurora once the 7/5 TM merge lands on aurora and file a followup bug for comment 3.

Comment 6

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/104b182daf70

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #6)
> http://hg.mozilla.org/tracemonkey/rev/104b182daf70

Somehow this didn't get propagated to mozilla-inbound via mozilla-central? Was this left out?

http://hg.mozilla.org/integration/mozilla-inbound/rev/104b182daf70 gives a no match found error.

Comment 8

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/569a960b4a64

Comment 9

[Christian Holler (:decoder)]

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

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.