Bug 1171177 - Remove VAROBJFIX. (r=luke)
☠☠ backed out by 178c1abf3fdb ☠ ☠
authorShu-yu Guo <shu@rfrn.org>
Wed, 17 Jun 2015 21:26:58 -0700
changeset 280274 0a3b1cd87c26062a78899f43f2b4a41b1d49bea2
parent 280273 662ec8b0561ebaefa8c8e563d4868b2b426fdff0
child 280275 83954c7df8ab17e6abbbb730b963fb976a8ce063
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-beta@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs1171177
milestone41.0a1
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 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
@@ -912,22 +912,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);