Bug 1397385 - Refactor js::GetThisValue r=jandem
authorTed Campbell <tcampbell@mozilla.com>
Thu, 07 Sep 2017 23:40:26 -0400
changeset 379747 7e2a1d9ffd7a518ea047dc96ce4d84ac18eeb706
parent 379746 cae034ec1d40a07972cdfddf41f7328998544a1c
child 379748 aac13bc10a7265c4c6ac073d3678fa5edc22e60d
push id50775
push usertcampbell@mozilla.com
push dateFri, 08 Sep 2017 14:57:36 +0000
treeherderautoland@0d384af83d69 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1397385
milestone57.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 1397385 - Refactor js::GetThisValue r=jandem Split GetThisValue into three concerns: 1) Sanity check |this| and convert Window to WindowProxy 2) Find the |this| of an extensible LexicalEnvironmentObject 3) Find the target of a WithEnvironmentObject MozReview-Commit-ID: I2U54IxClSy
js/src/jscompartment.cpp
js/src/jsobj.cpp
js/src/jsobj.h
js/src/vm/EnvironmentObject.cpp
js/src/vm/Interpreter.cpp
--- a/js/src/jscompartment.cpp
+++ b/js/src/jscompartment.cpp
@@ -577,17 +577,27 @@ JSCompartment::getOrCreateNonSyntacticLe
     RootedObject key(cx, enclosing);
     if (enclosing->is<WithEnvironmentObject>()) {
         MOZ_ASSERT(!enclosing->as<WithEnvironmentObject>().isSyntactic());
         key = &enclosing->as<WithEnvironmentObject>().object();
     }
     RootedObject lexicalEnv(cx, nonSyntacticLexicalEnvironments_->lookup(key));
 
     if (!lexicalEnv) {
-        lexicalEnv = LexicalEnvironmentObject::createNonSyntactic(cx, enclosing, enclosing);
+        // NOTE: The default global |this| value is set to key for compatibility
+        // with existing users of the lexical environment cache.
+        //  - When used by shared-global JSM loader, |this| must be the
+        //    NonSyntacticVariablesObject passed as enclosing.
+        //  - When used by SubscriptLoader, |this| must be the target object of
+        //    the WithEnvironmentObject wrapper.
+        //  - When used by XBL/DOM Events, we execute directly as a function and
+        //    do not access the |this| value.
+        // See js::GetFunctionThis / js::GetNonSyntacticGlobalThis
+        MOZ_ASSERT(key->is<NonSyntacticVariablesObject>() || !key->is<EnvironmentObject>());
+        lexicalEnv = LexicalEnvironmentObject::createNonSyntactic(cx, enclosing, /*thisv = */key);
         if (!lexicalEnv)
             return nullptr;
         if (!nonSyntacticLexicalEnvironments_->add(cx, key, lexicalEnv))
             return nullptr;
     }
 
     return &lexicalEnv->as<LexicalEnvironmentObject>();
 }
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -3318,30 +3318,42 @@ js::ToObjectSlow(JSContext* cx, JS::Hand
 
 Value
 js::GetThisValue(JSObject* obj)
 {
     if (obj->is<GlobalObject>())
         return ObjectValue(*ToWindowProxyIfWindow(obj));
 
     if (obj->is<LexicalEnvironmentObject>()) {
-        if (!obj->as<LexicalEnvironmentObject>().isExtensible())
-            return UndefinedValue();
-        return obj->as<LexicalEnvironmentObject>().thisValue();
+        MOZ_ASSERT(!obj->as<LexicalEnvironmentObject>().isExtensible());
+        return UndefinedValue();
     }
 
     if (obj->is<ModuleEnvironmentObject>())
         return UndefinedValue();
 
-    if (obj->is<WithEnvironmentObject>())
-        return ObjectValue(*obj->as<WithEnvironmentObject>().withThis());
+    MOZ_ASSERT(!obj->is<WithEnvironmentObject>());
 
     return ObjectValue(*obj);
 }
 
+Value
+js::GetThisValueOfLexical(JSObject* env)
+{
+    MOZ_ASSERT(IsExtensibleLexicalEnvironment(env));
+    return env->as<LexicalEnvironmentObject>().thisValue();
+}
+
+Value
+js::GetThisValueOfWith(JSObject* env)
+{
+    MOZ_ASSERT(env->is<WithEnvironmentObject>());
+    return GetThisValue(env->as<WithEnvironmentObject>().withThis());
+}
+
 class GetObjectSlotNameFunctor : public JS::CallbackTracer::ContextFunctor
 {
     JSObject* obj;
 
   public:
     explicit GetObjectSlotNameFunctor(JSObject* ctx) : obj(ctx) {}
     virtual void operator()(JS::CallbackTracer* trc, char* buf, size_t bufsize) override;
 };
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -1090,29 +1090,31 @@ ToPrimitive(JSContext* cx, JSType prefer
 /*
  * toString support. (This isn't called GetClassName because there's a macro in
  * <windows.h> with that name.)
  */
 MOZ_ALWAYS_INLINE const char*
 GetObjectClassName(JSContext* cx, HandleObject obj);
 
 /*
- * Return an object that may be used as `this` in place of obj. For most
- * objects this just returns obj.
+ * Prepare a |this| value to be returned to script. This includes replacing
+ * Windows with their corresponding WindowProxy.
  *
- * Some JSObjects shouldn't be exposed directly to script. This includes (at
- * least) WithEnvironmentObjects and Window objects. However, since both of
- * those can be on scope chains, we sometimes would expose those as `this` if
- * we were not so vigilant about calling GetThisValue where appropriate.
- *
- * See comments at ComputeImplicitThis.
+ * Helpers are also provided to first extract the |this| from specific
+ * types of environment.
  */
 Value
 GetThisValue(JSObject* obj);
 
+Value
+GetThisValueOfLexical(JSObject* env);
+
+Value
+GetThisValueOfWith(JSObject* env);
+
 /* * */
 
 typedef JSObject* (*ClassInitializerOp)(JSContext* cx, JS::HandleObject obj);
 
 /* Fast access to builtin constructors and prototypes. */
 bool
 GetBuiltinConstructor(JSContext* cx, JSProtoKey key, MutableHandleObject objp);
 
--- a/js/src/vm/EnvironmentObject.cpp
+++ b/js/src/vm/EnvironmentObject.cpp
@@ -1064,21 +1064,21 @@ LexicalEnvironmentObject::isExtensible()
 }
 
 Value
 LexicalEnvironmentObject::thisValue() const
 {
     MOZ_ASSERT(isExtensible());
     Value v = getReservedSlot(THIS_VALUE_OR_SCOPE_SLOT);
     if (v.isObject()) {
-        // If `v` is a Window, return the WindowProxy instead. We called
-        // GetThisValue (which also does ToWindowProxyIfWindow) when storing
-        // the value in THIS_VALUE_OR_SCOPE_SLOT, but it's possible the
-        // WindowProxy was attached to the global *after* we set
-        // THIS_VALUE_OR_SCOPE_SLOT.
+        // A WindowProxy may have been attached after this environment was
+        // created so check ToWindowProxyIfWindow again. For example,
+        // GlobalObject::createInternal will construct its lexical environment
+        // before SetWindowProxy can be called.
+        // See also: js::GetThisValue / js::GetThisValueOfLexical
         return ObjectValue(*ToWindowProxyIfWindow(&v.toObject()));
     }
     return v;
 }
 
 const Class LexicalEnvironmentObject::class_ = {
     "LexicalEnvironment",
     JSCLASS_HAS_RESERVED_SLOTS(LexicalEnvironmentObject::RESERVED_SLOTS) |
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -127,23 +127,24 @@ js::GetFunctionThis(JSContext* cx, Abstr
     }
 
     RootedValue thisv(cx, frame.thisArgument());
 
     // If there is a NSVO on environment chain, use it as basis for fallback
     // global |this|. This gives a consistent definition of global lexical
     // |this| between function and global contexts.
     //
-    // NOTE: If only non-syntactic WithEnvironments are on the chain, we use
-    // the global lexical |this| value.
+    // NOTE: If only non-syntactic WithEnvironments are on the chain, we use the
+    // global lexical |this| value. This is for compatibility with the Subscript
+    // Loader.
     if (frame.script()->hasNonSyntacticScope() && thisv.isNullOrUndefined()) {
         RootedObject env(cx, frame.environmentChain());
         while (true) {
             if (IsNSVOLexicalEnvironment(env) || IsGlobalLexicalEnvironment(env)) {
-                res.set(GetThisValue(env));
+                res.set(GetThisValueOfLexical(env));
                 return true;
             }
             if (!env->enclosingEnvironment()) {
                 // This can only happen in Debugger eval frames: in that case we
                 // don't always have a global lexical env, see EvaluateInEnv.
                 MOZ_ASSERT(env->is<GlobalObject>());
                 res.set(GetThisValue(env));
                 return true;
@@ -156,17 +157,17 @@ js::GetFunctionThis(JSContext* cx, Abstr
 }
 
 void
 js::GetNonSyntacticGlobalThis(JSContext* cx, HandleObject envChain, MutableHandleValue res)
 {
     RootedObject env(cx, envChain);
     while (true) {
         if (IsExtensibleLexicalEnvironment(env)) {
-            res.set(env->as<LexicalEnvironmentObject>().thisValue());
+            res.set(GetThisValueOfLexical(env));
             return;
         }
         if (!env->enclosingEnvironment()) {
             // This can only happen in Debugger eval frames: in that case we
             // don't always have a global lexical env, see EvaluateInEnv.
             MOZ_ASSERT(env->is<GlobalObject>());
             res.set(GetThisValue(env));
             return;
@@ -1490,16 +1491,19 @@ static inline Value
 ComputeImplicitThis(JSObject* obj)
 {
     if (obj->is<GlobalObject>())
         return UndefinedValue();
 
     if (obj->is<NonSyntacticVariablesObject>())
         return UndefinedValue();
 
+    if (obj->is<WithEnvironmentObject>())
+        return GetThisValueOfWith(obj);
+
     if (IsCacheableEnvironment(obj))
         return UndefinedValue();
 
     // Debugger environments need special casing, as despite being
     // non-syntactic, they wrap syntactic environments and should not be
     // treated like other embedding-specific non-syntactic environments.
     if (obj->is<DebugEnvironmentProxy>())
         return ComputeImplicitThis(&obj->as<DebugEnvironmentProxy>().environment());