Bug 1165486 - Replace the PlainObj varobj with NonSyntacticVariablesObject. (r=luke)

js/src/builtin/Eval.cpp

@@ -502,24 +502,17 @@ js::ExecuteInGlobalAndReturnScope(JSContext* cx, HandleObject global, HandleScri
         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()))
     {

js/src/jit-test/tests/debug/onNewScript-ExecuteInGlobalAndReturnScope.js

@@ -4,6 +4,7 @@ var g = newGlobal();
 var g2 = newGlobal();
 var dbg = new Debugger(g, g2);
 var log = '';
+var canary = 42;
 
 dbg.onNewScript = function (evalScript) {
   log += 'e';
@@ -24,5 +25,8 @@ dbg.onDebuggerStatement = function (frame) {
 };
 
 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');
+

js/src/jsapi.cpp

@@ -4138,11 +4138,8 @@ CompileFunction(JSContext* cx, const ReadOnlyCompileOptions& optionsArg,
                   HasNonSyntacticStaticScopeChain(enclosingStaticScope));
 
     CompileOptions options(cx, optionsArg);
-    if (!frontend::CompileFunctionBody(cx, fun, options, formals, srcBuf,
+    if (!frontend::CompileFunctionBody(cx, fun, options, formals, srcBuf, enclosingStaticScope))
-                                       enclosingStaticScope))
-    {
         return false;
-    }
 
     return true;
 }

js/src/jsfun.cpp

@@ -1978,6 +1978,20 @@ js::NewScriptedFunction(ExclusiveContext* cx, unsigned nargs,
                                 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,
@@ -1987,6 +2001,7 @@ js::NewFunctionWithProto(ExclusiveContext* cx, Native native,
 {
     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
@@ -1994,17 +2009,6 @@ js::NewFunctionWithProto(ExclusiveContext* cx, Native native,
     // 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)
@@ -2067,22 +2071,6 @@ js::CanReuseScriptForClone(JSCompartment* compartment, HandleFunction fun,
            (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)
@@ -2126,7 +2114,7 @@ js::CloneFunctionReuseScript(JSContext* cx, HandleFunction fun, HandleObject par
                              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));
 
@@ -2161,7 +2149,7 @@ js::CloneFunctionAndScript(JSContext* cx, HandleFunction fun, HandleObject paren
                            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));

js/src/jsobjinlines.h

@@ -224,6 +224,9 @@ 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);
 }
 
@@ -232,7 +235,9 @@ 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 {

js/src/vm/ScopeObject.cpp

@@ -570,6 +570,31 @@ const Class StaticNonSyntacticScopeObjects::class_ = {
     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*

js/src/vm/ScopeObject.h

@@ -399,17 +399,9 @@ class StaticEvalObject : public ScopeObject
 // scopes on the dynamic scope chain.
 //
 // There are two flavors of polluting global scopes on the dynamic scope
-// chain:
+// chain: either 0+ non-syntactic DynamicWithObjects, or 1
-//
+// NonSyntacticVariablesObject, created exclusively in
-// 1. 0+ non-syntactic DynamicWithObjects. This static scope helps ScopeIter
+// js::ExecuteInGlobalAndReturnScope.
-//    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.
 class StaticNonSyntacticScopeObjects : public ScopeObject
 {
   public:
@@ -423,6 +415,22 @@ class StaticNonSyntacticScopeObjects : public ScopeObject
     }
 };
 
+// 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:
@@ -1040,7 +1048,8 @@ 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<>
@@ -1072,8 +1081,8 @@ inline bool
 IsSyntacticScope(JSObject* scope)
 {
     return scope->is<ScopeObject>() &&
-           (!scope->is<DynamicWithObject>() ||
+           (!scope->is<DynamicWithObject>() || scope->as<DynamicWithObject>().isSyntactic()) &&
-            scope->as<DynamicWithObject>().isSyntactic());
+           !scope->is<NonSyntacticVariablesObject>();
 }
 
 inline const Value&
@@ -1111,7 +1120,8 @@ ScopeIter::hasNonSyntacticScopeObject() const
     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;
 }
@@ -1141,6 +1151,9 @@ ScopeIter::canHaveSyntacticScopeObject() const
       case NonSyntactic:
         return false;
     }
+
+    // Silence warnings.
+    return false;
 }
 
 inline JSObject&

js/src/vm/Stack.cpp

@@ -154,9 +154,9 @@ AssertDynamicScopeMatchesStaticScope(JSContext* cx, JSScript* script, JSObject*
     RootedObject enclosingScope(cx, script->enclosingStaticScope());
     for (StaticScopeIter<NoGC> i(enclosingScope); !i.done(); i++) {
         if (i.type() == StaticScopeIter<NoGC>::NonSyntactic) {
-            while (scope->is<DynamicWithObject>()) {
+            while (scope->is<DynamicWithObject>() || scope->is<NonSyntacticVariablesObject>()) {
-                MOZ_ASSERT(!scope->as<DynamicWithObject>().isSyntactic());
+                MOZ_ASSERT(!IsSyntacticScope(scope));
-                scope = &scope->as<DynamicWithObject>().enclosingScope();
+                scope = &scope->as<ScopeObject>().enclosingScope();
             }
         } else if (i.hasSyntacticDynamicScopeObject()) {
             switch (i.type()) {
@@ -185,9 +185,7 @@ AssertDynamicScopeMatchesStaticScope(JSContext* cx, JSScript* script, JSObject*
         }
     }
 
-    // The scope chain is always ended by one or more non-syntactic
+    MOZ_ASSERT(scope->is<GlobalObject>() || scope->is<DebugScopeObject>());
-    // ScopeObjects (viz. GlobalObject or an unqualified varobj).
-    MOZ_ASSERT(!IsSyntacticScope(scope));
 #endif
 }