Last Comment Bug 668438 - Crash [@ MakePlaceholder] or [@ LeaveFunction] or "Assertion failure: !p,"
: Crash [@ MakePlaceholder] or [@ LeaveFunction] or "Assertion failure: !p,"
Status: VERIFIED FIXED
[fixed-in-tracemonkey]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Jim Blandy :jimb
:
:
Mentors:
Depends on:
Blocks: jsfunfuzz 627859
  Show dependency treegraph
 
Reported: 2011-06-29 21:01 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-10 16:36 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack (9.20 KB, text/plain)
2011-06-29 21:01 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Let MakePlaceholder's callers put the placeholder in the lexdeps table, as that needs to be done differently in different cases. (3.95 KB, patch)
2011-07-01 10:39 PDT, Jim Blandy :jimb
cdleary: review+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2011-06-29 21:01:26 PDT
Created attachment 543051 [details]
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] 2011-06-30 01:42:40 PDT
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 2011-06-30 11:48:50 PDT
I can reproduce this.
Comment 3 Jim Blandy :jimb 2011-07-01 10:39:07 PDT
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 2011-07-01 10:39:40 PDT
Created 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.
Comment 5 Chris Leary [:cdleary] (not checking bugmail) 2011-07-06 15:48:57 PDT
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 2011-07-06 21:27:30 PDT
http://hg.mozilla.org/tracemonkey/rev/104b182daf70
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2011-07-12 21:17:15 PDT
(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) 2011-07-17 17:31:51 PDT
http://hg.mozilla.org/mozilla-central/rev/569a960b4a64
Comment 9 Christian Holler (:decoder) 2013-01-04 13:18:38 PST
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] 2013-01-10 16:36:47 PST
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.

Note You need to log in before you can comment on or make changes to this bug.