Bug 958949 - Save return value for generator function into local variable before running finally-block. r=jandem

js/src/asmjs/AsmJSValidate.cpp

@@ -85,13 +85,6 @@ UnaryKid(ParseNode *pn)
     return pn->pn_kid;
 }
 
-static inline ParseNode *
-ReturnExpr(ParseNode *pn)
-{
-    MOZ_ASSERT(pn->isKind(PNK_RETURN));
-    return UnaryKid(pn);
-}
-
 static inline ParseNode *
 BinaryRight(ParseNode *pn)
 {
@@ -106,6 +99,13 @@ BinaryLeft(ParseNode *pn)
     return pn->pn_left;
 }
 
+static inline ParseNode *
+ReturnExpr(ParseNode *pn)
+{
+    MOZ_ASSERT(pn->isKind(PNK_RETURN));
+    return BinaryLeft(pn);
+}
+
 static inline ParseNode *
 TernaryKid1(ParseNode *pn)
 {
@@ -4165,7 +4165,7 @@ static bool
 CheckFinalReturn(FunctionCompiler &f, ParseNode *stmt, RetType *retType)
 {
     if (stmt && stmt->isKind(PNK_RETURN)) {
-        if (ParseNode *coercionNode = UnaryKid(stmt)) {
+        if (ParseNode *coercionNode = BinaryLeft(stmt)) {
             AsmJSNumLit lit;
             if (IsLiteralOrConst(f, coercionNode, &lit)) {
                 switch (lit.which()) {

js/src/frontend/BytecodeEmitter.cpp

@@ -1074,9 +1074,9 @@ EmitAtomOp(ExclusiveContext *cx, JSAtom *atom, JSOp op, BytecodeEmitter *bce)
 {
     MOZ_ASSERT(JOF_OPTYPE(op) == JOF_ATOM);
 
-    // .generator lookups should be emitted as JSOP_GETALIASEDVAR instead of
-    // JSOP_GETNAME etc, to bypass |with| objects on the scope chain.
-    MOZ_ASSERT_IF(op == JSOP_GETNAME || op == JSOP_GETGNAME, atom != cx->names().dotGenerator);
+    // .generator and .genrval lookups should be emitted as JSOP_GETALIASEDVAR
+    // instead of JSOP_GETNAME etc, to bypass |with| objects on the scope chain.
+    MOZ_ASSERT_IF(op == JSOP_GETNAME || op == JSOP_GETGNAME, !bce->sc->isDotVariable(atom));
 
     if (op == JSOP_GETPROP && atom == cx->names().length) {
         /* Specialize length accesses for the interpreter. */
@@ -5588,6 +5588,16 @@ EmitContinue(ExclusiveContext *cx, BytecodeEmitter *bce, PropertyName *label)
     return EmitGoto(cx, bce, stmt, &stmt->continues, SRC_CONTINUE) >= 0;
 }
 
+static bool
+InTryBlockWithFinally(BytecodeEmitter *bce)
+{
+    for (StmtInfoBCE *stmt = bce->topStmt; stmt; stmt = stmt->down) {
+        if (stmt->type == STMT_FINALLY)
+            return true;
+    }
+    return false;
+}
+
 static bool
 EmitReturn(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn)
 {
@@ -5600,7 +5610,7 @@ EmitReturn(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn)
     }
 
     /* Push a return value */
-    if (ParseNode *pn2 = pn->pn_kid) {
+    if (ParseNode *pn2 = pn->pn_left) {
         if (!EmitTree(cx, bce, pn2))
             return false;
     } else {
@@ -5628,8 +5638,25 @@ EmitReturn(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn)
     ptrdiff_t top = bce->offset();
 
     bool isGenerator = bce->sc->isFunctionBox() && bce->sc->asFunctionBox()->isGenerator();
-    if (Emit1(cx, bce, isGenerator ? JSOP_SETRVAL : JSOP_RETURN) < 0)
-         return false;
+    bool useGenRVal = false;
+    if (isGenerator) {
+        if (bce->sc->asFunctionBox()->isStarGenerator() && InTryBlockWithFinally(bce)) {
+            // Emit JSOP_SETALIASEDVAR .genrval to store the return value on the
+            // scope chain, so it's not lost when we yield in a finally block.
+            useGenRVal = true;
+            MOZ_ASSERT(pn->pn_right);
+            if (!EmitTree(cx, bce, pn->pn_right))
+                return false;
+            if (Emit1(cx, bce, JSOP_POP) < 0)
+                return false;
+        } else {
+            if (Emit1(cx, bce, JSOP_SETRVAL) < 0)
+                return false;
+        }
+    } else {
+        if (Emit1(cx, bce, JSOP_RETURN) < 0)
+            return false;
+    }
 
     NonLocalExitScope nle(cx, bce);
 
@@ -5638,9 +5665,17 @@ EmitReturn(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn)
 
     if (isGenerator) {
         ScopeCoordinate sc;
-        // We know that .generator is on the top scope chain node, as we just
-        // exited nested scopes.
+        // We know that .generator and .genrval are on the top scope chain node,
+        // as we just exited nested scopes.
         sc.setHops(0);
+        if (useGenRVal) {
+            MOZ_ALWAYS_TRUE(LookupAliasedNameSlot(bce, bce->script, cx->names().dotGenRVal, &sc));
+            if (!EmitAliasedVarOp(cx, JSOP_GETALIASEDVAR, sc, DontCheckLexical, bce))
+                return false;
+            if (Emit1(cx, bce, JSOP_SETRVAL) < 0)
+                return false;
+        }
+
         MOZ_ALWAYS_TRUE(LookupAliasedNameSlot(bce, bce->script, cx->names().dotGenerator, &sc));
         if (!EmitAliasedVarOp(cx, JSOP_GETALIASEDVAR, sc, DontCheckLexical, bce))
             return false;

js/src/frontend/FoldConstants.cpp

@@ -363,8 +363,11 @@ Fold(ExclusiveContext *cx, ParseNode **pnp,
                 if (!Fold(cx, &pn->pn_left, handler, options, inGenexpLambda, condIf(pn, PNK_WHILE)))
                     return false;
             }
-            if (!Fold(cx, &pn->pn_right, handler, options, inGenexpLambda, condIf(pn, PNK_DOWHILE)))
-                return false;
+            /* Second kid may be null (for return in non-generator). */
+            if (pn->pn_right) {
+                if (!Fold(cx, &pn->pn_right, handler, options, inGenexpLambda, condIf(pn, PNK_DOWHILE)))
+                    return false;
+            }
         }
         pn1 = pn->pn_left;
         pn2 = pn->pn_right;

js/src/frontend/FullParseHandler.h

@@ -478,9 +478,9 @@ class FullParseHandler
         return new_<BreakStatement>(label, pos);
     }
 
-    ParseNode *newReturnStatement(ParseNode *expr, const TokenPos &pos) {
+    ParseNode *newReturnStatement(ParseNode *expr, ParseNode *genrval, const TokenPos &pos) {
         MOZ_ASSERT_IF(expr, pos.encloses(expr->pn_pos));
-        return new_<UnaryNode>(PNK_RETURN, JSOP_RETURN, pos, expr);
+        return new_<BinaryNode>(PNK_RETURN, JSOP_RETURN, pos, expr, genrval);
     }
 
     ParseNode *newWithStatement(uint32_t begin, ParseNode *expr, ParseNode *body,

js/src/frontend/ParseNode.h

@@ -311,7 +311,8 @@ enum ParseNodeKind
  *                                     pn_left: PNK_NAME with pn_used true and
  *                                              pn_lexdef (NOT pn_expr) set
  *                                     pn_right: initializer
- * PNK_RETURN   unary       pn_kid: return expr or null
+ * PNK_RETURN   binary      pn_left: return expr or null
+ *                          pn_right: .genrval name or null
  * PNK_SEMI     unary       pn_kid: expr or null statement
  *                          pn_prologue: true if Directive Prologue member
  *                              in original source, not introduced via

js/src/frontend/Parser.cpp

@@ -1047,7 +1047,7 @@ Parser<ParseHandler>::functionBody(FunctionSyntaxKind kind, FunctionBodyType typ
         if (!kid)
             return null();
 
-        pn = handler.newReturnStatement(kid, handler.getPosition(kid));
+        pn = handler.newReturnStatement(kid, null(), handler.getPosition(kid));
         if (!pn)
             return null();
     }
@@ -1087,6 +1087,14 @@ Parser<ParseHandler>::functionBody(FunctionSyntaxKind kind, FunctionBodyType typ
         if (!pc->define(tokenStream, context->names().dotGenerator, generator, Definition::VAR))
             return null();
 
+        if (pc->isStarGenerator()) {
+            Node genrval = newName(context->names().dotGenRVal);
+            if (!genrval)
+                return null();
+            if (!pc->define(tokenStream, context->names().dotGenRVal, genrval, Definition::VAR))
+                return null();
+        }
+
         generator = newName(context->names().dotGenerator);
         if (!generator)
             return null();
@@ -3111,7 +3119,7 @@ LexicalLookup(ContextT *ct, HandleAtom atom, int *slotp, typename ContextT::Stmt
          * can potentially override any static bindings introduced by statements
          * further up the stack, we have to abort the search.
          */
-        if (stmt->type == STMT_WITH && atom != ct->sc->context->names().dotGenerator)
+        if (stmt->type == STMT_WITH && !ct->sc->isDotVariable(atom))
             break;
 
         // Skip statements that do not introduce a new scope
@@ -5288,7 +5296,18 @@ Parser<ParseHandler>::returnStatement()
     if (!MatchOrInsertSemicolon(tokenStream))
         return null();
 
-    Node pn = handler.newReturnStatement(exprNode, TokenPos(begin, pos().end));
+    Node genrval = null();
+    if (pc->isStarGenerator()) {
+        genrval = newName(context->names().dotGenRVal);
+        if (!genrval)
+            return null();
+        if (!noteNameUse(context->names().dotGenRVal, genrval))
+            return null();
+        if (!checkAndMarkAsAssignmentLhs(genrval, PlainAssignment))
+            return null();
+    }
+
+    Node pn = handler.newReturnStatement(exprNode, genrval, TokenPos(begin, pos().end));
     if (!pn)
         return null();
 
@@ -5486,7 +5505,7 @@ Parser<FullParseHandler>::withStatement()
     for (AtomDefnRange r = pc->lexdeps->all(); !r.empty(); r.popFront()) {
         DefinitionNode defn = r.front().value().get<FullParseHandler>();
         DefinitionNode lexdep = handler.resolve(defn);
-        if (lexdep->name() != context->names().dotGenerator)
+        if (!pc->sc->isDotVariable(lexdep->name()))
             handler.deoptimizeUsesWithin(lexdep, TokenPos(begin, pos().begin));
     }
 
@@ -6556,10 +6575,9 @@ LegacyCompExprTransplanter::transplant(ParseNode *pn)
             MOZ_ASSERT(!stmt || stmt != pc->topStmt);
 #endif
             if (isGenexp && !dn->isOp(JSOP_CALLEE)) {
-                MOZ_ASSERT_IF(atom != parser->context->names().dotGenerator,
-                              !pc->decls().lookupFirst(atom));
+                MOZ_ASSERT_IF(!pc->sc->isDotVariable(atom), !pc->decls().lookupFirst(atom));
 
-                if (atom == parser->context->names().dotGenerator) {
+                if (pc->sc->isDotVariable(atom)) {
                     if (dn->dn_uses == pn) {
                         if (!BumpStaticLevel(parser->tokenStream, dn, pc))
                             return false;

js/src/frontend/SharedContext.h

@@ -206,6 +206,10 @@ class SharedContext
     bool needStrictChecks() {
         return strict || extraWarnings;
     }
+
+    bool isDotVariable(JSAtom *atom) const {
+        return atom == context->names().dotGenerator || atom == context->names().dotGenRVal;
+    }
 };
 
 class GlobalSharedContext : public SharedContext

js/src/frontend/SyntaxParseHandler.h

@@ -156,7 +156,7 @@ class SyntaxParseHandler
     Node newCaseOrDefault(uint32_t begin, Node expr, Node body) { return NodeGeneric; }
     Node newContinueStatement(PropertyName *label, const TokenPos &pos) { return NodeGeneric; }
     Node newBreakStatement(PropertyName *label, const TokenPos &pos) { return NodeGeneric; }
-    Node newReturnStatement(Node expr, const TokenPos &pos) { return NodeGeneric; }
+    Node newReturnStatement(Node expr, Node genrval, const TokenPos &pos) { return NodeGeneric; }
 
     Node newLabeledStatement(PropertyName *label, Node stmt, uint32_t begin) {
         return NodeGeneric;

js/src/jit-test/tests/generators/yield-in-finally.js

@@ -0,0 +1,178 @@
+// return value in try block should not be overridden by yield in finally block.
+
+load(libdir + "asserts.js");
+
+// simple
+function* g1() {
+  try {
+    return 42;
+  } finally {
+    yield 43;
+  }
+}
+var o = g1();
+var v = o.next();
+assertEq(v.done, false);
+assertEq(v.value, 43);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, 42);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, undefined);
+
+// without return value
+function* g2() {
+  try {
+    return;
+  } finally {
+    yield 43;
+  }
+}
+o = g2();
+v = o.next();
+assertEq(v.done, false);
+assertEq(v.value, 43);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, undefined);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, undefined);
+
+// nested try-finally
+function* g3() {
+  try {
+    try {
+      return 42;
+    } finally {
+      try {
+        return 43;
+      } finally {
+        yield 44;
+      }
+    }
+  } finally {
+    yield 45;
+  }
+}
+o = g3();
+v = o.next();
+assertEq(v.done, false);
+assertEq(v.value, 44);
+v = o.next();
+assertEq(v.done, false);
+assertEq(v.value, 45);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, 43);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, undefined);
+
+// yield*
+function* g4() {
+  try {
+    return 42;
+  } finally {
+    try {
+      return 43;
+    } finally {
+      yield* g5();
+    }
+  }
+}
+function* g5() {
+  yield 44;
+  return 45;
+}
+o = g4();
+v = o.next();
+assertEq(v.done, false);
+assertEq(v.value, 44);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, 43);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, undefined);
+
+// return in block scope
+function* g6() {
+  let a = 10;
+  {
+    let a = 20;
+    try {
+      let a = 30;
+      {
+        let a = 40;
+        return 42;
+      }
+    } finally {
+      yield 43;
+    }
+  }
+}
+o = g6();
+v = o.next();
+assertEq(v.done, false);
+assertEq(v.value, 43);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, 42);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, undefined);
+
+// no finally
+function* g7() {
+  try {
+    return 42;
+  } catch (e) {
+    yield 1;
+  }
+}
+o = g7();
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, 42);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, undefined);
+
+// legacy iterator have no return value
+function g8() {
+  try {
+    return;
+  } finally {
+    yield 43;
+  }
+}
+o = g8();
+v = o.next();
+assertEq(v, 43);
+assertThrowsInstanceOf(() => o.next(), StopIteration);
+
+// in "with" statement
+options("strict");
+eval(`
+function* g9() {
+  with ({ ".genrval": { value: 44, done: false } }) {
+    try {
+      return 42;
+    } finally {
+      yield 43;
+    }
+  }
+}
+o = g9();
+v = o.next();
+assertEq(v.done, false);
+assertEq(v.value, 43);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, 42);
+v = o.next();
+assertEq(v.done, true);
+assertEq(v.value, undefined);
+`);

js/src/jsreflect.cpp

@@ -2522,16 +2522,23 @@ ASTSerializer::statement(ParseNode *pn, MutableHandleValue dst)
       }
 
       case PNK_THROW:
-      case PNK_RETURN:
       {
         MOZ_ASSERT_IF(pn->pn_kid, pn->pn_pos.encloses(pn->pn_kid->pn_pos));
 
         RootedValue arg(cx);
 
         return optExpression(pn->pn_kid, &arg) &&
-               (pn->isKind(PNK_THROW)
-                ? builder.throwStatement(arg, &pn->pn_pos, dst)
-                : builder.returnStatement(arg, &pn->pn_pos, dst));
+               builder.throwStatement(arg, &pn->pn_pos, dst);
+      }
+
+      case PNK_RETURN:
+      {
+        MOZ_ASSERT_IF(pn->pn_left, pn->pn_pos.encloses(pn->pn_left->pn_pos));
+
+        RootedValue arg(cx);
+
+        return optExpression(pn->pn_left, &arg) &&
+               builder.returnStatement(arg, &pn->pn_pos, dst);
       }
 
       case PNK_DEBUGGER:
@@ -3295,7 +3302,7 @@ ASTSerializer::functionArgsAndBody(ParseNode *pn, NodeVector &args, NodeVector &
     switch (pnbody->getKind()) {
       case PNK_RETURN: /* expression closure, no destructured args */
         return functionArgs(pn, pnargs, nullptr, pnbody, args, defaults, rest) &&
-               expression(pnbody->pn_kid, body);
+               expression(pnbody->pn_left, body);
 
       case PNK_SEQ:    /* expression closure with destructured args */
       {
@@ -3303,7 +3310,7 @@ ASTSerializer::functionArgsAndBody(ParseNode *pn, NodeVector &args, NodeVector &
         LOCAL_ASSERT(pnstart && pnstart->isKind(PNK_RETURN));
 
         return functionArgs(pn, pnargs, pndestruct, pnbody, args, defaults, rest) &&
-               expression(pnstart->pn_kid, body);
+               expression(pnstart->pn_left, body);
       }
 
       case PNK_STATEMENTLIST:     /* statement closure */

js/src/vm/CommonPropertyNames.h

@@ -61,6 +61,7 @@
     macro(displayURL, displayURL, "displayURL") \
     macro(done, done, "done") \
     macro(dotGenerator, dotGenerator, ".generator") \
+    macro(dotGenRVal, dotGenRVal, ".genrval") \
     macro(each, each, "each") \
     macro(elementType, elementType, "elementType") \
     macro(empty, empty, "") \

js/src/vm/Xdr.h

@@ -35,7 +35,7 @@ namespace js {
  * Nightly) and without (all others).  FIXME: Bug 1066322 - Enable ES6 symbols
  * in all builds.
  */
-static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 222;
+static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 224;
 static_assert(XDR_BYTECODE_VERSION_SUBTRAHEND % 2 == 0, "see the comment above");
 static const uint32_t XDR_BYTECODE_VERSION =
     uint32_t(0xb973c0de - (XDR_BYTECODE_VERSION_SUBTRAHEND