Backed out changeset 559f43c43369 (bug 1341061)
authorSebastian Hengst <archaeopteryx@coole-files.de>
Fri, 24 Feb 2017 23:10:16 +0100
changeset 489622 9e6402a66c388736bb193fcd4c9d40594785ca4b
parent 489621 39be2cc943a7c3b48104811065ae0768de8eeaf6
child 489623 49187b2a839656915a614feac8de963bbe35483e
push id46871
push userbmo:sledru@mozilla.com
push dateSat, 25 Feb 2017 12:16:21 +0000
bugs1341061
milestone54.0a1
backs out559f43c4336928f676bc2a35b196932984d2b498
Backed out changeset 559f43c43369 (bug 1341061)
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
@@ -4068,65 +4068,42 @@ 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->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
+        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
             return false;
 
         if (post && emittedBindOp) {
-            if (!bce->emit2(JSOP_PICK, 2))                 // N? N+1 ENV?
-                return false;
-            if (!bce->emit1(JSOP_SWAP))                    // N? ENV? N+1
+            if (!bce->emit2(JSOP_PICK, 2))                 // N? N+1 SCOPE?
+                return false;
+            if (!bce->emit1(JSOP_SWAP))                    // N? SCOPE? N+1
                 return false;
         }
 
         return true;
     };
 
     if (!emitSetName(pn->pn_kid, emitRhs))
         return false;
@@ -5912,18 +5889,29 @@ 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 (!bce->emitGetNameAtLocationForCompoundAssignment(lhs->name(), lhsLoc))
-                    return false;
+                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;
+                }
             }
 
             // 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,18 +540,16 @@ 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 env(cx, &val.toObject());
+            RootedObject obj(cx, &val.toObject());
             RootedId id(cx, NameToId(name));
-            if (!GetNameBoundInEnvironment(cx, env, id, res))
+            if (!GetPropertyForNameLookup(cx, obj, 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,24 +1049,16 @@ 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,17 +169,21 @@ 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) {
@@ -193,17 +197,19 @@ 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, MaybeUnwrapSyntacticWithEnvironment(receiver));
+        RootedObject normalized(cx, receiver);
+        if (normalized->is<WithEnvironmentObject>() && !shape->hasDefaultGetter())
+            normalized = &normalized->as<WithEnvironmentObject>().object();
         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*> env(&rootObject0, &REGS.sp[-1].toObject());
+    ReservedRooted<JSObject*> obj(&rootObject0, &REGS.sp[-1].toObject());
     ReservedRooted<jsid> id(&rootId0, NameToId(script->getName(REGS.pc)));
     MutableHandleValue rval = REGS.stackHandleAt(-1);
-    if (!GetNameBoundInEnvironment(cx, env, id, rval))
+    if (!GetPropertyForNameLookup(cx, obj, 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,34 +2120,22 @@ 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::GetNameBoundInEnvironment(JSContext* cx, HandleObject envArg, HandleId id, MutableHandleValue vp)
+js::GetPropertyForNameLookup(JSContext* cx, HandleObject obj, HandleId id, MutableHandleValue 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);
+    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);
 }
 
 
 /*** [[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
-GetNameBoundInEnvironment(JSContext* cx, HandleObject env, HandleId id, MutableHandleValue vp);
+GetPropertyForNameLookup(JSContext* cx, HandleObject obj, HandleId id, MutableHandleValue vp);
 
 } /* namespace js */
 
 template <>
 inline bool
 JSObject::is<js::NativeObject>() const { return isNative(); }
 
 namespace js {