Backed out changeset c3a547a77df9 (bug 1171177)

js/src/jsapi-tests/testJSEvaluateScript.cpp

@@ -11,6 +11,8 @@ BEGIN_TEST(testJSEvaluateScript)
     JS::RootedObject obj(cx, JS_NewPlainObject(cx));
     CHECK(obj);
 
+    CHECK(JS::RuntimeOptionsRef(cx).varObjFix());
+
     static const char16_t src[] = MOZ_UTF16("var x = 5;");
 
     JS::RootedValue retval(cx);
@@ -22,10 +24,26 @@ BEGIN_TEST(testJSEvaluateScript)
 
     bool hasProp = true;
     CHECK(JS_AlreadyHasOwnProperty(cx, obj, "x", &hasProp));
-    CHECK(hasProp);
+    CHECK(!hasProp);
 
     hasProp = false;
     CHECK(JS_HasProperty(cx, global, "x", &hasProp));
+    CHECK(hasProp);
+
+    // Now do the same thing, but without JSOPTION_VAROBJFIX
+    JS::RuntimeOptionsRef(cx).setVarObjFix(false);
+
+    static const char16_t src2[] = MOZ_UTF16("var y = 5;");
+
+    CHECK(JS::Evaluate(cx, scopeChain, opts.setFileAndLine(__FILE__, __LINE__),
+                       src2, ArrayLength(src2) - 1, &retval));
+
+    hasProp = false;
+    CHECK(JS_AlreadyHasOwnProperty(cx, obj, "y", &hasProp));
+    CHECK(hasProp);
+
+    hasProp = true;
+    CHECK(JS_AlreadyHasOwnProperty(cx, global, "y", &hasProp));
     CHECK(!hasProp);
 
     return true;

js/src/jsapi-tests/tests.h

@@ -285,6 +285,7 @@ class JSAPITest
             return nullptr;
         JS_SetErrorReporter(rt, &reportError);
         setNativeStackQuota(rt);
+        JS::RuntimeOptionsRef(rt).setVarObjFix(true);
         return rt;
     }
 

js/src/jsapi.cpp

@@ -3296,16 +3296,6 @@ CreateNonSyntacticScopeChain(JSContext* cx, AutoObjectVector& scopeChain,
         staticScopeObj.set(StaticNonSyntacticScopeObjects::create(cx, nullptr));
         if (!staticScopeObj)
             return false;
-
-        // The XPConnect subscript loader, which may pass in its own dynamic
-        // scopes to load scripts in, expects the dynamic scope chain to be
-        // the holder of "var" declarations. In SpiderMonkey, such objects are
-        // called "qualified varobjs", the "qualified" part meaning the
-        // declaration was qualified by "var". There is only sadness.
-        //
-        // See JSObject::isQualifiedVarObj.
-        if (!dynamicScopeObj->setQualifiedVarObj(cx))
-            return false;
     }
 
     return true;

js/src/jsapi.h

@@ -1167,7 +1167,8 @@ class JS_PUBLIC_API(RuntimeOptions) {
         asyncStack_(true),
         werror_(false),
         strictMode_(false),
-        extraWarnings_(false)
+        extraWarnings_(false),
+        varObjFix_(false)
     {
     }
 
@@ -1249,6 +1250,16 @@ class JS_PUBLIC_API(RuntimeOptions) {
         return *this;
     }
 
+    bool varObjFix() const { return varObjFix_; }
+    RuntimeOptions& setVarObjFix(bool flag) {
+        varObjFix_ = flag;
+        return *this;
+    }
+    RuntimeOptions& toggleVarObjFix() {
+        varObjFix_ = !varObjFix_;
+        return *this;
+    }
+
   private:
     bool baseline_ : 1;
     bool ion_ : 1;
@@ -1259,6 +1270,7 @@ class JS_PUBLIC_API(RuntimeOptions) {
     bool werror_ : 1;
     bool strictMode_ : 1;
     bool extraWarnings_ : 1;
+    bool varObjFix_ : 1;
 };
 
 JS_PUBLIC_API(RuntimeOptions&)
@@ -3802,8 +3814,24 @@ JS_DecompileFunctionBody(JSContext* cx, JS::Handle<JSFunction*> fun, unsigned in
  * latter case, the first object in the provided list is used, unless the list
  * is empty, in which case the global is used.
  *
- * Why a runtime option?  The alternative is to add APIs duplicating those
- * for the other value of flags, and that doesn't seem worth the code bloat
+ * Using a non-global object as the variables object is problematic: in this
+ * case, variables created by 'var x = 0', e.g., go in that object, but
+ * variables created by assignment to an unbound id, 'x = 0', go in the global.
+ *
+ * ECMA requires that "global code" be executed with the variables object equal
+ * to the global object.  But these JS API entry points provide freedom to
+ * execute code against a "sub-global", i.e., a parented or scoped object, in
+ * which case the variables object will differ from the global, resulting in
+ * confusing and non-ECMA explicit vs. implicit variable creation.
+ *
+ * Caveat embedders: unless you already depend on this buggy variables object
+ * binding behavior, you should call RuntimeOptionsRef(rt).setVarObjFix(true)
+ * for each context in the application, if you use the versions of
+ * JS_ExecuteScript and JS::Evaluate that take a scope chain argument, or may
+ * ever use them in the future.
+ *
+ * Why a runtime option?  The alternative is to add APIs duplicating those below
+ * for the other value of varobjfix, and that doesn't seem worth the code bloat
  * cost.  Such new entry points would probably have less obvious names, too, so
  * would not tend to be used.  The RuntimeOptionsRef adjustment, OTOH, can be
  * more easily hacked into existing code that does not depend on the bug; such

js/src/vm/Interpreter.cpp

@@ -912,6 +912,12 @@ js::Execute(JSContext* cx, HandleScript script, JSObject& scopeChainArg, Value*
     } while ((s = s->enclosingScope()));
 #endif
 
+    /* The VAROBJFIX option makes varObj == globalObj in global code. */
+    if (!cx->runtime()->options().varObjFix()) {
+        if (!scopeChain->setQualifiedVarObj(cx))
+            return false;
+    }
+
     /* Use the scope chain as 'this', modulo outerization. */
     JSObject* thisObj = GetThisObject(cx, scopeChain);
     if (!thisObj)