Bug 1341061 - Manually unwrap WithEnvironmentObjects in GETBOUNDNAME. (r=arai)
☠☠ backed out by 9e6402a66c38 ☠ ☠
authorShu-yu Guo <shu@rfrn.org>
Fri, 24 Feb 2017 12:52:13 -0800
changeset 373915 559f43c4336928f676bc2a35b196932984d2b498
parent 373914 76c74d43a9b0d19b0d5b229f3b7ae264edacc23b
child 373916 456c1dcfe087675f3ee80ce68ccb88ce56021cbc
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersarai
bugs1341061
milestone54.0a1
Bug 1341061 - Manually unwrap WithEnvironmentObjects in GETBOUNDNAME. (r=arai) Also refactor some stuff in this area. Documenting why I think it's correct to use GETBOUNDNAME only for dynamic (vs global) lookups, without the normal NAME checks: 1. GETBOUNDNAME doesn't need to check TDZ because BINDNAME does it already, and GETBOUNDNAME is always preceded by BINDNAME. 2. '.this' doesn't need to be checked, because '.this' can't be assigned to in a compound assignment or inc/dec. 3. For a global name there is never @@unscopables on the global environments, so GNAME ops, while doing repeated lookups, remain valid optimizations because the repetition is unobservable. 4. We *will* do double @@unscopable lookup in the case of a script compiled for a syntactic scope then run under a non-syntactic WithEnvironmentObject (e.g. like frame scripts). But this is Gecko-specific and outside the spec, so we can do whatever we want here.
js/src/frontend/BytecodeEmitter.cpp
js/src/frontend/BytecodeEmitter.h
js/src/jit/SharedIC.cpp
js/src/vm/EnvironmentObject.h
js/src/vm/Interpreter-inl.h
js/src/vm/Interpreter.cpp
js/src/vm/NativeObject.cpp
js/src/vm/NativeObject.h
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -4042,42 +4042,65 @@ BytecodeEmitter::emitPropIncDec(ParseNod
         return false;
     if (post && !emit1(JSOP_POP))                   // RESULT
         return false;
 
     return true;
 }
 
 bool
+BytecodeEmitter::emitGetNameAtLocationForCompoundAssignment(JSAtom* name, const NameLocation& loc)
+{
+    if (loc.kind() == NameLocation::Kind::Dynamic) {
+        // For dynamic accesses we need to emit GETBOUNDNAME instead of
+        // GETNAME for correctness: looking up @@unscopables on the
+        // environment chain (due to 'with' environments) must only happen
+        // once.
+        //
+        // GETBOUNDNAME uses the environment already pushed on the stack from
+        // the earlier BINDNAME.
+        if (!emit1(JSOP_DUP))                              // ENV ENV
+            return false;
+        if (!emitAtomOp(name, JSOP_GETBOUNDNAME))          // ENV V
+            return false;
+    } else {
+        if (!emitGetNameAtLocation(name, loc))             // ENV? V
+            return false;
+    }
+
+    return true;
+}
+
+bool
 BytecodeEmitter::emitNameIncDec(ParseNode* pn)
 {
     MOZ_ASSERT(pn->pn_kid->isKind(PNK_NAME));
 
     bool post;
     JSOp binop = GetIncDecInfo(pn->getKind(), &post);
 
     auto emitRhs = [pn, post, binop](BytecodeEmitter* bce, const NameLocation& loc,
                                      bool emittedBindOp)
     {
         JSAtom* name = pn->pn_kid->name();
-        if (!bce->emitGetNameAtLocation(name, loc, false)) // SCOPE? V
-            return false;
-        if (!bce->emit1(JSOP_POS))                         // SCOPE? N
-            return false;
-        if (post && !bce->emit1(JSOP_DUP))                 // SCOPE? N? N
-            return false;
-        if (!bce->emit1(JSOP_ONE))                         // SCOPE? N? N 1
-            return false;
-        if (!bce->emit1(binop))                            // SCOPE? N? N+1
+        if (!bce->emitGetNameAtLocationForCompoundAssignment(name, loc)) // ENV? V
+            return false;
+        if (!bce->emit1(JSOP_POS))                         // ENV? N
+            return false;
+        if (post && !bce->emit1(JSOP_DUP))                 // ENV? N? N
+            return false;
+        if (!bce->emit1(JSOP_ONE))                         // ENV? N? N 1
+            return false;
+        if (!bce->emit1(binop))                            // ENV? N? N+1
             return false;
 
         if (post && emittedBindOp) {
-            if (!bce->emit2(JSOP_PICK, 2))                 // N? N+1 SCOPE?
-                return false;
-            if (!bce->emit1(JSOP_SWAP))                    // N? SCOPE? N+1
+            if (!bce->emit2(JSOP_PICK, 2))                 // N? N+1 ENV?
+                return false;
+            if (!bce->emit1(JSOP_SWAP))                    // N? ENV? N+1
                 return false;
         }
 
         return true;
     };
 
     if (!emitSetName(pn->pn_kid, emitRhs))
         return false;
@@ -5860,29 +5883,18 @@ BytecodeEmitter::emitAssignment(ParseNod
     // to emit BINDNAME is involved and should avoid duplication.
     if (lhs->isKind(PNK_NAME)) {
         auto emitRhs = [op, lhs, rhs](BytecodeEmitter* bce, const NameLocation& lhsLoc,
                                       bool emittedBindOp)
         {
             // For compound assignments, first get the LHS value, then emit
             // the RHS and the op.
             if (op != JSOP_NOP) {
-                if (lhsLoc.kind() == NameLocation::Kind::Dynamic) {
-                    // For dynamic accesses we can do better than a GETNAME
-                    // since the assignment already emitted a BINDNAME on the
-                    // top of the stack. As an optimization, use that to get
-                    // the name.
-                    if (!bce->emit1(JSOP_DUP))
-                        return false;
-                    if (!bce->emitAtomOp(lhs, JSOP_GETBOUNDNAME))
-                        return false;
-                } else {
-                    if (!bce->emitGetNameAtLocation(lhs->name(), lhsLoc))
-                        return false;
-                }
+                if (!bce->emitGetNameAtLocationForCompoundAssignment(lhs->name(), lhsLoc))
+                    return false;
             }
 
             // Emit the RHS. If we emitted a BIND[G]NAME, then the scope is on
             // the top of the stack and we need to pick the right RHS value.
             if (!EmitAssignmentRhs(bce, rhs, emittedBindOp ? 2 : 1))
                 return false;
 
             if (!lhs->isInParens() && op == JSOP_NOP && rhs && rhs->isDirectRHSAnonFunction()) {
--- a/js/src/frontend/BytecodeEmitter.h
+++ b/js/src/frontend/BytecodeEmitter.h
@@ -540,16 +540,18 @@ struct MOZ_STACK_CLASS BytecodeEmitter
     // used as a non-asserting version of emitUint16Operand.
     MOZ_MUST_USE bool emitLocalOp(JSOp op, uint32_t slot);
 
     MOZ_MUST_USE bool emitArgOp(JSOp op, uint16_t slot);
     MOZ_MUST_USE bool emitEnvCoordOp(JSOp op, EnvironmentCoordinate ec);
 
     MOZ_MUST_USE bool emitGetNameAtLocation(JSAtom* name, const NameLocation& loc,
                                             bool callContext = false);
+    MOZ_MUST_USE bool emitGetNameAtLocationForCompoundAssignment(JSAtom* name,
+                                                                 const NameLocation& loc);
     MOZ_MUST_USE bool emitGetName(JSAtom* name, bool callContext = false) {
         return emitGetNameAtLocation(name, lookupName(name), callContext);
     }
     MOZ_MUST_USE bool emitGetName(ParseNode* pn, bool callContext = false);
 
     template <typename RHSEmitter>
     MOZ_MUST_USE bool emitSetOrInitializeNameAtLocation(HandleAtom name, const NameLocation& loc,
                                                         RHSEmitter emitRhs, bool initialize);
--- a/js/src/jit/SharedIC.cpp
+++ b/js/src/jit/SharedIC.cpp
@@ -1969,19 +1969,19 @@ ComputeGetPropResult(JSContext* cx, Base
             res.setInt32(frame->numActualArgs());
         } else {
             MOZ_ASSERT(name == cx->names().callee);
             MOZ_ASSERT(frame->script()->hasMappedArgsObj());
             res.setObject(*frame->callee());
         }
     } else {
         if (op == JSOP_GETBOUNDNAME) {
-            RootedObject obj(cx, &val.toObject());
+            RootedObject env(cx, &val.toObject());
             RootedId id(cx, NameToId(name));
-            if (!GetPropertyForNameLookup(cx, obj, id, res))
+            if (!GetNameBoundInEnvironment(cx, env, id, res))
                 return false;
         } else {
             MOZ_ASSERT(op == JSOP_GETPROP || op == JSOP_CALLPROP || op == JSOP_LENGTH);
             if (!GetProperty(cx, val, name, res))
                 return false;
         }
     }
 
--- a/js/src/vm/EnvironmentObject.h
+++ b/js/src/vm/EnvironmentObject.h
@@ -1049,16 +1049,24 @@ IsExtensibleLexicalEnvironment(JSObject*
 
 inline bool
 IsGlobalLexicalEnvironment(JSObject* env)
 {
     return env->is<LexicalEnvironmentObject>() &&
            env->as<LexicalEnvironmentObject>().isGlobal();
 }
 
+inline JSObject*
+MaybeUnwrapSyntacticWithEnvironment(JSObject* env)
+{
+    if (env->is<WithEnvironmentObject>() && env->as<WithEnvironmentObject>().isSyntactic())
+        return &env->as<WithEnvironmentObject>().object();
+    return env;
+}
+
 template <typename SpecificEnvironment>
 inline bool
 IsFrameInitialEnvironment(AbstractFramePtr frame, SpecificEnvironment& env)
 {
     // A frame's initial environment is the innermost environment
     // corresponding to the scope chain from frame.script()->bodyScope() to
     // frame.script()->outermostScope(). This environment must be on the chain
     // for the frame to be considered initialized. That is, it must be on the
--- a/js/src/vm/Interpreter-inl.h
+++ b/js/src/vm/Interpreter-inl.h
@@ -169,21 +169,17 @@ GetLengthProperty(const Value& lval, Mut
                 return true;
             }
         }
     }
 
     return false;
 }
 
-enum class GetNameMode
-{
-    Normal,
-    TypeOf
-};
+enum class GetNameMode { Normal, TypeOf };
 
 template <GetNameMode mode>
 inline bool
 FetchName(JSContext* cx, HandleObject receiver, HandleObject holder, HandlePropertyName name,
           Handle<PropertyResult> prop, MutableHandleValue vp)
 {
     if (!prop) {
         switch (mode) {
@@ -197,19 +193,17 @@ FetchName(JSContext* cx, HandleObject re
 
     /* Take the slow path if shape was not found in a native object. */
     if (!receiver->isNative() || !holder->isNative()) {
         Rooted<jsid> id(cx, NameToId(name));
         if (!GetProperty(cx, receiver, receiver, id, vp))
             return false;
     } else {
         RootedShape shape(cx, prop.shape());
-        RootedObject normalized(cx, receiver);
-        if (normalized->is<WithEnvironmentObject>() && !shape->hasDefaultGetter())
-            normalized = &normalized->as<WithEnvironmentObject>().object();
+        RootedObject normalized(cx, MaybeUnwrapSyntacticWithEnvironment(receiver));
         if (shape->isDataDescriptor() && shape->hasDefaultGetter()) {
             /* Fast path for Object instance properties. */
             MOZ_ASSERT(shape->hasSlot());
             vp.set(holder->as<NativeObject>().getSlot(shape->slot()));
         } else {
             if (!NativeGetExistingProperty(cx, normalized, holder.as<NativeObject>(), shape, vp))
                 return false;
         }
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -2677,20 +2677,20 @@ CASE(JSOP_GETPROP_SUPER)
         goto error;
 
     REGS.sp--;
 }
 END_CASE(JSOP_GETPROP_SUPER)
 
 CASE(JSOP_GETBOUNDNAME)
 {
-    ReservedRooted<JSObject*> obj(&rootObject0, &REGS.sp[-1].toObject());
+    ReservedRooted<JSObject*> env(&rootObject0, &REGS.sp[-1].toObject());
     ReservedRooted<jsid> id(&rootId0, NameToId(script->getName(REGS.pc)));
     MutableHandleValue rval = REGS.stackHandleAt(-1);
-    if (!GetPropertyForNameLookup(cx, obj, id, rval))
+    if (!GetNameBoundInEnvironment(cx, env, id, rval))
         goto error;
 
     TypeScript::Monitor(cx, script, REGS.pc, rval);
     assertSameCompartmentDebugOnly(cx, rval);
 }
 END_CASE(JSOP_GETBOUNDNAME)
 
 CASE(JSOP_SETINTRINSIC)
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -2120,22 +2120,34 @@ js::NativeGetProperty(JSContext* cx, Han
 bool
 js::NativeGetPropertyNoGC(JSContext* cx, NativeObject* obj, const Value& receiver, jsid id, Value* vp)
 {
     AutoAssertNoException noexc(cx);
     return NativeGetPropertyInline<NoGC>(cx, obj, receiver, id, NotNameLookup, vp);
 }
 
 bool
-js::GetPropertyForNameLookup(JSContext* cx, HandleObject obj, HandleId id, MutableHandleValue vp)
+js::GetNameBoundInEnvironment(JSContext* cx, HandleObject envArg, HandleId id, MutableHandleValue vp)
 {
-    RootedValue receiver(cx, ObjectValue(*obj));
-    if (obj->getOpsGetProperty())
-        return GeneralizedGetProperty(cx, obj, id, receiver, NameLookup, vp);
-    return NativeGetPropertyInline<CanGC>(cx, obj.as<NativeObject>(), receiver, id, NameLookup, vp);
+    // Manually unwrap 'with' environments to prevent looking up @@unscopables
+    // twice.
+    //
+    // This is unfortunate because internally, the engine does not distinguish
+    // HasProperty from HasBinding: both are implemented as a HasPropertyOp
+    // hook on a WithEnvironmentObject.
+    //
+    // In the case of attempting to get the value of a binding already looked
+    // up via BINDNAME, calling HasProperty on the WithEnvironmentObject is
+    // equivalent to calling HasBinding a second time. This results in the
+    // incorrect behavior of performing the @@unscopables check again.
+    RootedObject env(cx, MaybeUnwrapSyntacticWithEnvironment(envArg));
+    RootedValue receiver(cx, ObjectValue(*env));
+    if (env->getOpsGetProperty())
+        return GeneralizedGetProperty(cx, env, id, receiver, NameLookup, vp);
+    return NativeGetPropertyInline<CanGC>(cx, env.as<NativeObject>(), receiver, id, NameLookup, vp);
 }
 
 
 /*** [[Set]] *************************************************************************************/
 
 static bool
 MaybeReportUndeclaredVarAssignment(JSContext* cx, HandleString propname)
 {
--- a/js/src/vm/NativeObject.h
+++ b/js/src/vm/NativeObject.h
@@ -1388,17 +1388,17 @@ NativeLookupOwnProperty(JSContext* cx,
  */
 extern bool
 NativeGetExistingProperty(JSContext* cx, HandleObject receiver, HandleNativeObject obj,
                           HandleShape shape, MutableHandleValue vp);
 
 /* * */
 
 extern bool
-GetPropertyForNameLookup(JSContext* cx, HandleObject obj, HandleId id, MutableHandleValue vp);
+GetNameBoundInEnvironment(JSContext* cx, HandleObject env, HandleId id, MutableHandleValue vp);
 
 } /* namespace js */
 
 template <>
 inline bool
 JSObject::is<js::NativeObject>() const { return isNative(); }
 
 namespace js {