567152 - "Assertion failure: !generic(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

delete this.uneval
function g(code) {
    var f = new Function(code)
    try {
        f()
    } catch (r) {}
    delete this.uneval
}
g("\
  [\
    []\
  ].some(function (a) {\
    this.w = function () {\
      a\
    }\
  })\
")

asserts js debug shell without -j on TM tip at Assertion failure: !generic(), at ../jsscope.h:426

Comment 1

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

Tested on 64-bit Linux.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   41518:539d04cccb8b
user:        Jason Orendorff
date:        Fri Apr 30 08:29:04 2010 -0500
summary:     Bug 560998 - Assertion failure: entry->vword.toObject() == JSVAL_TO_OBJECT(v). r=brendan.

Comment 2

[Jason Orendorff [:jorendorff]]

function c(a) {                  // make the global GENERIC
    this.w = function () { a; };
}
c(0);                            // after this, the global scope has both
                                 // BRANDED and GENERIC bits set
delete this.uneval;
delete this.uneval;              // JS_ResolveStandardClass redefines uneval
                                 // (in order to delete it again), triggering bug

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

It is a little weird that branded() asserts this way instead of just testing both bits. But ok.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

The question, really, is what should happen when we reach JSOP_UNBRAND or UNBRANDTHIS and the operand is already branded. Here is my guess.

I think overflowing rt->shapeGen here is harmless, or at least there's nothing to be done about it. I can comment that if it would help understanding.

Comment 5

[Brendan Eich [:brendan]]

Even simpler: once branded, there's no point in being generic, since the shape has already evolved unpredictably (only singleton objects are cacheable). So how about restricting state evolution to

 {} -> {branded} -(would-be generic)->{branded}

or

 {} -> {generic} -(would-be branded)->{generic}

/be

Comment 6

[Brendan Eich [:brendan]]

Jason, should we stall this while I work on bug 558451? Did you plan to do something like what is proposed in comment 5?

/be

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v3

Sorry. The weekend was crazy, and I was on a plane all yesterday.

I think we should push this unless it'll cause merge pains for you.

Comment 8

[Brendan Eich [:brendan]]

Don't worry about the merge pain, but please do respond to the proposed reduced state machine in comment 5.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

That's what v3 is, I think.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 447339 [details] [diff] [review]
v3

Thanks, missed v3 for some reason there...

/be

Comment 11

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

Just helping out...

http://hg.mozilla.org/tracemonkey/rev/331702c6884f

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/331702c6884f

Comment 13

[Christian Holler (:decoder)]

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

Comment 14

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

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