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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: gkw, Assigned: jimb)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
x86
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
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.
Crash Signature: [@ MakePlaceholder] [@ LeaveFunction]
Summary: "Assertion failure: !p," → Crash [@ MakePlaceholder] or [@ LeaveFunction] or "Assertion failure: !p,"
(Assignee)

Updated

6 years ago
Assignee: general → jimb
(Assignee)

Comment 2

6 years ago
I can reproduce this.
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
Attachment #543471 - Flags: review?(cdleary)
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.
Attachment #543471 - Flags: review?(cdleary) → review+
(Assignee)

Comment 6

6 years ago
http://hg.mozilla.org/tracemonkey/rev/104b182daf70
(Assignee)

Updated

6 years ago
Whiteboard: fixed-in-tracemonkey
(Reporter)

Comment 7

6 years ago
(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.
Whiteboard: fixed-in-tracemonkey → [inbound][fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/569a960b4a64
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound][fixed-in-tracemonkey] → [fixed-in-tracemonkey]
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-668438.js.
Flags: in-testsuite+
(Reporter)

Comment 10

4 years ago
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.