Bug 616294 - |delete x| inside a function in eval code, where that eval code includes |var x| at top level, actually does delete the binding for x. r=brendan
authorJeff Walden <jwalden@mit.edu>
Fri, 03 Dec 2010 14:54:52 -0800
changeset 59228 e1ef77fd860547d59834fe53b21411e674ab2324
parent 59227 1002cba2f2d6e7e8518546dfbc868468758ac9e0
child 59229 64e001d84e7ac2b60b38100f39bcaeb324211a43
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewersbrendan
bugs616294
milestone2.0b8pre
Bug 616294 - |delete x| inside a function in eval code, where that eval code includes |var x| at top level, actually does delete the binding for x. r=brendan
js/src/jsemit.cpp
js/src/jsparse.cpp
js/src/jsparse.h
js/src/tests/ecma_5/Expressions/jstests.list
js/src/tests/ecma_5/Expressions/nested-delete-name-in-evalcode.js
js/src/tests/ecma_5/extensions/jstests.list
js/src/tests/ecma_5/extensions/nested-delete-name-in-evalcode.js
--- a/js/src/jsemit.cpp
+++ b/js/src/jsemit.cpp
@@ -2156,31 +2156,25 @@ BindNameToSlot(JSContext *cx, JSCodeGene
 
     /*
      * Turn attempts to mutate const-declared bindings into get ops (for
      * pre-increment and pre-decrement ops, our caller will have to emit
      * JSOP_POS, JSOP_ONE, and JSOP_ADD as well).
      *
      * Turn JSOP_DELNAME into JSOP_FALSE if dn is known, as all declared
      * bindings visible to the compiler are permanent in JS unless the
-     * declaration originates in eval code. We detect eval code by testing
-     * cg->parser->callerFrame, which is set only by eval or a debugger
-     * equivalent.
-     *
-     * Note that this callerFrame non-null test must be qualified by testing
-     * !cg->funbox to exclude function code nested in eval code, which is not
-     * subject to the deletable binding exception.
+     * declaration originates at top level in eval code.
      */
     switch (op) {
       case JSOP_NAME:
       case JSOP_SETCONST:
         break;
       case JSOP_DELNAME:
         if (dn_kind != JSDefinition::UNKNOWN) {
-            if (cg->parser->callerFrame && !cg->funbox)
+            if (cg->parser->callerFrame && dn->isTopLevel())
                 JS_ASSERT(cg->compileAndGo());
             else
                 pn->pn_op = JSOP_FALSE;
             pn->pn_dflags |= PND_BOUND;
             return JS_TRUE;
         }
         break;
       default:
@@ -4574,21 +4568,16 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
              */
             JS_ASSERT(pn->pn_op == JSOP_NOP);
             JS_ASSERT(cg->inFunction());
             if (!EmitFunctionDefNop(cx, cg, pn->pn_index))
                 return JS_FALSE;
             break;
         }
 
-        JS_ASSERT_IF(cx->options & JSOPTION_ANONFUNFIX,
-                     pn->pn_defn ||
-                     (!pn->pn_used && !pn->isTopLevel()) ||
-                     (fun->flags & JSFUN_LAMBDA));
-
         JS_ASSERT_IF(pn->pn_funbox->tcflags & TCF_FUN_HEAVYWEIGHT,
                      FUN_KIND(fun) == JSFUN_INTERPRETED);
 
         /* Generate code for the function's body. */
         void *cg2mark = JS_ARENA_MARK(cg->codePool);
         void *cg2space;
         JS_ARENA_ALLOCATE_TYPE(cg2space, JSCodeGenerator, cg->codePool);
         if (!cg2space) {
--- a/js/src/jsparse.cpp
+++ b/js/src/jsparse.cpp
@@ -635,19 +635,19 @@ JSParseNode::newBinaryOrAppend(TokenKind
 
 namespace js {
 
 inline void
 NameNode::initCommon(JSTreeContext *tc)
 {
     pn_expr = NULL;
     pn_cookie.makeFree();
-    pn_dflags = tc->atTopLevel() ? PND_TOPLEVEL : 0;
-    if (!tc->topStmt || tc->topStmt->type == STMT_BLOCK)
-        pn_dflags |= PND_BLOCKCHILD;
+    pn_dflags = (!tc->topStmt || tc->topStmt->type == STMT_BLOCK)
+                ? PND_BLOCKCHILD
+                : 0;
     pn_blockid = tc->blockid();
 }
 
 NameNode *
 NameNode::create(JSAtom *atom, JSTreeContext *tc)
 {
     JSParseNode *pn;
 
@@ -1477,16 +1477,18 @@ Define(JSParseNode *pn, JSAtom *atom, JS
     }
 
     ale = tc->decls.add(tc->parser, atom, let ? JSAtomList::SHADOW : JSAtomList::UNIQUE);
     if (!ale)
         return false;
     ALE_SET_DEFN(ale, pn);
     pn->pn_defn = true;
     pn->pn_dflags &= ~PND_PLACEHOLDER;
+    if (!tc->parent)
+        pn->pn_dflags |= PND_TOPLEVEL;
     return true;
 }
 
 static void
 LinkUseToDef(JSParseNode *pn, JSDefinition *dn, JSTreeContext *tc)
 {
     JS_ASSERT(!pn->pn_used);
     JS_ASSERT(!pn->pn_defn);
@@ -1558,17 +1560,16 @@ MakeDefIntoUse(JSDefinition *dn, JSParse
             if (!lhs)
                 return NULL;
             //pn->dn_uses = lhs;
             dn = (JSDefinition *) lhs;
         }
 
         dn->pn_op = (js_CodeSpec[dn->pn_op].format & JOF_SET) ? JSOP_SETNAME : JSOP_NAME;
     } else if (dn->kind() == JSDefinition::FUNCTION) {
-        JS_ASSERT(dn->isTopLevel());
         JS_ASSERT(dn->pn_op == JSOP_NOP);
         dn->pn_type = TOK_NAME;
         dn->pn_arity = PN_NAME;
         dn->pn_atom = atom;
     }
 
     /* Now make dn no longer a definition, rather a use of pn. */
     JS_ASSERT(dn->pn_type == TOK_NAME);
@@ -2541,18 +2542,16 @@ LeaveFunction(JSParseNode *fn, JSTreeCon
 {
     JSTreeContext *tc = funtc->parent;
     tc->blockidGen = funtc->blockidGen;
 
     JSFunctionBox *funbox = fn->pn_funbox;
     funbox->tcflags |= funtc->flags & (TCF_FUN_FLAGS | TCF_COMPILE_N_GO | TCF_RETURN_EXPR);
 
     fn->pn_dflags |= PND_INITIALIZED;
-    JS_ASSERT_IF(tc->atTopLevel() && lambda == 0 && funAtom,
-                 fn->pn_dflags & PND_TOPLEVEL);
     if (!tc->topStmt || tc->topStmt->type == STMT_BLOCK)
         fn->pn_dflags |= PND_BLOCKCHILD;
 
     /*
      * Propagate unresolved lexical names up to tc->lexdeps, and save a copy
      * of funtc->lexdeps in a TOK_UPVARS node wrapping the function's formal
      * params and body. We do this only if there are lexical dependencies not
      * satisfied by the function's declarations, to avoid penalizing functions
@@ -2932,53 +2931,46 @@ Parser::functionDef(JSAtom *funAtom, Fun
                 pn = fn;
             }
 
             if (!Define(pn, funAtom, tc))
                 return NULL;
         }
 
         /*
-         * A function nested at top level inside another's body needs only a
-         * local variable to bind its name to its value, and not an activation
-         * object property (it might also need the activation property, if the
-         * outer function contains with statements, e.g., but the stack slot
-         * wins when jsemit.c's BindNameToSlot can optimize a JSOP_NAME into a
+         * A function directly inside another's body needs only a local
+         * variable to bind its name to its value, and not an activation object
+         * property (it might also need the activation property, if the outer
+         * function contains with statements, e.g., but the stack slot wins
+         * when jsemit.cpp's BindNameToSlot can optimize a JSOP_NAME into a
          * JSOP_GETLOCAL bytecode).
          */
-        if (topLevel) {
-            pn->pn_dflags |= PND_TOPLEVEL;
-
-            if (tc->inFunction()) {
-                JSLocalKind localKind;
-                uintN index;
-
-                /*
-                 * Define a local in the outer function so that BindNameToSlot
-                 * can properly optimize accesses. Note that we need a local
-                 * variable, not an argument, for the function statement. Thus
-                 * we add a variable even if a parameter with the given name
-                 * already exists.
-                 */
-                localKind = tc->fun()->lookupLocal(context, funAtom, &index);
-                switch (localKind) {
-                  case JSLOCAL_NONE:
-                  case JSLOCAL_ARG:
-                    index = tc->fun()->u.i.nvars;
-                    if (!tc->fun()->addLocal(context, funAtom, JSLOCAL_VAR))
-                        return NULL;
-                    /* FALL THROUGH */
-
-                  case JSLOCAL_VAR:
-                    pn->pn_cookie.set(tc->staticLevel, index);
-                    pn->pn_dflags |= PND_BOUND;
-                    break;
-
-                  default:;
-                }
+        if (topLevel && tc->inFunction()) {
+            /*
+             * Define a local in the outer function so that BindNameToSlot
+             * can properly optimize accesses. Note that we need a local
+             * variable, not an argument, for the function statement. Thus
+             * we add a variable even if a parameter with the given name
+             * already exists.
+             */
+            uintN index;
+            switch (tc->fun()->lookupLocal(context, funAtom, &index)) {
+              case JSLOCAL_NONE:
+              case JSLOCAL_ARG:
+                index = tc->fun()->u.i.nvars;
+                if (!tc->fun()->addLocal(context, funAtom, JSLOCAL_VAR))
+                    return NULL;
+                /* FALL THROUGH */
+
+              case JSLOCAL_VAR:
+                pn->pn_cookie.set(tc->staticLevel, index);
+                pn->pn_dflags |= PND_BOUND;
+                break;
+
+              default:;
             }
         }
     }
 
     JSTreeContext *outertc = tc;
 
     /* Initialize early for possible flags mutation via destructuringExpr. */
     JSTreeContext funtc(tc->parser);
--- a/js/src/jsparse.h
+++ b/js/src/jsparse.h
@@ -474,17 +474,17 @@ public:
     JSParseNode  *maybeExpr()   { return pn_used ? NULL : expr(); }
     JSDefinition *maybeLexDef() { return pn_used ? lexdef() : NULL; }
 
 /* PN_FUNC and PN_NAME pn_dflags bits. */
 #define PND_LET         0x01            /* let (block-scoped) binding */
 #define PND_CONST       0x02            /* const binding (orthogonal to let) */
 #define PND_INITIALIZED 0x04            /* initialized declaration */
 #define PND_ASSIGNED    0x08            /* set if ever LHS of assignment */
-#define PND_TOPLEVEL    0x10            /* function at top of body or prog */
+#define PND_TOPLEVEL    0x10            /* see isTopLevel() below */
 #define PND_BLOCKCHILD  0x20            /* use or def is direct block child */
 #define PND_GVAR        0x40            /* gvar binding, can't close over
                                            because it could be deleted */
 #define PND_PLACEHOLDER 0x80            /* placeholder definition for lexdep */
 #define PND_FUNARG     0x100            /* downward or upward funarg usage */
 #define PND_BOUND      0x200            /* bound to a stack or global slot */
 #define PND_DEOPTIMIZED 0x400           /* former pn_used name node, pn_lexdef
                                            still valid, but this use no longer
@@ -524,24 +524,37 @@ public:
         return pn_cookie.slot();
     }
 
     inline bool test(uintN flag) const;
 
     bool isLet() const          { return test(PND_LET); }
     bool isConst() const        { return test(PND_CONST); }
     bool isInitialized() const  { return test(PND_INITIALIZED); }
-    bool isTopLevel() const     { return test(PND_TOPLEVEL); }
     bool isBlockChild() const   { return test(PND_BLOCKCHILD); }
     bool isPlaceholder() const  { return test(PND_PLACEHOLDER); }
     bool isDeoptimized() const  { return test(PND_DEOPTIMIZED); }
     bool isAssigned() const     { return test(PND_ASSIGNED); }
     bool isFunArg() const       { return test(PND_FUNARG); }
     bool isClosed() const       { return test(PND_CLOSED); }
 
+    /*
+     * True iff this definition creates a top-level binding in the overall
+     * script being compiled -- that is, it affects the whole program's
+     * bindings, not bindings for a specific function (unless this definition
+     * is in the outermost scope in eval code, executed within a function) or
+     * the properties of a specific object (through the with statement).
+     *
+     * NB: Function sub-statements found in overall program code and not nested
+     *     within other functions are not currently top level, even though (if
+     *     executed) they do create top-level bindings; there is no particular
+     *     rationale for this behavior.
+     */
+    bool isTopLevel() const     { return test(PND_TOPLEVEL); }
+
     /* Defined below, see after struct JSDefinition. */
     void setFunArg();
 
     void become(JSParseNode *pn2);
     void clear();
 
     /* True if pn is a parsenode representing a literal constant. */
     bool isLiteral() const {
--- a/js/src/tests/ecma_5/Expressions/jstests.list
+++ b/js/src/tests/ecma_5/Expressions/jstests.list
@@ -1,4 +1,5 @@
 url-prefix ../../jsreftest.html?test=ecma_5/Expressions/
 script 11.1.5-01.js
 script named-accessor-function.js
+script nested-delete-name-in-evalcode.js
 script object-literal-accessor-arguments.js
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/Expressions/nested-delete-name-in-evalcode.js
@@ -0,0 +1,163 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 616294;
+var summary =
+  "|delete x| inside a function in eval code, where that eval code includes " +
+  "|var x| at top level, actually does delete the binding for x";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var f;
+
+function testOuterVar()
+{
+  return eval("var x; (function() { return delete x; })");
+}
+
+f = testOuterVar();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testOuterFunction()
+{
+  return eval("function x() { } (function() { return delete x; })");
+}
+
+f = testOuterFunction();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testOuterForVar()
+{
+  return eval("for (var x; false; ); (function() { return delete x; })");
+}
+
+f = testOuterForVar();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testOuterForInVar()
+{
+  return eval("for (var x in {}); (function() { return delete x; })");
+}
+
+f = testOuterForInVar();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testOuterNestedVar()
+{
+  return eval("for (var q = 0; q < 5; q++) { var x; } (function() { return delete x; })");
+}
+
+f = testOuterNestedVar();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testOuterNestedConditionalVar()
+{
+  return eval("for (var q = 0; q < 5; q++) { if (false) { var x; } } (function() { return delete x; })");
+}
+
+f = testOuterNestedConditionalVar();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testVarInWith()
+{
+  return eval("with ({}) { var x; } (function() { return delete x; })");
+}
+
+f = testVarInWith();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testForVarInWith()
+{
+  return eval("with ({}) { for (var x = 0; x < 5; x++); } (function() { return delete x; })");
+}
+
+f = testForVarInWith();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testForInVarInWith()
+{
+  return eval("with ({}) { for (var x in {}); } (function() { return delete x; })");
+}
+
+f = testForInVarInWith();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testUnknown()
+{
+  return eval("nameToDelete = 17; (function() { return delete nameToDelete; })");
+}
+
+f = testUnknown();
+
+assertEq(f(), true); // configurable global property, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testArgumentShadow()
+{
+  return eval("var x; (function(x) { return delete x; })");
+}
+
+f = testArgumentShadow();
+
+assertEq(f(), false); // non-configurable argument => false
+
+
+function testArgument()
+{
+  return eval("(function(x) { return delete x; })");
+}
+
+f = testArgument();
+
+assertEq(f(), false); // non-configurable argument => false
+
+
+function testFunctionLocal()
+{
+  return eval("(function() { var x; return delete x; })");
+}
+
+f = testFunctionLocal();
+
+assertEq(f(), false); // defined by function code => not configurable => false
+
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("All tests passed!");
--- a/js/src/tests/ecma_5/extensions/jstests.list
+++ b/js/src/tests/ecma_5/extensions/jstests.list
@@ -10,8 +10,9 @@ script string-literal-getter-setter-deco
 script watch-inherited-property.js
 script watch-replaced-setter.js
 script watch-setter-become-setter.js
 script watch-value-prop-becoming-setter.js
 script watchpoint-deletes-JSPropertyOp-setter.js
 script eval-native-callback-is-indirect.js
 script regress-bug607284.js
 script Object-keys-and-object-ids.js
+fails script nested-delete-name-in-evalcode.js # bug 604301, at a minimum
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/extensions/nested-delete-name-in-evalcode.js
@@ -0,0 +1,98 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 616294;
+var summary =
+  "|delete x| inside a function in eval code, where that eval code includes " +
+  "|var x| at top level, actually does delete the binding for x";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var f;
+
+function testOuterLet()
+{
+  return eval("let x; (function() { return delete x; })");
+}
+
+f = testOuterLet();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testOuterForLet()
+{
+  return eval("for (let x; false; ); (function() { return delete x; })");
+}
+
+f = testOuterForLet();
+
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testOuterForInLet()
+{
+  return eval("for (let x in {}); (function() { return delete x; })");
+}
+
+f = testOuterForInLet();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // not there => true (only non-configurable => false)
+
+
+function testOuterNestedVarInLetBlock()
+{
+  return eval("let (x = 7) { var x = 9; } (function() { return delete x; })");
+}
+
+f = testOuterNestedVarInLetBlock();
+
+assertEq(f(), true); // configurable var, so remove => true
+assertEq(f(), true); // let still there, configurable => true
+assertEq(f(), true); // nothing at all => true
+
+
+function testOuterNestedVarInForLet()
+{
+  return eval("for (let q = 0; q < 5; q++) { var x; } (function() { return delete x; })");
+}
+
+f = testOuterNestedVarInForLet();
+
+assertEq(f(), true); // configurable, so remove => true
+assertEq(f(), true); // there => true
+
+
+function testArgumentShadowLet()
+{
+  return eval("let x; (function(x) { return delete x; })");
+}
+
+f = testArgumentShadowLet();
+
+assertEq(f(), false); // non-configurable argument => false
+
+
+function testFunctionLocal()
+{
+  return eval("(function() { let x; return delete x; })");
+}
+
+f = testFunctionLocal();
+
+assertEq(f(), false); // defined by function code => not configurable => false
+
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("All tests passed!");