Bug 1171177 - Remove VAROBJFIX. (r=luke)
☠☠ backed out by bb430c06683f ☠ ☠
authorShu-yu Guo <shu@rfrn.org>
Mon, 15 Jun 2015 17:38:01 -0700
changeset 267100 6f535aa717257acbd903fb950c938190dcb0c72e
parent 267099 ce3c302864bff1faa4efaa2587c5c9ca7295fa97
child 267101 18c286d070ada8caab8ac522d2bcd6aec5fa0482
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-esr52@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs1171177
milestone41.0a1
Bug 1171177 - Remove VAROBJFIX. (r=luke)
js/src/jsapi-tests/testJSEvaluateScript.cpp
js/src/jsapi-tests/tests.h
js/src/jsapi.cpp
js/src/jsapi.h
js/src/vm/Interpreter.cpp
--- a/js/src/jsapi-tests/testJSEvaluateScript.cpp
+++ b/js/src/jsapi-tests/testJSEvaluateScript.cpp
@@ -6,48 +6,30 @@
 
 using mozilla::ArrayLength;
 
 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);
     JS::CompileOptions opts(cx);
     JS::AutoObjectVector scopeChain(cx);
     CHECK(scopeChain.append(obj));
     CHECK(JS::Evaluate(cx, scopeChain, opts.setFileAndLine(__FILE__, __LINE__),
                        src, ArrayLength(src) - 1, &retval));
 
     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;
 }
 END_TEST(testJSEvaluateScript)
 
 
--- a/js/src/jsapi-tests/tests.h
+++ b/js/src/jsapi-tests/tests.h
@@ -280,17 +280,16 @@ class JSAPITest
     }
 
     virtual JSRuntime * createRuntime() {
         JSRuntime* rt = JS_NewRuntime(8L * 1024 * 1024);
         if (!rt)
             return nullptr;
         JS_SetErrorReporter(rt, &reportError);
         setNativeStackQuota(rt);
-        JS::RuntimeOptionsRef(rt).setVarObjFix(true);
         return rt;
     }
 
     virtual void destroyRuntime() {
         MOZ_RELEASE_ASSERT(!cx);
         MOZ_RELEASE_ASSERT(rt);
         JS_DestroyRuntime(rt);
         rt = nullptr;
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -3291,16 +3291,26 @@ CreateNonSyntacticScopeChain(JSContext* 
         return false;
 
     if (scopeChain.empty()) {
         staticScopeObj.set(nullptr);
     } else {
         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;
 }
 
 static bool
 IsFunctionCloneable(HandleFunction fun, HandleObject dynamicScope)
 {
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -1162,18 +1162,17 @@ class JS_PUBLIC_API(RuntimeOptions) {
       : baseline_(true),
         ion_(true),
         asmJS_(true),
         nativeRegExp_(true),
         unboxedArrays_(false),
         asyncStack_(true),
         werror_(false),
         strictMode_(false),
-        extraWarnings_(false),
-        varObjFix_(false)
+        extraWarnings_(false)
     {
     }
 
     bool baseline() const { return baseline_; }
     RuntimeOptions& setBaseline(bool flag) {
         baseline_ = flag;
         return *this;
     }
@@ -1245,37 +1244,26 @@ class JS_PUBLIC_API(RuntimeOptions) {
         extraWarnings_ = flag;
         return *this;
     }
     RuntimeOptions& toggleExtraWarnings() {
         extraWarnings_ = !extraWarnings_;
         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;
     bool asmJS_ : 1;
     bool nativeRegExp_ : 1;
     bool unboxedArrays_ : 1;
     bool asyncStack_ : 1;
     bool werror_ : 1;
     bool strictMode_ : 1;
     bool extraWarnings_ : 1;
-    bool varObjFix_ : 1;
 };
 
 JS_PUBLIC_API(RuntimeOptions&)
 RuntimeOptionsRef(JSRuntime* rt);
 
 JS_PUBLIC_API(RuntimeOptions&)
 RuntimeOptionsRef(JSContext* cx);
 
@@ -3809,34 +3797,18 @@ JS_DecompileFunctionBody(JSContext* cx, 
  * NB: JS_ExecuteScript and the JS::Evaluate APIs come in two flavors: either
  * they use the global as the scope, or they take an AutoObjectVector of objects
  * to use as the scope chain.  In the former case, the global is also used as
  * the "this" keyword value and the variables object (ECMA parlance for where
  * 'var' and 'function' bind names) of the execution context for script.  In the
  * latter case, the first object in the provided list is used, unless the list
  * is empty, in which case the global is used.
  *
- * 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
+ * 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
  * 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
  * code can continue to use the familiar JS::Evaluate, etc., entry points.
  */
 
 /*
  * Evaluate a script in the scope of the current global of cx.
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -907,22 +907,16 @@ js::Execute(JSContext* cx, HandleScript 
 #ifdef DEBUG
     JSObject* s = scopeChain;
     do {
         assertSameCompartment(cx, s);
         MOZ_ASSERT_IF(!s->enclosingScope(), s->is<GlobalObject>());
     } 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)
         return false;
     Value thisv = ObjectValue(*thisObj);
 
     return ExecuteKernel(cx, script, *scopeChain, thisv, NullValue(), EXECUTE_GLOBAL,
                          NullFramePtr() /* evalInFrame */, rval);