Bug 718531 - Fix functions with try-finally to not return wrong value in some cases. r=shu

js/src/frontend/BytecodeEmitter.cpp

@@ -143,6 +143,7 @@ BytecodeEmitter::BytecodeEmitter(BytecodeEmitter *parent,
     yieldOffsetList(sc->context),
     typesetCount(0),
     hasSingletons(false),
+    hasTryFinally(false),
     emittingForInit(false),
     emittingRunOnceLambda(false),
     insideEval(insideEval),
@@ -3120,37 +3121,51 @@ frontend::EmitFunctionScript(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNo
     if (!EmitTree(cx, bce, body))
         return false;
 
-    // If we fall off the end of a generator, do a final yield.
+    if (bce->sc->isFunctionBox()) {
-    if (bce->sc->isFunctionBox() && bce->sc->asFunctionBox()->isGenerator()) {
+        if (bce->sc->asFunctionBox()->isGenerator()) {
-        if (bce->sc->asFunctionBox()->isStarGenerator() && !EmitPrepareIteratorResult(cx, bce))
+            // If we fall off the end of a generator, do a final yield.
-            return false;
+            if (bce->sc->asFunctionBox()->isStarGenerator() && !EmitPrepareIteratorResult(cx, bce))
+                return false;
 
-        if (Emit1(cx, bce, JSOP_UNDEFINED) < 0)
+            if (Emit1(cx, bce, JSOP_UNDEFINED) < 0)
-            return false;
+                return false;
 
-        if (bce->sc->asFunctionBox()->isStarGenerator() && !EmitFinishIteratorResult(cx, bce, true))
+            if (bce->sc->asFunctionBox()->isStarGenerator() &&
-            return false;
+                !EmitFinishIteratorResult(cx, bce, true))
+            {
+                return false;
+            }
 
-        if (Emit1(cx, bce, JSOP_SETRVAL) < 0)
+            if (Emit1(cx, bce, JSOP_SETRVAL) < 0)
-            return false;
+                return false;
 
-        ScopeCoordinate sc;
+            ScopeCoordinate sc;
-        // We know that .generator is on the top scope chain node, as we are
+            // We know that .generator is on the top scope chain node, as we are
-        // at the function end.
+            // at the function end.
-        sc.setHops(0);
+            sc.setHops(0);
-        MOZ_ALWAYS_TRUE(LookupAliasedNameSlot(bce, bce->script, cx->names().dotGenerator, &sc));
+            MOZ_ALWAYS_TRUE(LookupAliasedNameSlot(bce, bce->script, cx->names().dotGenerator, &sc));
-        if (!EmitAliasedVarOp(cx, JSOP_GETALIASEDVAR, sc, DontCheckLexical, bce))
+            if (!EmitAliasedVarOp(cx, JSOP_GETALIASEDVAR, sc, DontCheckLexical, bce))
-            return false;
+                return false;
 
-        // No need to check for finally blocks, etc as in EmitReturn.
+            // No need to check for finally blocks, etc as in EmitReturn.
-        if (!EmitYieldOp(cx, bce, JSOP_FINALYIELDRVAL))
+            if (!EmitYieldOp(cx, bce, JSOP_FINALYIELDRVAL))
-            return false;
+                return false;
+        } else {
+            // Non-generator functions just return |undefined|. The JSOP_RETRVAL
+            // emitted below will do that, except if the script has a finally
+            // block: there can be a non-undefined value in the return value
+            // slot. We just emit an explicit return in this case.
+            if (bce->hasTryFinally) {
+                if (Emit1(cx, bce, JSOP_UNDEFINED) < 0)
+                    return false;
+                if (Emit1(cx, bce, JSOP_RETURN) < 0)
+                    return false;
+            }
+        }
     }
 
-    /*
+    // Always end the script with a JSOP_RETRVAL. Some other parts of the codebase
-     * Always end the script with a JSOP_RETRVAL. Some other parts of the codebase
+    // depend on this opcode, e.g. InterpreterRegs::setToEndOfScript.
-     * depend on this opcode, e.g. js_InternalInterpret.
-     */
     if (Emit1(cx, bce, JSOP_RETRVAL) < 0)
         return false;
 
@@ -4658,6 +4673,7 @@ EmitTry(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn)
         {
             return false;
         }
+        bce->hasTryFinally = true;
         MOZ_ASSERT(bce->stackDepth == depth);
     }
     if (!PopStatementBCE(cx, bce))

js/src/frontend/BytecodeEmitter.h

@@ -162,6 +162,8 @@ struct BytecodeEmitter
 
     bool            hasSingletons:1;    /* script contains singleton initializer JSOP_OBJECT */
 
+    bool            hasTryFinally:1;    /* script contains finally block */
+
     bool            emittingForInit:1;  /* true while emitting init expr of for; exclude 'in' */
 
     bool            emittingRunOnceLambda:1; /* true while emitting a lambda which is only

js/src/jit-test/tests/basic/finally-implicit-return.js

@@ -0,0 +1,45 @@
+// Implicit return at the end of a function should return |undefined|,
+// even if we initially set another return value and then executed a finally
+// block.
+
+function test1() {
+    try {
+        try {
+            return 3;
+        } finally {
+            throw 4;
+        }
+    } catch (e) {}
+}
+assertEq(test1(), undefined);
+
+function test2() {
+    L: try {
+        return 3;
+    } finally {
+        break L;
+    }
+}
+assertEq(test2(), undefined);
+
+function test3() {
+    for (var i=0; i<2; i++) {
+        try {
+            return 5;
+        } finally {
+            continue;
+        }
+    }
+}
+assertEq(test3(), undefined);
+
+// "return;" should work the same way.
+function test4() {
+    L: try {
+        return 6;
+    } finally {
+        break L;
+    }
+    return;
+}
+assertEq(test4(), undefined);