Backed out 2 changesets (bug 1171177)

Backed out changeset 18c286d070ad (bug 1171177)
Backed out changeset 6f535aa71725 (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
+ * Using a non-global object as the variables object is problematic: in this
- * for the other value of flags, and that doesn't seem worth the code bloat
+ * 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/jsobj.h

@@ -216,35 +216,16 @@ class JSObject : public js::gc::Cell
         return setFlags(cx, js::BaseShape::WATCHED, GENERATE_SHAPE);
     }
 
-    // A "qualified" varobj is the object on which "qualified" variable
+    /* See InterpreterFrame::varObj. */
-    // declarations (i.e., those defined with "var") are kept.
+    inline bool isQualifiedVarObj();
-    //
-    // Conceptually, when a var binding is defined, it is defined on the
-    // innermost qualified varobj on the scope chain.
-    //
-    // Function scopes (CallObjects) are qualified varobjs, and there can be
-    // no other qualified varobj that is more inner for var bindings in that
-    // function. As such, all references to local var bindings in a function
-    // may be statically bound to the function scope. This is subject to
-    // further optimization. Unaliased bindings inside functions reside
-    // entirely on the frame, not in CallObjects.
-    //
-    // Global scopes are also qualified varobjs. It is possible to statically
-    // know, for a given script, that are no more inner qualified varobjs, so
-    // free variable references can be statically bound to the global.
-    //
-    // Finally, there are non-syntactic qualified varobjs used by embedders
-    // (e.g., Gecko and XPConnect), as they often wish to run scripts under a
-    // scope that captures var bindings.
-    inline bool isQualifiedVarObj() const;
     bool setQualifiedVarObj(js::ExclusiveContext* cx) {
         return setFlags(cx, js::BaseShape::QUALIFIED_VAROBJ);
     }
 
-    // An "unqualified" varobj is the object on which "unqualified"
+    inline bool isUnqualifiedVarObj();
-    // assignments (i.e., bareword assignments for which the LHS does not
+    bool setUnqualifiedVarObj(js::ExclusiveContext* cx) {
-    // exist on the scope chain) are kept.
+        return setFlags(cx, js::BaseShape::UNQUALIFIED_VAROBJ);
-    inline bool isUnqualifiedVarObj() const;
+    }
 
     /*
      * Objects with an uncacheable proto can have their prototype mutated

js/src/jsobjinlines.h

@@ -220,25 +220,24 @@ js::DeleteElement(JSContext* cx, HandleObject obj, uint32_t index, ObjectOpResul
 /* * */
 
 inline bool
-JSObject::isQualifiedVarObj() const
+JSObject::isQualifiedVarObj()
 {
     if (is<js::DebugScopeObject>())
         return as<js::DebugScopeObject>().scope().isQualifiedVarObj();
-    bool rv = hasAllFlags(js::BaseShape::QUALIFIED_VAROBJ);
+    // TODO: We would like to assert that only GlobalObject or
-    MOZ_ASSERT_IF(rv,
+    // NonSyntacticVariables object is a qualified varobj, but users of
-                  is<js::GlobalObject>() ||
+    // js::Execute still need to be vetted. See bug 1171177.
-                  is<js::CallObject>() ||
+    return hasAllFlags(js::BaseShape::QUALIFIED_VAROBJ);
-                  is<js::NonSyntacticVariablesObject>() ||
-                  (is<js::DynamicWithObject>() && !as<js::DynamicWithObject>().isSyntactic()));
-    return rv;
 }
 
 inline bool
-JSObject::isUnqualifiedVarObj() const
+JSObject::isUnqualifiedVarObj()
 {
     if (is<js::DebugScopeObject>())
         return as<js::DebugScopeObject>().scope().isUnqualifiedVarObj();
-    return is<js::GlobalObject>() || is<js::NonSyntacticVariablesObject>();
+    bool rv = hasAllFlags(js::BaseShape::UNQUALIFIED_VAROBJ);
+    MOZ_ASSERT_IF(rv, is<js::GlobalObject>() || is<js::NonSyntacticVariablesObject>());
+    return rv;
 }
 
 namespace js {

js/src/vm/GlobalObject.cpp

@@ -247,7 +247,6 @@ GlobalObject::createInternal(JSContext* cx, const Class* clasp)
         return nullptr;
 
     Rooted<GlobalObject*> global(cx, &obj->as<GlobalObject>());
-    MOZ_ASSERT(global->isUnqualifiedVarObj());
 
     // Initialize the private slot to null if present, as GC can call class
     // hooks before the caller gets to set this to a non-garbage value.
@@ -258,6 +257,8 @@ GlobalObject::createInternal(JSContext* cx, const Class* clasp)
 
     if (!global->setQualifiedVarObj(cx))
         return nullptr;
+    if (!global->setUnqualifiedVarObj(cx))
+        return nullptr;
     if (!global->setDelegate(cx))
         return nullptr;
 

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)

js/src/vm/ScopeObject.cpp

@@ -579,10 +579,12 @@ NonSyntacticVariablesObject::create(JSContext* cx, Handle<GlobalObject*> global)
     if (!obj)
         return nullptr;
 
-    MOZ_ASSERT(obj->isUnqualifiedVarObj());
     if (!obj->setQualifiedVarObj(cx))
         return nullptr;
 
+    if (!obj->setUnqualifiedVarObj(cx))
+        return nullptr;
+
     obj->setEnclosingScope(global);
     return obj;
 }

js/src/vm/Shape.h

@@ -353,10 +353,16 @@ class BaseShape : public gc::TenuredCell
         UNCACHEABLE_PROTO   =  0x800,
         IMMUTABLE_PROTOTYPE = 0x1000,
 
-        // See JSObject::isQualifiedVarObj().
+        // These two flags control which scope a new variables ends up on in the
+        // scope chain. If the variable is "qualified" (i.e., if it was defined
+        // using var, let, or const) then it ends up on the lowest scope in the
+        // chain that has the QUALIFIED_VAROBJ flag set. If it's "unqualified"
+        // (i.e., if it was introduced without any var, let, or const, which
+        // incidentally is an error in strict mode) then it goes on the lowest
+        // scope in the chain with the UNQUALIFIED_VAROBJ flag set (which is
+        // typically the global).
         QUALIFIED_VAROBJ    = 0x2000,
-
+        UNQUALIFIED_VAROBJ  = 0x4000,
-        // 0x4000 is unused.
 
         // For a function used as an interpreted constructor, whether a 'new'
         // type had constructor information cleared.