643222 - "Assertion failure: !obj->isCall(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(function () {
    eval("var x=delete(x)")
})()


asserts js debug shell on TM changeset c811be25eaad without -m nor -j nor -a at Assertion failure: !obj->isCall()

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   63095:67b102d581dd
user:        Jim Blandy
date:        Tue Mar 15 12:18:36 2011 -0700
summary:     Bug 554955: Give blocks and call objects unique shapes when they have parents that may be extended with new bindings. r=jorendorff

Comment 2

[Jim Blandy :jimb]

Attachment: Don't assert that we never cached adds to call objects.

This assertion should not have been added. Although most adds to call
objects are done by DEFVAR operations, which don't create property cache
entries, the test case shows a situation in which a SETNAME, which is
cached, does the add. Since the object whose property should receive the
value of an assignment's RHS is chosen before the RHS is evaluated, it is
correct to put the value of the 'delete' expression on the call.

It would be nice to ensure that this call object, which we know we are
adding bindings to, had been recognized as extensible at compile time by
the analysis added in bug 554955. However, we only set "extensible parents"
flag on the bindings of calls whose *parents* are extensible, not the
extensible calls themselves. So there isn't anything convenient to check
here.

Comment 3

[Jim Blandy :jimb]

Something was bugging me about what I'd written; perhaps I've found a bug in the ECMAScript spec.

When executing this code:

(function () {
    eval("var x=delete(x)")
})()

- On entry to the function, we create a lexical environment with a declarative environment record (10.4.3).

- When we enter the eval code, we create the binding for 'x' in that environment record  (10.5, step 8.c.1). This is a deletable binding, as per 10.5 step 2.

- When we evaluate the declaration, we produce a reference with that environment record as its base, delete the binding, and then call PutValue (12.2, VariableDeclaration : Identifier Initialiser semantics).

- PutValue calls the environment record's SetMutableBinding concrete method (8.7.2 step 5.a).

- SetMutableBinding's assertion fails, because 'x' is no longer bound in that environment record (10.2.1.1.3 step 2).

I'll ask about this on es-discuss.

Comment 4

[Jim Blandy :jimb]

I don't think this has any bearing on the appropriateness of the fix, though.

Comment 5

[Allen Wirfs-Brock]

(In reply to comment #3)
I believe that has already been discussed.  I'll have to search for thread but I'm pretty sure that the conclusion is that it's weird but an unavoidable consequence to various invariants we want to maintain.

Comment 6

[Jim Blandy :jimb]

(In reply to comment #5)
> (In reply to comment #3)
> I believe that has already been discussed.  I'll have to search for thread but
> I'm pretty sure that the conclusion is that it's weird but an unavoidable
> consequence to various invariants we want to maintain.

Yes --- Waldo found it, and you commented on it here:

https://mail.mozilla.org/pipermail/es5-discuss/2010-November/003839.html

So the patch is definitely right.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 520559 [details] [diff] [review]
Don't assert that we never cached adds to call objects.

Yep. It's good to have that test in the suite. Too bad we didn't have it before.

Comment 8

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

Can the patch here please be landed on TM? (hoping to avoid bitrot)

Comment 9

[Jim Blandy :jimb]

Will do, just re-testing now.

Comment 10

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/1e5ded29dd9f

Comment 11

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

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/1e5ded29dd9f

Comment 12

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

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