Bug 531675 - Ignore the second argument of eval, except to warn once per JSScript (per function, or per global script for uses not in functions) that it's not supported. (Again.) r=mrbkap
authorJeff Walden <jwalden@mit.edu>
Fri, 23 Jul 2010 11:27:07 -0500
changeset 48589 ff6cf05b19f1d41b85542c3def35d90d72e76c6c
parent 48588 e0bc614d6ac250740766c1f458444a16c09ecb17
child 48590 898b2067238343af56dcdd887dcc8a18d85183ec
push id14748
push userrsayre@mozilla.com
push dateSun, 01 Aug 2010 00:33:23 +0000
treeherdermozilla-central@f0df797bb2a9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs531675
milestone2.0b2pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 531675 - Ignore the second argument of eval, except to warn once per JSScript (per function, or per global script for uses not in functions) that it's not supported. (Again.) r=mrbkap
js/src/jsobj.cpp
js/src/jsscript.cpp
js/src/jsscript.h
js/src/tests/js1_4/Eval/jstests.list
js/src/tests/js1_4/Eval/regress-531037.js
js/src/trace-test/tests/basic/bug531037.js
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -1067,119 +1067,77 @@ obj_eval(JSContext *cx, uintN argc, Valu
         }
     }
 
     if (!argv[0].isString()) {
         *vp = argv[0];
         return JS_TRUE;
     }
 
-    /* Accept an optional trailing argument that overrides the scope object. */
-    JSObject *scopeobj = NULL;
-    if (argc >= 2) {
-        if (!js_ValueToObjectOrNull(cx, argv[1], &scopeobj))
+    /*
+     * We once supported a second argument to eval to use as the scope chain
+     * when evaluating the code string.  Warn when such uses are seen so that
+     * authors will know that support for eval(s, o) has been removed.
+     */
+    if (argc > 1 && !caller->script->warnedAboutTwoArgumentEval) {
+        static const char TWO_ARGUMENT_WARNING[] =
+            "Support for eval(code, scopeObject) has been removed. "
+            "Use |with (scopeObject) eval(code);| instead.";
+        if (!JS_ReportWarning(cx, TWO_ARGUMENT_WARNING))
             return JS_FALSE;
-        argv[1].setObjectOrNull(scopeobj);
-        JSObject *obj = scopeobj;
-        while (obj) {
-            if (obj->isDenseArray() && !obj->makeDenseArraySlow(cx))
-                return false;
-            JSObject *parent = obj->getParent();
-            if (!obj->isNative() ||
-                (!parent && !(obj->getClass()->flags & JSCLASS_IS_GLOBAL))) {
-                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
-                                     JSMSG_INVALID_EVAL_SCOPE_ARG);
-                return false;
-            }
-            obj = parent;
-        }
-    }
-
-    /* Guard for conditionally-created with object below. */
-    struct WithGuard {
-        JSObject *obj;
-        WithGuard() : obj(NULL) {}
-        ~WithGuard() { if (obj) obj->setPrivate(NULL); }
-    } withGuard;
+        caller->script->warnedAboutTwoArgumentEval = true;
+    }
 
     /* From here on, control must exit through label out with ok set. */
     MUST_FLOW_THROUGH("out");
     uintN staticLevel = caller->script->staticLevel + 1;
-    if (!scopeobj) {
-        /*
-         * Bring fp->scopeChain up to date. We're either going to use
-         * it (direct call) or save it and restore it (indirect call).
-         */
-        JSObject *callerScopeChain = js_GetScopeChain(cx, caller);
-        if (!callerScopeChain)
-            return JS_FALSE;
+
+    /*
+     * Bring fp->scopeChain up to date. We're either going to use
+     * it (direct call) or save it and restore it (indirect call).
+     */
+    JSObject *callerScopeChain = js_GetScopeChain(cx, caller);
+    if (!callerScopeChain)
+        return JS_FALSE;
+
+    JSObject *scopeobj = NULL;
 
 #if JS_HAS_EVAL_THIS_SCOPE
+    /*
+     * If we see an indirect call, then run eval in the global scope. We do
+     * this so the compiler can make assumptions about what bindings may or
+     * may not exist in the current frame if it doesn't see 'eval'.
+     */
+    if (indirectCall) {
+        /* Pretend that we're top level. */
+        staticLevel = 0;
+
+        OBJ_TO_INNER_OBJECT(cx, obj);
+        if (!obj)
+            return JS_FALSE;
+
+        if (!js_CheckPrincipalsAccess(cx, obj,
+                                      JS_StackFramePrincipals(cx, caller),
+                                      cx->runtime->atomState.evalAtom)) {
+            return JS_FALSE;
+        }
+
+        /* NB: We know inner is a global object here. */
+        JS_ASSERT(!obj->getParent());
+        scopeobj = obj;
+    } else {
         /*
-         * If we see an indirect call, then run eval in the global scope. We do
-         * this so the compiler can make assumptions about what bindings may or
-         * may not exist in the current frame if it doesn't see 'eval'.
+         * Compile using the caller's current scope object.
+         *
+         * NB: This means that the C API must not be used to call eval.
          */
-        if (indirectCall) {
-            /* Pretend that we're top level. */
-            staticLevel = 0;
-
-            OBJ_TO_INNER_OBJECT(cx, obj);
-            if (!obj)
-                return JS_FALSE;
-
-            if (!js_CheckPrincipalsAccess(cx, obj,
-                                          JS_StackFramePrincipals(cx, caller),
-                                          cx->runtime->atomState.evalAtom)) {
-                return JS_FALSE;
-            }
-
-            /* NB: We know inner is a global object here. */
-            JS_ASSERT(!obj->getParent());
-            scopeobj = obj;
-        } else {
-            /*
-             * Compile using the caller's current scope object.
-             *
-             * NB: This means that native callers (who reach this point through
-             * the C API) must use the two parameter form.
-             */
-            JS_ASSERT_IF(caller->argv, caller->callobj);
-            scopeobj = callerScopeChain;
-        }
+        JS_ASSERT_IF(caller->argv, caller->callobj);
+        scopeobj = callerScopeChain;
+    }
 #endif
-    } else {
-        scopeobj = scopeobj->wrappedObject(cx);
-        OBJ_TO_INNER_OBJECT(cx, scopeobj);
-        if (!scopeobj)
-            return JS_FALSE;
-
-        if (!js_CheckPrincipalsAccess(cx, scopeobj,
-                                      JS_StackFramePrincipals(cx, caller),
-                                      cx->runtime->atomState.evalAtom))
-            return JS_FALSE;
-
-        /*
-         * If scopeobj is not a global object, then we need to wrap it in a
-         * with object to maintain invariants in the engine (see bug 520164).
-         */
-        if (scopeobj->getParent()) {
-            JSObject *global = scopeobj->getGlobal();
-            withGuard.obj = js_NewWithObject(cx, scopeobj, global, 0);
-            if (!withGuard.obj)
-                return JS_FALSE;
-
-            scopeobj = withGuard.obj;
-            JS_ASSERT(argc >= 2);
-            argv[1].setObject(*withGuard.obj);
-        }
-
-        /* We're pretending that we're in global code. */
-        staticLevel = 0;
-    }
 
     /* Ensure we compile this eval with the right object in the scope chain. */
     JSObject *result = js_CheckScopeChainValidity(cx, scopeobj, js_eval_str);
     JS_ASSERT_IF(result, result == scopeobj);
     if (!result)
         return JS_FALSE;
 
     // CSP check: is eval() allowed at all?
--- a/js/src/jsscript.cpp
+++ b/js/src/jsscript.cpp
@@ -69,17 +69,17 @@
 #include "jsscriptinlines.h"
 
 using namespace js;
 
 static const jsbytecode emptyScriptCode[] = {JSOP_STOP, SRC_NULL};
 
 /* static */ const JSScript JSScript::emptyScriptConst = {
     const_cast<jsbytecode*>(emptyScriptCode),
-    1, JSVERSION_DEFAULT, 0, 0, 0, 0, 0, 0, true, false, false, false,
+    1, JSVERSION_DEFAULT, 0, 0, 0, 0, 0, 0, true, false, false, false, true,
     const_cast<jsbytecode*>(emptyScriptCode),
     {0, NULL}, NULL, 0, 0, 0, NULL
 };
 
 #if JS_HAS_XDR
 
 JSBool
 js_XDRScript(JSXDRState *xdr, JSScript **scriptp, bool needMutableScript,
--- a/js/src/jsscript.h
+++ b/js/src/jsscript.h
@@ -167,16 +167,19 @@ struct JSScript {
                                        0 if none */
     uint8           constOffset;    /* offset to the array of constants or
                                        0 if none */
     bool            noScriptRval:1; /* no need for result value of last
                                        expression statement */
     bool            savedCallerFun:1; /* object 0 is caller function */
     bool            hasSharps:1;      /* script uses sharp variables */
     bool            strictModeCode:1; /* code is in strict mode */
+    bool            warnedAboutTwoArgumentEval:1; /* have warned about use of
+                                                     obsolete eval(s, o) in
+                                                     this script */
 
     jsbytecode      *main;      /* main entry point, after predef'ing prolog */
     JSAtomMap       atomMap;    /* maps immediate index to literal struct */
     const char      *filename;  /* source filename or null */
     uint32          lineno;     /* base line number of script */
     uint16          nslots;     /* vars plus maximum stack depth */
     uint16          staticLevel;/* static level for display maintenance */
     JSPrincipals    *principals;/* principals for this script */
--- a/js/src/tests/js1_4/Eval/jstests.list
+++ b/js/src/tests/js1_4/Eval/jstests.list
@@ -1,6 +1,5 @@
 url-prefix ../../jsreftest.html?test=js1_4/Eval/
 script eval-001.js
 script eval-002.js
 script eval-003.js
-script regress-531037.js
 script regress-531682.js
deleted file mode 100644
--- a/js/src/tests/js1_4/Eval/regress-531037.js
+++ /dev/null
@@ -1,39 +0,0 @@
-/* -*- Mode: java; tab-width:8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
-/*
- * Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/licenses/publicdomain/
- */
-
-var gTestfile = 'regress-531037.js';
-//-----------------------------------------------------------------------------
-var BUGNUMBER = 531037;
-var summary = 'Checking corner cases of eval(source, scope) form';
-var actual;
-var expect = "No crash";
-
-//-----------------------------------------------------------------------------
-test();
-//-----------------------------------------------------------------------------
-
-function test() {
-    enterFunc ('test');
-    printBugNumber(BUGNUMBER);
-    printStatus (summary);
-
-    var b = 10;
-    var fff = function() { return --b >= 0; };
-    var src = "while (fff());";
-    eval(src, null);
-    b = 10;
-    try {
-        eval(src, {fff: function() {throw 0;}});
-        throw new Error("Unexpected success of eval");
-    } catch (e) {
-        if (e !== 0)
-            throw e;
-    }
-    actual = "No crash";
-    reportCompare(expect, actual, summary);
-    exitFunc ('test');
-}
-
deleted file mode 100644
--- a/js/src/trace-test/tests/basic/bug531037.js
+++ /dev/null
@@ -1,22 +0,0 @@
-/* -*- Mode: java; tab-width:8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
-/*
- * Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/licenses/publicdomain/
- */
-
-(function() {
-    var b = 10;
-    var fff = function() { return --b >= 0; };
-    var src = "while (fff());";
-    eval(src, null);
-    b = 10;
-    try {
-        eval(src, {fff: function() {throw 0;}});
-        throw new Error("Unexpected success of eval");
-    } catch (e) {
-        if (e !== 0)
-            throw e;
-    }
-})();
-
-// We expect this to finish without crashes or exceptions