496245 - "Assertion failure: fun->u.i.script->upvarsOffset"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

(function(a){ 1(function(){delete a;}); })();

Assertion failure: fun->u.i.script->upvarsOffset, at ../jsfun.cpp:2191

Expected something along the lines of "TypeError: 1 is not a function".

Testing TM branch.  Happens with or without -j.

Comment 1

[Brendan Eich [:brendan]]

Sorry, dmandelin -- I gave you a bum steer on this one. The emitter can optimize away certain uses of upvars, resulting in flat closures with zero upvarsOffsets. I didn't want to mess with this edge case by trying to revise the "kind" of function from flat to null, safely. The check should go back. I'll patch now.

This is at least wanted.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix, slightly faster than old way

Use the faster fun->u.i.nupvars rather than fun->u.i.script->upvarsOffset, but assert that things are consistent no matter the value of the latter.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 381462 [details] [diff] [review]
fix, slightly faster than old way

This fixes the first assertion for me, but I still assert in js_NewFlatClosure trying to get script upvars out of a script that has no upvars. But more, the outer function, containing 'a' is not made heavyweight, so we end up deleting the wrong property:

js> a = 10
10
js> a
10
js> (function(a){ (function(){delete a;})(); })();
js> a
typein:7: ReferenceError: a is not defined

the 'delete' should be a no-op there.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(That was with a fix for the second assertion too.)

Comment 5

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

autoBisect shows this is probably related to bug 494269 :

The first bad revision is:
changeset:   28896:a16ed38ff63a
user:        David Mandelin
date:        Wed Jun 03 11:19:20 2009 -0700
summary:     Bug 494269: trace JSOP_LAMBDA_FC, r=brendan,gal

Comment 6

[Brendan Eich [:brendan]]

If 494269 is not yet on 1.9.1 or even wanted1.9.1+, then this bug should not be wanted1.9.1.

That delete should be a no-op. Blake, is the bug in comment 3 independent of bug 494269 (the JITting of JSOP_LAMBDA_FC)? If so, please spin out.

/be

Comment 7

[Robert Sayre]

Oh, right. Sorry about that.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> That delete should be a no-op. Blake, is the bug in comment 3 independent of
> bug 494269 (the JITting of JSOP_LAMBDA_FC)? If so, please spin out.

I filed bug 496422 on that. This fix (as it stands) though is still incomplete without:

@@ -2205,18 +2207,21 @@ js_AllocFlatClosure(JSContext *cx, JSFun
 
 JS_DEFINE_CALLINFO_3(extern, OBJECT, js_AllocFlatClosure,
                      CONTEXT, FUNCTION, OBJECT, 0, 0)
 
 JSObject *
 js_NewFlatClosure(JSContext *cx, JSFunction *fun)
 {
     JSObject *closure = js_AllocFlatClosure(cx, fun, cx->fp->scopeChain);
+    JSScript *script = fun->u.i.script;
+    if (!closure || script->upvarsOffset == 0)
+        return closure;
 
-    JSUpvarArray *uva = JS_SCRIPT_UPVARS(fun->u.i.script);
+    JSUpvarArray *uva = JS_SCRIPT_UPVARS(script);
     JS_ASSERT(uva->length <= size_t(closure->dslots[-1]));
 
     uintN level = fun->u.i.script->staticLevel;
     for (uint32 i = 0, n = uva->length; i < n; i++)
         closure->dslots[i] = js_GetUpvar(cx, level, uva->vector[i]);
 
     return closure;
 }

or similar.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Full patch

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 381608 [details] [diff] [review]
Full patch

This needs to land to avoid stopping fuzzers.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/594138cd96e3

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/594138cd96e3

Comment 14

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929