Backed out changesets b41a69e1b81c and 04f4fec38c9d (bug 1171177) for ASAN jsreftest crashes.
authorRyan VanderMeulen <ryanvm@gmail.com>
Fri, 19 Jun 2015 10:56:01 -0400
changeset 280552 937562e73564632d3fa7f512ffe0f37791df5f90
parent 280551 4c42035bc7f8945ca10f799925ec54d25dd7d154
child 280553 bfc988bd3c77b961248c666a4edd83de2a102490
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)
bugs1171177
milestone41.0a1
backs outb41a69e1b81c4ab38dd1b0291c4bc37aaf2e85b1
04f4fec38c9d25b4ca1bd8760d8754ddc4c23ccb
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
Backed out changesets b41a69e1b81c and 04f4fec38c9d (bug 1171177) for ASAN jsreftest crashes.
js/src/jsapi-tests/testJSEvaluateScript.cpp
js/src/jsapi-tests/tests.h
js/src/jsapi.cpp
js/src/jsapi.h
js/src/jsobj.h
js/src/jsobjinlines.h
js/src/vm/GlobalObject.cpp
js/src/vm/Interpreter.cpp
js/src/vm/ScopeObject.cpp
js/src/vm/Shape.h
--- a/js/src/jsapi-tests/testJSEvaluateScript.cpp
+++ b/js/src/jsapi-tests/testJSEvaluateScript.cpp
@@ -6,30 +6,48 @@
 
 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,16 +280,17 @@ 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
@@ -3307,26 +3307,16 @@ 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
@@ -1181,17 +1181,18 @@ class JS_PUBLIC_API(RuntimeOptions) {
       : baseline_(true),
         ion_(true),
         asmJS_(true),
         nativeRegExp_(true),
         unboxedArrays_(false),
         asyncStack_(true),
         werror_(false),
         strictMode_(false),
-        extraWarnings_(false)
+        extraWarnings_(false),
+        varObjFix_(false)
     {
     }
 
     bool baseline() const { return baseline_; }
     RuntimeOptions& setBaseline(bool flag) {
         baseline_ = flag;
         return *this;
     }
@@ -1263,26 +1264,37 @@ 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);
 
@@ -3816,18 +3828,34 @@ 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.
  *
- * 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
  * 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/jsobj.h
+++ b/js/src/jsobj.h
@@ -211,45 +211,26 @@ class JSObject : public js::gc::Cell
     inline bool isBoundFunction() const;
     inline bool hasSpecialEquality() const;
 
     inline bool watched() const;
     bool setWatched(js::ExclusiveContext* cx) {
         return setFlags(cx, js::BaseShape::WATCHED, GENERATE_SHAPE);
     }
 
-    // A "qualified" varobj is the object on which "qualified" variable
-    // declarations (i.e., those defined with "var") are kept.
-    //
-    // 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;
+    /* See InterpreterFrame::varObj. */
+    inline bool isQualifiedVarObj();
     bool setQualifiedVarObj(js::ExclusiveContext* cx) {
         return setFlags(cx, js::BaseShape::QUALIFIED_VAROBJ);
     }
 
-    // An "unqualified" varobj is the object on which "unqualified"
-    // assignments (i.e., bareword assignments for which the LHS does not
-    // exist on the scope chain) are kept.
-    inline bool isUnqualifiedVarObj() const;
+    inline bool isUnqualifiedVarObj();
+    bool setUnqualifiedVarObj(js::ExclusiveContext* cx) {
+        return setFlags(cx, js::BaseShape::UNQUALIFIED_VAROBJ);
+    }
 
     /*
      * Objects with an uncacheable proto can have their prototype mutated
      * without inducing a shape change on the object. Property cache entries
      * and JIT inline caches should not be filled for lookups across prototype
      * lookups on the object.
      */
     inline bool hasUncacheableProto() const;
--- a/js/src/jsobjinlines.h
+++ b/js/src/jsobjinlines.h
@@ -215,35 +215,34 @@ js::DeleteElement(JSContext* cx, HandleO
         return false;
     return DeleteProperty(cx, obj, id, result);
 }
 
 
 /* * */
 
 inline bool
-JSObject::isQualifiedVarObj() const
+JSObject::isQualifiedVarObj()
 {
     if (is<js::DebugScopeObject>())
         return as<js::DebugScopeObject>().scope().isQualifiedVarObj();
-    bool rv = hasAllFlags(js::BaseShape::QUALIFIED_VAROBJ);
-    MOZ_ASSERT_IF(rv,
-                  is<js::GlobalObject>() ||
-                  is<js::CallObject>() ||
-                  is<js::NonSyntacticVariablesObject>() ||
-                  (is<js::DynamicWithObject>() && !as<js::DynamicWithObject>().isSyntactic()));
-    return rv;
+    // TODO: We would like to assert that only GlobalObject or
+    // NonSyntacticVariables object is a qualified varobj, but users of
+    // js::Execute still need to be vetted. See bug 1171177.
+    return hasAllFlags(js::BaseShape::QUALIFIED_VAROBJ);
 }
 
 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 {
 
 inline bool
 ClassCanHaveFixedData(const Class* clasp)
 {
     // Normally, the number of fixed slots given an object is the maximum
--- a/js/src/vm/GlobalObject.cpp
+++ b/js/src/vm/GlobalObject.cpp
@@ -242,27 +242,28 @@ GlobalObject::createInternal(JSContext* 
     MOZ_ASSERT(clasp->flags & JSCLASS_IS_GLOBAL);
     MOZ_ASSERT(clasp->trace == JS_GlobalObjectTraceHook);
 
     JSObject* obj = NewObjectWithGivenProto(cx, clasp, nullptr, SingletonObject);
     if (!obj)
         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.
     if (clasp->flags & JSCLASS_HAS_PRIVATE)
         global->setPrivate(nullptr);
 
     cx->compartment()->initGlobal(*global);
 
     if (!global->setQualifiedVarObj(cx))
         return nullptr;
+    if (!global->setUnqualifiedVarObj(cx))
+        return nullptr;
     if (!global->setDelegate(cx))
         return nullptr;
 
     return global;
 }
 
 GlobalObject*
 GlobalObject::new_(JSContext* cx, const Class* clasp, JSPrincipals* principals,
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -912,16 +912,22 @@ 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);
--- a/js/src/vm/ScopeObject.cpp
+++ b/js/src/vm/ScopeObject.cpp
@@ -574,20 +574,22 @@ const Class StaticNonSyntacticScopeObjec
 NonSyntacticVariablesObject::create(JSContext* cx, Handle<GlobalObject*> global)
 {
     Rooted<NonSyntacticVariablesObject*> obj(cx,
         NewObjectWithNullTaggedProto<NonSyntacticVariablesObject>(cx, TenuredObject,
                                                                   BaseShape::DELEGATE));
     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;
 }
 
 const Class NonSyntacticVariablesObject::class_ = {
     "NonSyntacticVariablesObject",
     JSCLASS_HAS_RESERVED_SLOTS(NonSyntacticVariablesObject::RESERVED_SLOTS) |
     JSCLASS_IS_ANONYMOUS
--- a/js/src/vm/Shape.h
+++ b/js/src/vm/Shape.h
@@ -348,20 +348,26 @@ class BaseShape : public gc::TenuredCell
         BOUND_FUNCTION      =   0x40,
         HAD_ELEMENTS_ACCESS =   0x80,
         WATCHED             =  0x100,
         ITERATED_SINGLETON  =  0x200,
         NEW_GROUP_UNKNOWN   =  0x400,
         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,
-
-        // 0x4000 is unused.
+        UNQUALIFIED_VAROBJ  = 0x4000,
 
         // For a function used as an interpreted constructor, whether a 'new'
         // type had constructor information cleared.
         NEW_SCRIPT_CLEARED  = 0x8000,
 
         OBJECT_FLAG_MASK    = 0xfff8
     };