Bug 1165486 - Replace the PlainObj varobj with NonSyntacticVariablesObject. (r=luke)
☠☠ backed out by b4e617011c42 ☠ ☠
authorShu-yu Guo <shu@rfrn.org>
Wed, 17 Jun 2015 21:26:57 -0700
changeset 280269 7b157964c5722b0d2358773f733dced5a36e2fe3
parent 280268 5c01ab1d9790fec3149b186e6c7659670ae1eaac
child 280270 5abb37cb04950f15ef98d37b41ae7a60ff6d6a10
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
bugs1165486
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 1165486 - Replace the PlainObj varobj with NonSyntacticVariablesObject. (r=luke)
js/src/builtin/Eval.cpp
js/src/jit-test/tests/debug/onNewScript-ExecuteInGlobalAndReturnScope.js
js/src/jsapi.cpp
js/src/jsfun.cpp
js/src/jsobjinlines.h
js/src/vm/ScopeObject.cpp
js/src/vm/ScopeObject.h
js/src/vm/Stack.cpp
--- a/js/src/builtin/Eval.cpp
+++ b/js/src/builtin/Eval.cpp
@@ -497,34 +497,27 @@ js::ExecuteInGlobalAndReturnScope(JSCont
             return false;
         script = CloneGlobalScript(cx, staticScope, script);
         if (!script)
             return false;
 
         Debugger::onNewScript(cx, script);
     }
 
-    RootedObject scope(cx, JS_NewPlainObject(cx));
+    Rooted<GlobalObject*> globalRoot(cx, &global->as<GlobalObject>());
+    Rooted<ScopeObject*> scope(cx, NonSyntacticVariablesObject::create(cx, globalRoot));
     if (!scope)
         return false;
 
-    if (!scope->setQualifiedVarObj(cx))
-        return false;
-
-    if (!scope->setUnqualifiedVarObj(cx))
-        return false;
-
     JSObject* thisobj = GetThisObject(cx, global);
     if (!thisobj)
         return false;
 
     RootedValue thisv(cx, ObjectValue(*thisobj));
     RootedValue rval(cx);
-    // XXXbz when this is fixed to pass in an actual ScopeObject, fix
-    // up the assert in js::CloneFunctionObject accordingly.
     if (!ExecuteKernel(cx, script, *scope, thisv, UndefinedValue(), EXECUTE_GLOBAL,
                        NullFramePtr() /* evalInFrame */, rval.address()))
     {
         return false;
     }
 
     scopeArg.set(scope);
     return true;
--- a/js/src/jit-test/tests/debug/onNewScript-ExecuteInGlobalAndReturnScope.js
+++ b/js/src/jit-test/tests/debug/onNewScript-ExecuteInGlobalAndReturnScope.js
@@ -1,14 +1,15 @@
 // Debugger should be notified of scripts created with ExecuteInGlobalAndReturnScope.
 
 var g = newGlobal();
 var g2 = newGlobal();
 var dbg = new Debugger(g, g2);
 var log = '';
+var canary = 42;
 
 dbg.onNewScript = function (evalScript) {
   log += 'e';
 
   dbg.onNewScript = function (clonedScript) {
     log += 'c';
     clonedScript.setBreakpoint(0, {
       hit(frame) {
@@ -19,10 +20,13 @@ dbg.onNewScript = function (evalScript) 
   };
 };
 
 dbg.onDebuggerStatement = function (frame) {
   log += 'd';
 };
 
 assertEq(log, '');
-var evalScope = g.evalReturningScope("debugger; // nee", g2);
+var evalScope = g.evalReturningScope("canary = 'dead'; debugger; // nee", g2);
 assertEq(log, 'ecbd');
+assertEq(canary, 42);
+assertEq(evalScope.canary, 'dead');
+
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -4133,21 +4133,18 @@ CompileFunction(JSContext* cx, const Rea
         return false;
 
     // Make sure the static scope chain matches up when we have a
     // non-syntactic scope.
     MOZ_ASSERT_IF(!enclosingDynamicScope->is<GlobalObject>(),
                   HasNonSyntacticStaticScopeChain(enclosingStaticScope));
 
     CompileOptions options(cx, optionsArg);
-    if (!frontend::CompileFunctionBody(cx, fun, options, formals, srcBuf,
-                                       enclosingStaticScope))
-    {
+    if (!frontend::CompileFunctionBody(cx, fun, options, formals, srcBuf, enclosingStaticScope))
         return false;
-    }
 
     return true;
 }
 
 JS_PUBLIC_API(bool)
 JS::CompileFunction(JSContext* cx, AutoObjectVector& scopeChain,
                     const ReadOnlyCompileOptions& options,
                     const char* name, unsigned nargs, const char* const* argnames,
--- a/js/src/jsfun.cpp
+++ b/js/src/jsfun.cpp
@@ -1973,43 +1973,47 @@ js::NewScriptedFunction(ExclusiveContext
                         NewObjectKind newKind /* = GenericObject */,
                         HandleObject enclosingDynamicScope /* = nullptr */)
 {
     return NewFunctionWithProto(cx, nullptr, nargs, flags,
                                 enclosingDynamicScope ? enclosingDynamicScope : cx->global(),
                                 atom, nullptr, allocKind, newKind);
 }
 
+#ifdef DEBUG
+static bool
+NewFunctionScopeIsWellFormed(ExclusiveContext* cx, HandleObject parent)
+{
+    // Assert that the parent is null, global, or a debug scope proxy. All
+    // other cases of polluting global scope behavior are handled by
+    // ScopeObjects (viz. non-syntactic DynamicWithObject and
+    // NonSyntacticVariablesObject).
+    RootedObject realParent(cx, SkipScopeParent(parent));
+    return !realParent || realParent == cx->global() ||
+           realParent->is<DebugScopeObject>();
+}
+#endif
+
 JSFunction*
 js::NewFunctionWithProto(ExclusiveContext* cx, Native native,
                          unsigned nargs, JSFunction::Flags flags, HandleObject enclosingDynamicScope,
                          HandleAtom atom, HandleObject proto,
                          gc::AllocKind allocKind /* = AllocKind::FUNCTION */,
                          NewObjectKind newKind /* = GenericObject */)
 {
     MOZ_ASSERT(allocKind == AllocKind::FUNCTION || allocKind == AllocKind::FUNCTION_EXTENDED);
     MOZ_ASSERT_IF(native, !enclosingDynamicScope);
+    MOZ_ASSERT(NewFunctionScopeIsWellFormed(cx, enclosingDynamicScope));
 
     RootedObject funobj(cx);
     // Don't mark asm.js module functions as singleton since they are
     // cloned (via CloneFunctionObjectIfNotSingleton) which assumes that
     // isSingleton implies isInterpreted.
     if (native && !IsAsmJSModuleNative(native))
         newKind = SingletonObject;
-#ifdef DEBUG
-    RootedObject nonScopeParent(cx, SkipScopeParent(enclosingDynamicScope));
-    // We'd like to assert that nonScopeParent is null-or-global, but
-    // js::ExecuteInGlobalAndReturnScope and debugger eval bits mess that up.
-    // Assert that it's one of those or a debug scope proxy or the unqualified
-    // var obj, since it should still be ok to parent to the global in that
-    // case.
-    MOZ_ASSERT(!nonScopeParent || nonScopeParent == cx->global() ||
-               nonScopeParent->is<DebugScopeObject>() ||
-               nonScopeParent->isUnqualifiedVarObj());
-#endif
     funobj = NewObjectWithClassProto(cx, &JSFunction::class_, proto, allocKind,
                                      newKind);
     if (!funobj)
         return nullptr;
 
     RootedFunction fun(cx, &funobj->as<JSFunction>());
 
     if (allocKind == AllocKind::FUNCTION_EXTENDED)
@@ -2062,32 +2066,16 @@ js::CanReuseScriptForClone(JSCompartment
     // We need to clone the script if we're interpreted and not already marked
     // as having a non-syntactic scope. If we're lazy, go ahead and clone the
     // script; see the big comment at the end of CopyScriptInternal for the
     // explanation of what's going on there.
     return !fun->isInterpreted() ||
            (fun->hasScript() && fun->nonLazyScript()->hasNonSyntacticScope());
 }
 
-static bool
-FunctionCloneScopeIsWellFormed(JSContext* cx, HandleObject parent)
-{
-    MOZ_ASSERT(parent);
-    RootedObject realParent(cx, SkipScopeParent(parent));
-    // We'd like to assert that realParent is null-or-global, but
-    // js::ExecuteInGlobalAndReturnScope and debugger eval bits mess that up.
-    // Assert that it's one of those or a debug scope proxy or the unqualified
-    // var obj, since it should still be ok to parent to the global in that
-    // case.
-    return !realParent || realParent == cx->global() ||
-           realParent->is<DebugScopeObject>() ||
-           realParent->isUnqualifiedVarObj();
-}
-
-
 static inline JSFunction*
 NewFunctionClone(JSContext* cx, HandleFunction fun, NewObjectKind newKind,
                  gc::AllocKind allocKind, HandleObject proto)
 {
     RootedObject cloneProto(cx, proto);
     if (!proto && fun->isStarGenerator()) {
         cloneProto = GlobalObject::getOrCreateStarGeneratorFunctionPrototype(cx, cx->global());
         if (!cloneProto)
@@ -2121,17 +2109,17 @@ NewFunctionClone(JSContext* cx, HandleFu
 }
 
 JSFunction*
 js::CloneFunctionReuseScript(JSContext* cx, HandleFunction fun, HandleObject parent,
                              gc::AllocKind allocKind /* = FUNCTION */ ,
                              NewObjectKind newKind /* = GenericObject */,
                              HandleObject proto /* = nullptr */)
 {
-    MOZ_ASSERT(FunctionCloneScopeIsWellFormed(cx, parent));
+    MOZ_ASSERT(NewFunctionScopeIsWellFormed(cx, parent));
     MOZ_ASSERT(!fun->isBoundFunction());
     MOZ_ASSERT(CanReuseScriptForClone(cx->compartment(), fun, parent));
 
     RootedFunction clone(cx, NewFunctionClone(cx, fun, newKind, allocKind, proto));
     if (!clone)
         return nullptr;
 
     if (fun->hasScript()) {
@@ -2156,17 +2144,17 @@ js::CloneFunctionReuseScript(JSContext* 
 }
 
 JSFunction*
 js::CloneFunctionAndScript(JSContext* cx, HandleFunction fun, HandleObject parent,
                            HandleObject newStaticScope,
                            gc::AllocKind allocKind /* = FUNCTION */,
                            HandleObject proto /* = nullptr */)
 {
-    MOZ_ASSERT(FunctionCloneScopeIsWellFormed(cx, parent));
+    MOZ_ASSERT(NewFunctionScopeIsWellFormed(cx, parent));
     MOZ_ASSERT(!fun->isBoundFunction());
 
     RootedFunction clone(cx, NewFunctionClone(cx, fun, SingletonObject, allocKind, proto));
     if (!clone)
         return nullptr;
 
     if (fun->hasScript()) {
         clone->initScript(nullptr);
--- a/js/src/jsobjinlines.h
+++ b/js/src/jsobjinlines.h
@@ -219,25 +219,30 @@ js::DeleteElement(JSContext* cx, HandleO
 
 /* * */
 
 inline bool
 JSObject::isQualifiedVarObj()
 {
     if (is<js::DebugScopeObject>())
         return as<js::DebugScopeObject>().scope().isQualifiedVarObj();
+    // 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()
 {
     if (is<js::DebugScopeObject>())
         return as<js::DebugScopeObject>().scope().isUnqualifiedVarObj();
-    return hasAllFlags(js::BaseShape::UNQUALIFIED_VAROBJ);
+    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/ScopeObject.cpp
+++ b/js/src/vm/ScopeObject.cpp
@@ -565,16 +565,41 @@ StaticNonSyntacticScopeObjects::create(J
 }
 
 const Class StaticNonSyntacticScopeObjects::class_ = {
     "StaticNonSyntacticScopeObjects",
     JSCLASS_HAS_RESERVED_SLOTS(StaticNonSyntacticScopeObjects::RESERVED_SLOTS) |
     JSCLASS_IS_ANONYMOUS
 };
 
+/* static */ NonSyntacticVariablesObject*
+NonSyntacticVariablesObject::create(JSContext* cx, Handle<GlobalObject*> global)
+{
+    Rooted<NonSyntacticVariablesObject*> obj(cx,
+        NewObjectWithNullTaggedProto<NonSyntacticVariablesObject>(cx, TenuredObject,
+                                                                  BaseShape::DELEGATE));
+    if (!obj)
+        return nullptr;
+
+    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
+};
+
 /*****************************************************************************/
 
 /* static */ ClonedBlockObject*
 ClonedBlockObject::create(JSContext* cx, Handle<StaticBlockObject*> block, HandleObject enclosing)
 {
     MOZ_ASSERT(block->getClass() == &BlockObject::class_);
 
     RootedObjectGroup group(cx, ObjectGroup::defaultNewGroup(cx, &BlockObject::class_,
--- a/js/src/vm/ScopeObject.h
+++ b/js/src/vm/ScopeObject.h
@@ -394,40 +394,48 @@ class StaticEvalObject : public ScopeObj
         return getReservedSlot(SCOPE_CHAIN_SLOT).isObject();
     }
 };
 
 // Static scope objects that stand in for one or more "polluting global"
 // scopes on the dynamic scope chain.
 //
 // There are two flavors of polluting global scopes on the dynamic scope
-// chain:
-//
-// 1. 0+ non-syntactic DynamicWithObjects. This static scope helps ScopeIter
-//    iterate these DynamicWithObjects.
-//
-// 2. 1 PlainObject that is a both a QualifiedVarObj and an UnqualifiedVarObj,
-//    created exclusively in js::ExecuteInGlobalAndReturnScope.
-//
-//    Since this PlainObject is not a ScopeObject, ScopeIter cannot iterate
-//    through it. Instead, this PlainObject always comes after the syntactic
-//    portion of the dynamic scope chain in front of a GlobalObject.
+// chain: either 0+ non-syntactic DynamicWithObjects, or 1
+// NonSyntacticVariablesObject, created exclusively in
+// js::ExecuteInGlobalAndReturnScope.
 class StaticNonSyntacticScopeObjects : public ScopeObject
 {
   public:
     static const unsigned RESERVED_SLOTS = 1;
     static const Class class_;
 
     static StaticNonSyntacticScopeObjects* create(JSContext* cx, HandleObject enclosing);
 
     JSObject* enclosingScopeForStaticScopeIter() {
         return getReservedSlot(SCOPE_CHAIN_SLOT).toObjectOrNull();
     }
 };
 
+// A non-syntactic dynamic scope object that captures non-lexical
+// bindings. That is, a scope object that captures both qualified var
+// assignments and unqualified bareword assignments. Its parent is always the
+// real global.
+//
+// This is used in ExecuteInGlobalAndReturnScope and sits in front of the
+// global scope to capture 'var' and bareword asignments.
+class NonSyntacticVariablesObject : public ScopeObject
+{
+  public:
+    static const unsigned RESERVED_SLOTS = 1;
+    static const Class class_;
+
+    static NonSyntacticVariablesObject* create(JSContext* cx, Handle<GlobalObject*> global);
+};
+
 class NestedScopeObject : public ScopeObject
 {
   public:
     /*
      * A refinement of enclosingScope that returns nullptr if the enclosing
      * scope is not a NestedScopeObject.
      */
     inline NestedScopeObject* enclosingNestedScope() const;
@@ -1035,17 +1043,18 @@ JSObject::is<js::NestedScopeObject>() co
 
 template<>
 inline bool
 JSObject::is<js::ScopeObject>() const
 {
     return is<js::CallObject>() ||
            is<js::DeclEnvObject>() ||
            is<js::NestedScopeObject>() ||
-           is<js::UninitializedLexicalObject>();
+           is<js::UninitializedLexicalObject>() ||
+           is<js::NonSyntacticVariablesObject>();
 }
 
 template<>
 inline bool
 JSObject::is<js::DebugScopeObject>() const
 {
     // Note: don't use is<ProxyObject>() here -- it also matches subclasses!
     return hasClass(&js::ProxyObject::class_) &&
@@ -1067,18 +1076,18 @@ JSObject::is<js::StaticBlockObject>() co
 }
 
 namespace js {
 
 inline bool
 IsSyntacticScope(JSObject* scope)
 {
     return scope->is<ScopeObject>() &&
-           (!scope->is<DynamicWithObject>() ||
-            scope->as<DynamicWithObject>().isSyntactic());
+           (!scope->is<DynamicWithObject>() || scope->as<DynamicWithObject>().isSyntactic()) &&
+           !scope->is<NonSyntacticVariablesObject>();
 }
 
 inline const Value&
 ScopeObject::aliasedVar(ScopeCoordinate sc)
 {
     MOZ_ASSERT(is<CallObject>() || is<ClonedBlockObject>());
     return getSlot(sc.slot());
 }
@@ -1106,17 +1115,18 @@ inline bool
 ScopeIter::hasNonSyntacticScopeObject() const
 {
     // The case we're worrying about here is a NonSyntactic static scope which
     // has 0+ corresponding non-syntactic DynamicWithObject scopes or a
     // NonSyntacticVariablesObject.
     if (ssi_.type() == StaticScopeIter<CanGC>::NonSyntactic) {
         MOZ_ASSERT_IF(scope_->is<DynamicWithObject>(),
                       !scope_->as<DynamicWithObject>().isSyntactic());
-        return scope_->is<DynamicWithObject>();
+        return scope_->is<DynamicWithObject>() ||
+               scope_->is<NonSyntacticVariablesObject>();
     }
     return false;
 }
 
 inline bool
 ScopeIter::hasAnyScopeObject() const
 {
     return hasSyntacticScopeObject() || hasNonSyntacticScopeObject();
@@ -1136,16 +1146,19 @@ ScopeIter::canHaveSyntacticScopeObject()
       case With:
         return true;
       case Eval:
         // Only strict eval scopes can have dynamic scope objects.
         return staticEval().isStrict();
       case NonSyntactic:
         return false;
     }
+
+    // Silence warnings.
+    return false;
 }
 
 inline JSObject&
 ScopeIter::enclosingScope() const
 {
     // As an engine invariant (maintained internally and asserted by Execute),
     // ScopeObjects and non-ScopeObjects cannot be interleaved on the scope
     // chain; every scope chain must start with zero or more ScopeObjects and
--- a/js/src/vm/Stack.cpp
+++ b/js/src/vm/Stack.cpp
@@ -149,19 +149,19 @@ InterpreterFrame::createRestParameter(JS
 
 static inline void
 AssertDynamicScopeMatchesStaticScope(JSContext* cx, JSScript* script, JSObject* scope)
 {
 #ifdef DEBUG
     RootedObject enclosingScope(cx, script->enclosingStaticScope());
     for (StaticScopeIter<NoGC> i(enclosingScope); !i.done(); i++) {
         if (i.type() == StaticScopeIter<NoGC>::NonSyntactic) {
-            while (scope->is<DynamicWithObject>()) {
-                MOZ_ASSERT(!scope->as<DynamicWithObject>().isSyntactic());
-                scope = &scope->as<DynamicWithObject>().enclosingScope();
+            while (scope->is<DynamicWithObject>() || scope->is<NonSyntacticVariablesObject>()) {
+                MOZ_ASSERT(!IsSyntacticScope(scope));
+                scope = &scope->as<ScopeObject>().enclosingScope();
             }
         } else if (i.hasSyntacticDynamicScopeObject()) {
             switch (i.type()) {
               case StaticScopeIter<NoGC>::Function:
                 MOZ_ASSERT(scope->as<CallObject>().callee().nonLazyScript() == i.funScript());
                 scope = &scope->as<CallObject>().enclosingScope();
                 break;
               case StaticScopeIter<NoGC>::Block:
@@ -180,19 +180,17 @@ AssertDynamicScopeMatchesStaticScope(JSC
                 break;
               case StaticScopeIter<NoGC>::NonSyntactic:
                 MOZ_CRASH("NonSyntactic should not have a syntactic scope");
                 break;
             }
         }
     }
 
-    // The scope chain is always ended by one or more non-syntactic
-    // ScopeObjects (viz. GlobalObject or an unqualified varobj).
-    MOZ_ASSERT(!IsSyntacticScope(scope));
+    MOZ_ASSERT(scope->is<GlobalObject>() || scope->is<DebugScopeObject>());
 #endif
 }
 
 bool
 InterpreterFrame::initFunctionScopeObjects(JSContext* cx)
 {
     CallObject* callobj = CallObject::createForFunction(cx, this);
     if (!callobj)