Bug 493232 - Wrong variable value accessed in closure. r=brendan.
authorJason Orendorff <jorendorff@mozilla.com>
Wed, 27 May 2009 15:46:03 -0500
changeset 28814 f9ad6d736430d7faf3234c2aef928882c4b812e5
parent 28813 5b0eb5022f03a9f0e0f74da79c0a036bb20155ed
child 28815 b07ebbc4784e22123e5c1d0fbfb18a07a5efdb57
push id7273
push userrsayre@mozilla.com
push dateThu, 28 May 2009 22:52:43 +0000
treeherdermozilla-central@ac3e487c5fff [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbrendan
bugs493232
milestone1.9.2a1pre
Bug 493232 - Wrong variable value accessed in closure. r=brendan.
js/src/jsparse.cpp
js/src/jsparse.h
js/src/jsscript.h
--- a/js/src/jsparse.cpp
+++ b/js/src/jsparse.cpp
@@ -289,16 +289,23 @@ JSCompiler::newFunctionBox(JSObject *obj
     funbox->object = obj;
     funbox->node = fn;
     funbox->siblings = tc->functionList;
     tc->functionList = funbox;
     ++tc->compiler->functionCount;
     funbox->kids = NULL;
     funbox->parent = tc->funbox;
     funbox->queued = false;
+    funbox->inLoop = false;
+    for (JSStmtInfo *stmt = tc->topStmt; stmt; stmt = stmt->down) {
+        if (STMT_IS_LOOP(stmt)) {
+            funbox->inLoop = true;
+            break;
+        }
+    }
     funbox->level = tc->staticLevel;
     funbox->tcflags = TCF_IN_FUNCTION | (tc->flags & TCF_COMPILE_N_GO);
     return funbox;
 }
 
 void
 JSCompiler::trace(JSTracer *trc)
 {
@@ -739,16 +746,18 @@ JSCompiler::parse(JSObject *chain)
         } else {
             if (!js_FoldConstants(context, pn, &tc))
                 pn = NULL;
         }
     }
     return pn;
 }
 
+JS_STATIC_ASSERT(FREE_STATIC_LEVEL == JS_BITMASK(JSFB_LEVEL_BITS));
+
 static inline bool
 SetStaticLevel(JSTreeContext *tc, uintN staticLevel)
 {
     /*
      * Reserve FREE_STATIC_LEVEL (0xffff) in order to reserve FREE_UPVAR_COOKIE
      * (0xffffffff) and other cookies with that level.
      *
      * This is a lot simpler than error-checking every MAKE_UPVAR_COOKIE, and
@@ -1951,60 +1960,65 @@ JSCompiler::setFunctionKinds(JSFunctionB
                         JSFunctionBox *afunbox = funbox;
                         uintN lexdepLevel = lexdep->frameLevel();
 
                         JS_ASSERT(lexdepLevel <= funbox->level);
                         while (afunbox->level != lexdepLevel) {
                             afunbox = afunbox->parent;
 
                             /*
-                             * afunbox cannot be null here. That is, we are
-                             * sure to find a function box whose level ==
-                             * lexdepLevel before walking off the top of the
-                             * funbox tree.
+                             * afunbox can't be null because we are sure
+                             * to find a function box whose level == lexdepLevel
+                             * before walking off the top of the funbox tree.
+                             * See bug 493260 comments 16-18.
                              *
-                             * Proof: lexdepLevel is at least the base
-                             * staticLevel for this compilation (often 0 but
-                             * nonzero when compiling for local eval) and at
-                             * most funbox->level. The path we are walking
-                             * includes one function box each of precisely that
-                             * range of levels.
-                             *
-                             * Assert but check anyway (bug 493260 comment 16).
+                             * Assert but check anyway, to check future changes
+                             * that bind eval upvars in the parser.
                              */
                             JS_ASSERT(afunbox);
 
                             /*
                              * If this function is reaching up across an
                              * enclosing funarg, we cannot make a flat
                              * closure. The display stops working once the
                              * funarg escapes.
                              */
                             if (!afunbox || afunbox->node->isFunArg())
                                 goto break2;
                         }
 
                         /*
+                         * If afunbox's function (which is at the same level as
+                         * lexdep) is in a loop, pessimistically assume the
+                         * variable initializer may be in the same loop. A flat
+                         * closure would then be unsafe, as the captured
+                         * variable could be assigned after the closure is
+                         * created. See bug 493232.
+                         */
+                        if (afunbox->inLoop)
+                            break;
+
+                        /*
                          * with and eval defeat lexical scoping; eval anywhere
                          * in a variable's scope can assign to it. Both defeat
                          * the flat closure optimization. The parser detects
                          * these cases and flags the function heavyweight.
                          */
                         JSFunctionBox *parentbox = afunbox->parent ? afunbox->parent : afunbox;
                         if (parentbox->tcflags & TCF_FUN_HEAVYWEIGHT)
                             break;
 
                         /*
-                         * If afunbox's function (which is at the same level as
-                         * lexdep) is not a lambda, it will be hoisted, so it
-                         * could capture the undefined value that by default
-                         * initializes var/let/const bindings. And if lexdep is
-                         * a function that comes at (meaning a function refers
-                         * to its own name) or strictly after afunbox, we also
-                         * break to defeat the flat closure optimization.
+                         * If afunbox's function is not a lambda, it will be
+                         * hoisted, so it could capture the undefined value
+                         * that by default initializes var/let/const
+                         * bindings. And if lexdep is a function that comes at
+                         * (meaning a function refers to its own name) or
+                         * strictly after afunbox, we also break to defeat the
+                         * flat closure optimization.
                          */
                         JSFunction *afun = (JSFunction *) afunbox->object;
                         if (!(afun->flags & JSFUN_LAMBDA)) {
                             if (lexdep->isBindingForm())
                                 break;
                             if (lexdep->pn_pos >= afunbox->node->pn_pos)
                                 break;
                         }
@@ -6079,25 +6093,32 @@ class CompExprTransplanter {
     bool transplant(JSParseNode *pn);
 };
 
 /*
  * Any definitions nested within the comprehension expression of a generator
  * expression must move "down" one static level, which of course increases the
  * upvar-frame-skip count.
  */
-static void
+static bool
 BumpStaticLevel(JSParseNode *pn, JSTreeContext *tc)
 {
     if (pn->pn_cookie != FREE_UPVAR_COOKIE) {
         uintN level = UPVAR_FRAME_SKIP(pn->pn_cookie) + 1;
 
         JS_ASSERT(level >= tc->staticLevel);
+        if (level >= FREE_STATIC_LEVEL) {
+            JS_ReportErrorNumber(tc->compiler->context, js_GetErrorMessage, NULL,
+                                 JSMSG_TOO_DEEP, js_function_str);
+            return false;
+        }
+
         pn->pn_cookie = MAKE_UPVAR_COOKIE(level, UPVAR_FRAME_SLOT(pn->pn_cookie));
     }
+    return true;
 }
 
 static void
 AdjustBlockId(JSParseNode *pn, uintN adjust, JSTreeContext *tc)
 {
     JS_ASSERT(pn->pn_arity == PN_LIST || pn->pn_arity == PN_FUNC || pn->pn_arity == PN_NAME);
     pn->pn_blockid += adjust;
     if (pn->pn_blockid >= tc->blockidGen)
@@ -6168,18 +6189,18 @@ CompExprTransplanter::transplant(JSParse
       }
 
       case PN_NAME:
         transplant(pn->maybeExpr());
         if (pn->pn_arity == PN_FUNC)
             --funcLevel;
 
         if (pn->pn_defn) {
-            if (genexp)
-                BumpStaticLevel(pn, tc);
+            if (genexp && !BumpStaticLevel(pn, tc))
+                return false;
         } else if (pn->pn_used) {
             JS_ASSERT(pn->pn_op != JSOP_NOP);
             JS_ASSERT(pn->pn_cookie == FREE_UPVAR_COOKIE);
 
             JSDefinition *dn = pn->pn_lexdef;
             JS_ASSERT(dn->pn_defn);
 
             /*
@@ -6187,41 +6208,41 @@ CompExprTransplanter::transplant(JSParse
              * to the left of the root node, and if pn is the last use visited
              * in the comprehension expression (to avoid adjusting the blockid
              * multiple times).
              *
              * Non-placeholder definitions within the comprehension expression
              * will be visited further below.
              */
             if (dn->isPlaceholder() && dn->pn_pos >= root->pn_pos && dn->dn_uses == pn) {
-                if (genexp)
-                    BumpStaticLevel(dn, tc);
+                if (genexp && !BumpStaticLevel(dn, tc))
+                    return false;
                 AdjustBlockId(dn, adjust, tc);
             }
 
             JSAtom *atom = pn->pn_atom;
 #ifdef DEBUG
             JSStmtInfo *stmt = js_LexicalLookup(tc, atom, NULL);
             JS_ASSERT(!stmt || stmt != tc->topStmt);
 #endif
             if (genexp && PN_OP(dn) != JSOP_CALLEE) {
                 JS_ASSERT(!tc->decls.lookup(atom));
 
                 if (dn->pn_pos < root->pn_pos || dn->isPlaceholder()) {
                     JSAtomListElement *ale = tc->lexdeps.add(tc->compiler, dn->pn_atom);
                     if (!ale)
-                        return NULL;
+                        return false;
 
                     if (dn->pn_pos >= root->pn_pos) {
                         tc->parent->lexdeps.remove(tc->compiler, atom);
                     } else {
                         JSDefinition *dn2 = (JSDefinition *)
                             NewNameNode(tc->compiler->context, TS(tc->compiler), dn->pn_atom, tc);
                         if (!dn2)
-                            return NULL;
+                            return false;
 
                         dn2->pn_type = dn->pn_type;
                         dn2->pn_pos = root->pn_pos;
                         dn2->pn_defn = true;
                         dn2->pn_dflags |= PND_PLACEHOLDER;
 
                         JSParseNode **pnup = &dn->dn_uses;
                         JSParseNode *pnu;
--- a/js/src/jsparse.h
+++ b/js/src/jsparse.h
@@ -747,24 +747,27 @@ JSParseNode::setFunArg()
 }
 
 struct JSObjectBox {
     JSObjectBox         *traceLink;
     JSObjectBox         *emitLink;
     JSObject            *object;
 };
 
+#define JSFB_LEVEL_BITS 14
+
 struct JSFunctionBox : public JSObjectBox
 {
     JSParseNode         *node;
     JSFunctionBox       *siblings;
     JSFunctionBox       *kids;
     JSFunctionBox       *parent;
     uint32              queued:1,
-                        level:15,
+                        inLoop:1,               /* in a loop in parent function */
+                        level:JSFB_LEVEL_BITS,
                         tcflags:16;
 };
 
 struct JSFunctionBoxQueue {
     JSFunctionBox       **vector;
     size_t              head, tail;
     size_t              lengthMask;
 
--- a/js/src/jsscript.h
+++ b/js/src/jsscript.h
@@ -82,17 +82,17 @@ typedef struct JSObjectArray {
 } JSObjectArray;
 
 typedef struct JSUpvarArray {
     uint32          *vector;    /* array of indexed upvar cookies */
     uint32          length;     /* count of indexed upvar cookies */
 } JSUpvarArray;
 
 #define CALLEE_UPVAR_SLOT               0xffff
-#define FREE_STATIC_LEVEL               0xffff
+#define FREE_STATIC_LEVEL               0x3fff
 #define FREE_UPVAR_COOKIE               0xffffffff
 #define MAKE_UPVAR_COOKIE(skip,slot)    ((skip) << 16 | (slot))
 #define UPVAR_FRAME_SKIP(cookie)        ((uint32)(cookie) >> 16)
 #define UPVAR_FRAME_SLOT(cookie)        ((uint16)(cookie))
 
 #define JS_OBJECT_ARRAY_SIZE(length)                                          \
     (offsetof(JSObjectArray, vector) + sizeof(JSObject *) * (length))