Bug 1260577 - Fix |obj[expr] += e2| erroneously calling expr.toString() twice. (r=till)
authorEric Faust <efaustbmo@gmail.com>
Tue, 29 Mar 2016 16:46:20 -0700
changeset 291006 cd2868983231ad1dcf5c4c9e12d0f03b12cbf950
parent 291005 928fe146f1c10b2d439f2b088e242229b9b05350
child 291007 26a75fc6f8c74255ad350f62f3dd122e89c8b148
push id19656
push usergwagner@mozilla.com
push dateMon, 04 Apr 2016 13:43:23 +0000
treeherderb2g-inbound@e99061fde28a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstill
bugs1260577
milestone48.0a1
Bug 1260577 - Fix |obj[expr] += e2| erroneously calling expr.toString() twice. (r=till)
js/src/frontend/BytecodeEmitter.cpp
js/src/frontend/BytecodeEmitter.h
js/src/tests/ecma_6/Statements/property-reference-self-assignment.js
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -2815,18 +2815,23 @@ BytecodeEmitter::emitElemOperands(ParseN
     } else if (opts == EmitElemOption::Call) {
         if (!emit1(JSOP_DUP))
             return false;
     }
 
     if (!emitTree(pn->pn_right))
         return false;
 
-    if (opts == EmitElemOption::Set && !emit2(JSOP_PICK, 2))
-        return false;
+    if (opts == EmitElemOption::Set) {
+        if (!emit2(JSOP_PICK, 2))
+            return false;
+    } else if (opts == EmitElemOption::IncDec || opts == EmitElemOption::SelfAssign) {
+        if (!emit1(JSOP_TOID))
+            return false;
+    }
     return true;
 }
 
 bool
 BytecodeEmitter::emitSuperElemOperands(ParseNode* pn, EmitElemOption opts)
 {
     MOZ_ASSERT(pn->isKind(PNK_ELEM) && pn->as<PropertyByValue>().isSuper());
 
@@ -2835,18 +2840,20 @@ BytecodeEmitter::emitSuperElemOperands(P
     // Since JSOP_THIS might throw in derived class constructors, we cannot
     // just push it earlier as the receiver. We have to swap it down instead.
 
     if (!emitTree(pn->pn_right))
         return false;
 
     // We need to convert the key to an object id first, so that we do not do
     // it inside both the GETELEM and the SETELEM.
-    if (opts == EmitElemOption::IncDec && !emit1(JSOP_TOID))
-        return false;
+    if (opts == EmitElemOption::IncDec || opts == EmitElemOption::SelfAssign) {
+        if (!emit1(JSOP_TOID))
+            return false;
+    }
 
     if (!emitGetThisForSuperBase(pn->pn_left))
         return false;
 
     if (opts == EmitElemOption::Call) {
         if (!emit1(JSOP_SWAP))
             return false;
 
@@ -2908,16 +2915,19 @@ BytecodeEmitter::emitSuperElemOp(ParseNo
 
 bool
 BytecodeEmitter::emitElemIncDec(ParseNode* pn)
 {
     MOZ_ASSERT(pn->pn_kid->isKind(PNK_ELEM));
 
     bool isSuper = pn->pn_kid->as<PropertyByValue>().isSuper();
 
+    // We need to convert the key to an object id first, so that we do not do
+    // it inside both the GETELEM and the SETELEM. This is done by
+    // emit(Super)ElemOperands.
     if (isSuper) {
         if (!emitSuperElemOperands(pn->pn_kid, EmitElemOption::IncDec))
             return false;
     } else {
         if (!emitElemOperands(pn->pn_kid, EmitElemOption::IncDec))
             return false;
     }
 
@@ -2931,22 +2941,17 @@ BytecodeEmitter::emitElemIncDec(ParseNod
         if (!emitDupAt(2))                              // KEY THIS OBJ KEY
             return false;
         if (!emitDupAt(2))                              // KEY THIS OBJ KEY THIS
             return false;
         if (!emitDupAt(2))                              // KEY THIS OBJ KEY THIS OBJ
             return false;
         getOp = JSOP_GETELEM_SUPER;
     } else {
-        // We need to convert the key to an object id first, so that we do not do
-        // it inside both the GETELEM and the SETELEM. In the super case, this is
-        // done by emitSuperElemOperands.
-                                                        // OBJ KEY*
-        if (!emit1(JSOP_TOID))                          // OBJ KEY
-            return false;
+                                                        // OBJ KEY
         if (!emit1(JSOP_DUP2))                          // OBJ KEY OBJ KEY
             return false;
         getOp = JSOP_GETELEM;
     }
     if (!emitElemOpBase(getOp))                         // OBJ KEY V
         return false;
     if (!emit1(JSOP_POS))                               // OBJ KEY N
         return false;
@@ -4596,30 +4601,30 @@ BytecodeEmitter::emitAssignment(ParseNod
         } else {
             if (!emitTree(lhs->expr()))
                 return false;
             offset += 1;
         }
         if (!makeAtomIndex(lhs->pn_atom, &atomIndex))
             return false;
         break;
-      case PNK_ELEM:
+      case PNK_ELEM: {
         MOZ_ASSERT(lhs->isArity(PN_BINARY));
+        EmitElemOption opt = op == JSOP_NOP ? EmitElemOption::Get : EmitElemOption::SelfAssign;
         if (lhs->as<PropertyByValue>().isSuper()) {
-            if (!emitSuperElemOperands(lhs))
+            if (!emitSuperElemOperands(lhs, opt))
                 return false;
             offset += 3;
         } else {
-            if (!emitTree(lhs->pn_left))
-                return false;
-            if (!emitTree(lhs->pn_right))
+            if (!emitElemOperands(lhs, opt))
                 return false;
             offset += 2;
         }
         break;
+      }
       case PNK_ARRAY:
       case PNK_OBJECT:
         break;
       case PNK_CALL:
         MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL);
         if (!emitTree(lhs))
             return false;
         if (!emit1(JSOP_POP))
--- a/js/src/frontend/BytecodeEmitter.h
+++ b/js/src/frontend/BytecodeEmitter.h
@@ -514,17 +514,17 @@ struct BytecodeEmitter
     bool emitPropOp(ParseNode* pn, JSOp op);
     bool emitPropIncDec(ParseNode* pn);
 
     bool emitComputedPropertyName(ParseNode* computedPropName);
 
     // Emit bytecode to put operands for a JSOP_GETELEM/CALLELEM/SETELEM/DELELEM
     // opcode onto the stack in the right order. In the case of SETELEM, the
     // value to be assigned must already be pushed.
-    enum class EmitElemOption { Get, Set, Call, IncDec };
+    enum class EmitElemOption { Get, Set, Call, IncDec, SelfAssign };
     bool emitElemOperands(ParseNode* pn, EmitElemOption opts);
 
     bool emitElemOpBase(JSOp op);
     bool emitElemOp(ParseNode* pn, JSOp op);
     bool emitElemIncDec(ParseNode* pn);
 
     bool emitCatch(ParseNode* pn);
     bool emitIf(ParseNode* pn);
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_6/Statements/property-reference-self-assignment.js
@@ -0,0 +1,38 @@
+var hits = 0;
+
+var p = { toString() { hits++; return "prop" } };
+var obj = { foo: 1 };
+
+
+var ops = [["obj[p]++", 2],
+           ["++obj[p]", 2],
+           ["--obj[p]", 0],
+           ["obj[p]--", 0],
+           ["obj[p] += 2", 3],
+           ["obj[p] -= 2", -1],
+           ["obj[p] *= 2", 2],
+           ["obj[p] /= 2", 0.5],
+           ["obj[p] %= 2", 1],
+           ["obj[p] >>>= 2", 0],
+           ["obj[p] >>= 2", 0],
+           ["obj[p] <<= 2", 4],
+           ["obj[p] |= 2", 3],
+           ["obj[p] ^= 2", 3],
+           ["obj[p] &= 2", 0]];
+
+var testHits = 0;
+for (let op of ops) {
+    // Seed the value for each test.
+    obj.prop = 1;
+
+    // Do the operation.
+    eval(op[0]);
+    assertEq(obj.prop, op[1]);
+
+    // We should always call toString once, for each operation.
+    testHits++;
+    assertEq(hits, testHits);
+}
+
+if (typeof reportCompare === 'function')
+    reportCompare(0,0,"OK");