Bug 609756 - {Pre,Post}{in,de}crements on function calls must ToNumber after evaluating the call, in case ToNumber's effects are observable via toString/valueOf. r=arai
authorJeff Walden <jwalden@mit.edu>
Sun, 04 Sep 2016 12:07:58 -0700
changeset 312908 947563d9be7980bd602c5270fa4a98764461f0ed
parent 312907 8082a95eccd36bca0e0f59f53d514a7fe8baf5ea
child 312909 558bb8851eb8a861bb1d11ac352e1d8a5b7d9e85
push id30665
push usercbook@mozilla.com
push dateWed, 07 Sep 2016 15:20:43 +0000
treeherdermozilla-central@95acb9299faf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersarai
bugs609756
milestone51.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 609756 - {Pre,Post}{in,de}crements on function calls must ToNumber after evaluating the call, in case ToNumber's effects are observable via toString/valueOf. r=arai
js/src/frontend/BytecodeEmitter.cpp
js/src/frontend/BytecodeEmitter.h
js/src/frontend/FullParseHandler.h
js/src/frontend/ParseNode.h
js/src/frontend/Parser.cpp
js/src/frontend/Parser.h
js/src/frontend/SyntaxParseHandler.h
js/src/tests/ecma_5/extensions/inc-dec-functioncall.js
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -3420,16 +3420,18 @@ BytecodeEmitter::emitPropIncDec(ParseNod
         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
@@ -3644,16 +3646,37 @@ BytecodeEmitter::emitElemIncDec(ParseNod
         return false;
     if (post && !emit1(JSOP_POP))                       // RESULT
         return false;
 
     return true;
 }
 
 bool
+BytecodeEmitter::emitCallIncDec(ParseNode* incDec)
+{
+    MOZ_ASSERT(incDec->isKind(PNK_PREINCREMENT) ||
+               incDec->isKind(PNK_POSTINCREMENT) ||
+               incDec->isKind(PNK_PREDECREMENT) ||
+               incDec->isKind(PNK_POSTDECREMENT));
+
+    MOZ_ASSERT(incDec->pn_kid->isKind(PNK_CALL));
+
+    ParseNode* call = incDec->pn_kid;
+    if (!emitTree(call))                                // CALLRESULT
+        return false;
+    if (!emit1(JSOP_POS))                               // N
+        return false;
+
+    // The increment/decrement has no side effects, so proceed to throw for
+    // invalid assignment target.
+    return emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS);
+}
+
+bool
 BytecodeEmitter::emitNumberOp(double dval)
 {
     int32_t ival;
     if (NumberIsInt32(dval, &ival)) {
         if (ival == 0)
             return emit1(JSOP_ZERO);
         if (ival == 1)
             return emit1(JSOP_ONE);
@@ -4413,27 +4436,19 @@ BytecodeEmitter::emitDestructuringLHS(Pa
                 JSOp setOp = sc->strict() ? JSOP_STRICTSETELEM : JSOP_SETELEM;
                 if (!emitElemOp(target, setOp))
                     return false;
             }
             break;
           }
 
           case PNK_CALL:
-            MOZ_ASSERT(target->pn_xflags & PNX_SETCALL);
-            if (!emitTree(target))
-                return false;
-
-            // Pop the call return value. Below, we pop the RHS too, balancing
-            // the stack --- presumably for the benefit of bytecode
-            // analysis. (The interpreter will never reach these instructions
-            // since we just emitted JSOP_SETCALL, which always throws. It's
-            // possible no analyses actually depend on this either.)
-            if (!emit1(JSOP_POP))
-                return false;
+            MOZ_ASSERT_UNREACHABLE("Parser::reportIfNotValidSimpleAssignmentTarget "
+                                   "rejects function calls as assignment "
+                                   "targets in destructuring assignments");
             break;
 
           default:
             MOZ_CRASH("emitDestructuringLHS: bad lhs kind");
         }
 
         // Pop the assigned value.
         if (!emit1(JSOP_POP))
@@ -4939,19 +4954,25 @@ BytecodeEmitter::emitAssignment(ParseNod
             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;
+
+        // Assignment to function calls is forbidden, but we have to make the
+        // call first.  Now we can throw.
+        if (!emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS))
+            return false;
+
+        // Rebalance the stack to placate stack-depth assertions.
         if (!emit1(JSOP_POP))
             return false;
         break;
       default:
         MOZ_ASSERT(0);
     }
 
     if (op != JSOP_NOP) {
@@ -4988,22 +5009,19 @@ BytecodeEmitter::emitAssignment(ParseNod
                     return false;
                 elemOp = JSOP_GETELEM;
             }
             if (!emitElemOpBase(elemOp))
                 return false;
             break;
           }
           case PNK_CALL:
-            /*
-             * We just emitted a JSOP_SETCALL (which will always throw) and
-             * popped the call's return value. Push a random value to make sure
-             * the stack depth is correct.
-             */
-            MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL);
+            // We just emitted a JSOP_THROWMSG and popped the call's return
+            // value.  Push a random value to make sure the stack depth is
+            // correct.
             if (!emit1(JSOP_NULL))
                 return false;
             break;
           default:;
         }
     }
 
     if (!EmitAssignmentRhs(this, rhs, offset))
@@ -5023,18 +5041,17 @@ BytecodeEmitter::emitAssignment(ParseNod
         JSOp setOp = lhs->as<PropertyAccess>().isSuper() ?
                        (sc->strict() ? JSOP_STRICTSETPROP_SUPER : JSOP_SETPROP_SUPER) :
                        (sc->strict() ? JSOP_STRICTSETPROP : JSOP_SETPROP);
         if (!emitIndexOp(setOp, atomIndex))
             return false;
         break;
       }
       case PNK_CALL:
-        /* Do nothing. The JSOP_SETCALL we emitted will always throw. */
-        MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL);
+        // We threw above, so nothing to do here.
         break;
       case PNK_ELEM: {
         JSOp setOp = lhs->as<PropertyByValue>().isSuper() ?
                        sc->strict() ? JSOP_STRICTSETELEM_SUPER : JSOP_SETELEM_SUPER :
                        sc->strict() ? JSOP_STRICTSETELEM : JSOP_SETELEM;
         if (!emit1(setOp))
             return false;
         break;
@@ -7522,17 +7539,16 @@ BytecodeEmitter::emitDeleteExpression(Pa
 
     // If useless, just emit JSOP_TRUE; otherwise convert |delete <expr>| to
     // effectively |<expr>, true|.
     bool useful = false;
     if (!checkSideEffects(expression, &useful))
         return false;
 
     if (useful) {
-        MOZ_ASSERT_IF(expression->isKind(PNK_CALL), !(expression->pn_xflags & PNX_SETCALL));
         if (!emitTree(expression))
             return false;
         if (!emit1(JSOP_POP))
             return false;
     }
 
     return emit1(JSOP_TRUE);
 }
@@ -7782,19 +7798,16 @@ BytecodeEmitter::emitCallOrNew(ParseNode
         return false;
     }
 
     ParseNode* pn2 = pn->pn_head;
     bool spread = JOF_OPTYPE(pn->getOp()) == JOF_BYTE;
     switch (pn2->getKind()) {
       case PNK_NAME:
         if (emitterMode == BytecodeEmitter::SelfHosting && !spread) {
-            // We shouldn't see foo(bar) = x in self-hosted code.
-            MOZ_ASSERT(!(pn->pn_xflags & PNX_SETCALL));
-
             // Calls to "forceInterpreter", "callFunction",
             // "callContentFunction", or "resumeGenerator" in self-hosted
             // code generate inline bytecode.
             if (pn2->name() == cx->names().callFunction ||
                 pn2->name() == cx->names().callContentFunction ||
                 pn2->name() == cx->names().constructContentFunction)
             {
                 return emitSelfHostedCallFunction(pn);
@@ -7948,20 +7961,16 @@ BytecodeEmitter::emitCallOrNew(ParseNode
         pn->isOp(JSOP_STRICTEVAL) ||
         pn->isOp(JSOP_SPREADEVAL) ||
         pn->isOp(JSOP_STRICTSPREADEVAL))
     {
         uint32_t lineNum = parser->tokenStream.srcCoords.lineNum(pn->pn_pos.begin);
         if (!emitUint32Operand(JSOP_LINENO, lineNum))
             return false;
     }
-    if (pn->pn_xflags & PNX_SETCALL) {
-        if (!emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS))
-            return false;
-    }
 
     return true;
 }
 
 bool
 BytecodeEmitter::emitRightAssociative(ParseNode* pn)
 {
     // ** is the only right-associative operator.
@@ -8060,28 +8069,24 @@ BytecodeEmitter::emitSequenceExpr(ParseN
     return true;
 }
 
 // Using MOZ_NEVER_INLINE in here is a workaround for llvm.org/pr14047. See
 // the comment on emitSwitch.
 MOZ_NEVER_INLINE bool
 BytecodeEmitter::emitIncOrDec(ParseNode* pn)
 {
-    /* Emit lvalue-specialized code for ++/-- operators. */
-    ParseNode* pn2 = pn->pn_kid;
-    switch (pn2->getKind()) {
+    switch (pn->pn_kid->getKind()) {
       case PNK_DOT:
         return emitPropIncDec(pn);
       case PNK_ELEM:
         return emitElemIncDec(pn);
       case PNK_CALL:
-        MOZ_ASSERT(pn2->pn_xflags & PNX_SETCALL);
-        return emitTree(pn2);
+        return emitCallIncDec(pn);
       default:
-        MOZ_ASSERT(pn2->isKind(PNK_NAME));
         return emitNameIncDec(pn);
     }
 
     return true;
 }
 
 // Using MOZ_NEVER_INLINE in here is a workaround for llvm.org/pr14047. See
 // the comment on emitSwitch.
--- a/js/src/frontend/BytecodeEmitter.h
+++ b/js/src/frontend/BytecodeEmitter.h
@@ -496,16 +496,17 @@ struct MOZ_STACK_CLASS BytecodeEmitter
     MOZ_MUST_USE bool emitJumpNoFallthrough(JSOp op, JumpList* jump);
     MOZ_MUST_USE bool emitJump(JSOp op, JumpList* jump);
     MOZ_MUST_USE bool emitBackwardJump(JSOp op, JumpTarget target, JumpList* jump,
                                        JumpTarget* fallthrough);
     void patchJumpsToTarget(JumpList jump, JumpTarget target);
     MOZ_MUST_USE bool emitJumpTargetAndPatch(JumpList jump);
 
     MOZ_MUST_USE bool emitCall(JSOp op, uint16_t argc, ParseNode* pn = nullptr);
+    MOZ_MUST_USE bool emitCallIncDec(ParseNode* incDec);
 
     MOZ_MUST_USE bool emitLoopHead(ParseNode* nextpn, JumpTarget* top);
     MOZ_MUST_USE bool emitLoopEntry(ParseNode* nextpn, JumpList entryJump);
 
     MOZ_MUST_USE bool emitGoto(NestableControl* target, JumpList* jumplist,
                                SrcNoteType noteType = SRC_NULL);
 
     MOZ_MUST_USE bool emitIndex32(JSOp op, uint32_t index);
--- a/js/src/frontend/FullParseHandler.h
+++ b/js/src/frontend/FullParseHandler.h
@@ -191,20 +191,16 @@ class FullParseHandler
             return null();
         return new_<RegExpLiteral>(objbox, pos);
     }
 
     ParseNode* newConditional(ParseNode* cond, ParseNode* thenExpr, ParseNode* elseExpr) {
         return new_<ConditionalExpression>(cond, thenExpr, elseExpr);
     }
 
-    void markAsSetCall(ParseNode* pn) {
-        pn->pn_xflags |= PNX_SETCALL;
-    }
-
     ParseNode* newDelete(uint32_t begin, ParseNode* expr) {
         if (expr->isKind(PNK_NAME)) {
             expr->setOp(JSOP_DELNAME);
             return newUnary(PNK_DELETENAME, JSOP_NOP, begin, expr);
         }
 
         if (expr->isKind(PNK_DOT))
             return newUnary(PNK_DELETEPROP, JSOP_NOP, begin, expr);
--- a/js/src/frontend/ParseNode.h
+++ b/js/src/frontend/ParseNode.h
@@ -616,21 +616,20 @@ class ParseNode
 
     void setScopeBody(ParseNode* body) {
         MOZ_ASSERT(pn_arity == PN_SCOPE);
         pn_u.scope.body = body;
     }
 
 /* PN_LIST pn_xflags bits. */
 #define PNX_FUNCDEFS    0x01            /* contains top-level function statements */
-#define PNX_SETCALL     0x02            /* call expression in lvalue context */
-#define PNX_ARRAYHOLESPREAD 0x04        /* one or more of
+#define PNX_ARRAYHOLESPREAD 0x02        /* one or more of
                                            1. array initialiser has holes
                                            2. array initializer has spread node */
-#define PNX_NONCONST    0x08            /* initialiser has non-constants */
+#define PNX_NONCONST    0x04            /* initialiser has non-constants */
 
     bool functionIsHoisted() const {
         MOZ_ASSERT(pn_arity == PN_CODE && getKind() == PNK_FUNCTION);
         MOZ_ASSERT(isOp(JSOP_LAMBDA) ||        // lambda, genexpr
                    isOp(JSOP_LAMBDA_ARROW) ||  // arrow function
                    isOp(JSOP_DEFFUN) ||        // non-body-level function statement
                    isOp(JSOP_NOP) ||           // body-level function stmt in global code
                    isOp(JSOP_GETLOCAL) ||      // body-level function stmt in function code
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -3736,29 +3736,25 @@ Parser<ParseHandler>::PossibleError::tra
         other->errorNumber_   = errorNumber_;
         other->strict_        = strict_;
         other->state_         = state_;
     }
 }
 
 template <typename ParseHandler>
 bool
-Parser<ParseHandler>::makeSetCall(Node target, unsigned msg)
+Parser<ParseHandler>::checkAssignmentToCall(Node target, unsigned msg)
 {
     MOZ_ASSERT(handler.isFunctionCall(target));
 
     // Assignment to function calls is forbidden in ES6.  We're still somewhat
     // concerned about sites using this in dead code, so forbid it only in
     // strict mode code (or if the werror option has been set), and otherwise
     // warn.
-    if (!report(ParseStrictError, pc->sc()->strict(), target, msg))
-        return false;
-
-    handler.markAsSetCall(target);
-    return true;
+    return report(ParseStrictError, pc->sc()->strict(), target, msg);
 }
 
 template <>
 bool
 Parser<FullParseHandler>::checkDestructuringName(ParseNode* expr, Maybe<DeclarationKind> maybeDecl)
 {
     MOZ_ASSERT(!handler.isUnparenthesizedDestructuringPattern(expr));
 
@@ -5041,17 +5037,17 @@ Parser<ParseHandler>::validateForInOrOfL
         if (!reportIfArgumentsEvalTarget(target))
             return false;
 
         handler.adjustGetToSet(target);
         return true;
     }
 
     if (handler.isFunctionCall(target))
-        return makeSetCall(target, JSMSG_BAD_FOR_LEFTSIDE);
+        return checkAssignmentToCall(target, JSMSG_BAD_FOR_LEFTSIDE);
 
     report(ParseError, false, target, JSMSG_BAD_FOR_LEFTSIDE);
     return false;
 }
 
 template <class ParseHandler>
 bool
 Parser<ParseHandler>::forHeadStart(YieldHandling yieldHandling,
@@ -7158,17 +7154,17 @@ Parser<ParseHandler>::checkAndMarkAsAssi
         if (!reportIfArgumentsEvalTarget(target))
             return false;
 
         handler.adjustGetToSet(target);
         return true;
     }
 
     MOZ_ASSERT(handler.isFunctionCall(target));
-    return makeSetCall(target, JSMSG_BAD_LEFTSIDE_OF_ASS);
+    return checkAssignmentToCall(target, JSMSG_BAD_LEFTSIDE_OF_ASS);
 }
 
 class AutoClearInDestructuringDecl
 {
     ParseContext* pc_;
     Maybe<DeclarationKind> saved_;
 
   public:
@@ -7478,17 +7474,17 @@ Parser<ParseHandler>::checkAndMarkAsIncO
 
     // Mark.
     if (handler.isNameAnyParentheses(target)) {
         // Assignment to arguments/eval is allowed outside strict mode code,
         // but it's dodgy.  Report a strict warning (error, if werror was set).
         if (!reportIfArgumentsEvalTarget(target))
             return false;
     } else if (handler.isFunctionCall(target)) {
-        if (!makeSetCall(target, JSMSG_BAD_INCOP_OPERAND))
+        if (!checkAssignmentToCall(target, JSMSG_BAD_INCOP_OPERAND))
             return false;
     }
     return true;
 }
 
 template <typename ParseHandler>
 typename ParseHandler::Node
 Parser<ParseHandler>::unaryOpExpr(YieldHandling yieldHandling, ParseNodeKind kind, JSOp op,
--- a/js/src/frontend/Parser.h
+++ b/js/src/frontend/Parser.h
@@ -1314,17 +1314,17 @@ class Parser final : private JS::AutoGCR
     // Recursive methods for checking/name-analyzing subcomponents of a
     // destructuring pattern.  The array/object methods *must* be passed arrays
     // or objects.  The name method may be passed anything but will report an
     // error if not passed a name.
     bool checkDestructuringArray(Node arrayPattern, mozilla::Maybe<DeclarationKind> maybeDecl);
     bool checkDestructuringObject(Node objectPattern, mozilla::Maybe<DeclarationKind> maybeDecl);
     bool checkDestructuringName(Node expr, mozilla::Maybe<DeclarationKind> maybeDecl);
 
-    bool makeSetCall(Node node, unsigned errnum);
+    bool checkAssignmentToCall(Node node, unsigned errnum);
 
     Node newNumber(const Token& tok) {
         return handler.newNumber(tok.number(), tok.decimalPoint(), tok.pos);
     }
 
     static Node null() { return ParseHandler::null(); }
 
     bool reportBadReturn(Node pn, ParseReportKind kind, unsigned errnum, unsigned anonerrnum);
--- a/js/src/frontend/SyntaxParseHandler.h
+++ b/js/src/frontend/SyntaxParseHandler.h
@@ -214,20 +214,16 @@ class SyntaxParseHandler
 
     template <class Boxer>
     Node newRegExp(RegExpObject* reobj, const TokenPos& pos, Boxer& boxer) { return NodeGeneric; }
 
     Node newConditional(Node cond, Node thenExpr, Node elseExpr) { return NodeGeneric; }
 
     Node newElision() { return NodeGeneric; }
 
-    void markAsSetCall(Node node) {
-        MOZ_ASSERT(node == NodeFunctionCall);
-    }
-
     Node newDelete(uint32_t begin, Node expr) {
         return NodeGeneric;
     }
 
     Node newTypeof(uint32_t begin, Node kid) {
         return NodeGeneric;
     }
 
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/extensions/inc-dec-functioncall.js
@@ -0,0 +1,90 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 609756;
+var summary =
+  "Perform ToNumber on the result of the |fun()| in |fun()++| before " +
+  "throwing";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var hadSideEffect;
+
+function f()
+{
+  return { valueOf: function() { hadSideEffect = true; return 0; } };
+}
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() { f()++; }, ReferenceError);
+assertEq(hadSideEffect, true);
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() {
+  for (var i = 0; i < 20; i++)
+  {
+    if (i > 18)
+      f()++;
+  }
+}, ReferenceError);
+assertEq(hadSideEffect, true);
+
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() { f()--; }, ReferenceError);
+assertEq(hadSideEffect, true);
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() {
+  for (var i = 0; i < 20; i++)
+  {
+    if (i > 18)
+      f()--;
+  }
+}, ReferenceError);
+assertEq(hadSideEffect, true);
+
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() { ++f(); }, ReferenceError);
+assertEq(hadSideEffect, true);
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() {
+  for (var i = 0; i < 20; i++)
+  {
+    if (i > 18)
+      ++f();
+  }
+}, ReferenceError);
+assertEq(hadSideEffect, true);
+
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() { --f(); }, ReferenceError);
+assertEq(hadSideEffect, true);
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() {
+  for (var i = 0; i < 20; i++)
+  {
+    if (i > 18)
+      --f();
+  }
+}, ReferenceError);
+assertEq(hadSideEffect, true);
+
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");