From a17e9d1d89e6f3dbd4a211afafcdea24741e0fc0 Mon Sep 17 00:00:00 2001 From: Wes Kocher Date: Mon, 15 Jun 2015 18:07:15 -0700 Subject: [PATCH] Backed out 2 changesets (bug 1171177) Backed out changeset 18c286d070ad (bug 1171177) Backed out changeset 6f535aa71725 (bug 1171177) --- js/src/jsapi-tests/testJSEvaluateScript.cpp | 20 +++++++++++- js/src/jsapi-tests/tests.h | 1 + js/src/jsapi.cpp | 10 ------ js/src/jsapi.h | 34 +++++++++++++++++++-- js/src/jsobj.h | 31 ++++--------------- js/src/jsobjinlines.h | 19 ++++++------ js/src/vm/GlobalObject.cpp | 3 +- js/src/vm/Interpreter.cpp | 6 ++++ js/src/vm/ScopeObject.cpp | 4 ++- js/src/vm/Shape.h | 12 ++++++-- 10 files changed, 86 insertions(+), 54 deletions(-) diff --git a/js/src/jsapi-tests/testJSEvaluateScript.cpp b/js/src/jsapi-tests/testJSEvaluateScript.cpp index efda289c38f..615b90c092d 100644 --- a/js/src/jsapi-tests/testJSEvaluateScript.cpp +++ b/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; diff --git a/js/src/jsapi-tests/tests.h b/js/src/jsapi-tests/tests.h index b70d45b23db..d3423fea057 100644 --- a/js/src/jsapi-tests/tests.h +++ b/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; } diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 737e33249f3..c8df8402221 100644 --- a/js/src/jsapi.cpp +++ b/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; diff --git a/js/src/jsapi.h b/js/src/jsapi.h index 33af84cadc7..7f0f738954a 100644 --- a/js/src/jsapi.h +++ b/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 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 diff --git a/js/src/jsobj.h b/js/src/jsobj.h index 0c5062ab408..550e1ae0142 100644 --- a/js/src/jsobj.h +++ b/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 - // 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 diff --git a/js/src/jsobjinlines.h b/js/src/jsobjinlines.h index 7a16c5d43a8..b5934559971 100644 --- a/js/src/jsobjinlines.h +++ b/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()) return as().scope().isQualifiedVarObj(); - bool rv = hasAllFlags(js::BaseShape::QUALIFIED_VAROBJ); - MOZ_ASSERT_IF(rv, - is() || - is() || - is() || - (is() && !as().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()) return as().scope().isUnqualifiedVarObj(); - return is() || is(); + bool rv = hasAllFlags(js::BaseShape::UNQUALIFIED_VAROBJ); + MOZ_ASSERT_IF(rv, is() || is()); + return rv; } namespace js { diff --git a/js/src/vm/GlobalObject.cpp b/js/src/vm/GlobalObject.cpp index 2c5fe4e4e9e..7893af1ae00 100644 --- a/js/src/vm/GlobalObject.cpp +++ b/js/src/vm/GlobalObject.cpp @@ -247,7 +247,6 @@ GlobalObject::createInternal(JSContext* cx, const Class* clasp) return nullptr; Rooted global(cx, &obj->as()); - 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; diff --git a/js/src/vm/Interpreter.cpp b/js/src/vm/Interpreter.cpp index b9845700640..0d80e0fb479 100644 --- a/js/src/vm/Interpreter.cpp +++ b/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) diff --git a/js/src/vm/ScopeObject.cpp b/js/src/vm/ScopeObject.cpp index c987a547bd6..53016b172be 100644 --- a/js/src/vm/ScopeObject.cpp +++ b/js/src/vm/ScopeObject.cpp @@ -579,10 +579,12 @@ NonSyntacticVariablesObject::create(JSContext* cx, Handle 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; } diff --git a/js/src/vm/Shape.h b/js/src/vm/Shape.h index 92ec0bba660..673370c0666 100644 --- a/js/src/vm/Shape.h +++ b/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, - - // 0x4000 is unused. + UNQUALIFIED_VAROBJ = 0x4000, // For a function used as an interpreted constructor, whether a 'new' // type had constructor information cleared.