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

--HG--
extra : rebase_source : 7e22a2ec3cfb6fa5510af4ba317e9a6d36b37555

js/src/jsemit.cpp

@@ -2161,13 +2161,7 @@ BindNameToSlot(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
      *
      * 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:
@@ -2175,7 +2169,7 @@ BindNameToSlot(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
         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;
@@ -4579,11 +4573,6 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
             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);
 

js/src/jsparse.cpp

@@ -640,9 +640,9 @@ 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();
 }
 
@@ -1482,6 +1482,8 @@ Define(JSParseNode *pn, JSAtom *atom, JSTreeContext *tc, bool let = 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;
 }
 
@@ -1563,7 +1565,6 @@ MakeDefIntoUse(JSDefinition *dn, JSParseNode *pn, JSAtom *atom, JSTreeContext *t
 
         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;
@@ -2546,8 +2547,6 @@ LeaveFunction(JSParseNode *fn, JSTreeContext *funtc, JSAtom *funAtom = NULL,
     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;
 
@@ -2937,43 +2936,36 @@ Parser::functionDef(JSAtom *funAtom, FunctionType type, uintN lambda)
         }
 
         /*
-         * 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 (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 */
 
-            if (tc->inFunction()) {
-                JSLocalKind localKind;
-                uintN index;
+              case JSLOCAL_VAR:
+                pn->pn_cookie.set(tc->staticLevel, index);
+                pn->pn_dflags |= PND_BOUND;
+                break;
 
-                /*
-                 * 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:;
-                }
+              default:;
             }
         }
     }

js/src/jsparse.h

@@ -479,7 +479,7 @@ public:
 #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 */
@@ -529,7 +529,6 @@ public:
     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); }
@@ -537,6 +536,20 @@ public:
     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();
 

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

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!");

js/src/tests/ecma_5/extensions/jstests.list

@@ -15,3 +15,4 @@ 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

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!");