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

VERIFIED FIXED

Status

()

defect
--
critical
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: gkw, Assigned: jimb)

Tracking

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

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(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()
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
Blocks: 554955
Assignee: general → jimb
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.
Attachment #520559 - Flags: review?(jorendorff)
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.
I don't think this has any bearing on the appropriateness of the fix, though.
(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.
(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 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.
Attachment #520559 - Flags: review?(jorendorff) → review+
Can the patch here please be landed on TM? (hoping to avoid bitrot)
blocking-fx: --- → ?
Will do, just re-testing now.
http://hg.mozilla.org/tracemonkey/rev/1e5ded29dd9f
Whiteboard: fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking-fx: ? → ---
Flags: in-testsuite+
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.