From d371e1429d5cb876e73b1fe1af48e8d1e72f8b95 Mon Sep 17 00:00:00 2001 From: Jan de Mooij Date: Thu, 19 Feb 2015 13:19:03 +0100 Subject: [PATCH] Bug 718531 - Fix functions with try-finally to not return wrong value in some cases. r=shu --- js/src/frontend/BytecodeEmitter.cpp | 64 ++++++++++++------- js/src/frontend/BytecodeEmitter.h | 2 + .../tests/basic/finally-implicit-return.js | 45 +++++++++++++ 3 files changed, 87 insertions(+), 24 deletions(-) create mode 100644 js/src/jit-test/tests/basic/finally-implicit-return.js diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp index cc12efb927f3..a8fb4c036538 100644 --- a/js/src/frontend/BytecodeEmitter.cpp +++ b/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() && bce->sc->asFunctionBox()->isGenerator()) { - if (bce->sc->asFunctionBox()->isStarGenerator() && !EmitPrepareIteratorResult(cx, bce)) - return false; + if (bce->sc->isFunctionBox()) { + if (bce->sc->asFunctionBox()->isGenerator()) { + // If we fall off the end of a generator, do a final yield. + if (bce->sc->asFunctionBox()->isStarGenerator() && !EmitPrepareIteratorResult(cx, bce)) + return false; - if (Emit1(cx, bce, JSOP_UNDEFINED) < 0) - return false; + if (Emit1(cx, bce, JSOP_UNDEFINED) < 0) + return false; - if (bce->sc->asFunctionBox()->isStarGenerator() && !EmitFinishIteratorResult(cx, bce, true)) - return false; + if (bce->sc->asFunctionBox()->isStarGenerator() && + !EmitFinishIteratorResult(cx, bce, true)) + { + return false; + } - if (Emit1(cx, bce, JSOP_SETRVAL) < 0) - return false; + if (Emit1(cx, bce, JSOP_SETRVAL) < 0) + return false; - ScopeCoordinate sc; - // We know that .generator is on the top scope chain node, as we are - // at the function end. - sc.setHops(0); - MOZ_ALWAYS_TRUE(LookupAliasedNameSlot(bce, bce->script, cx->names().dotGenerator, &sc)); - if (!EmitAliasedVarOp(cx, JSOP_GETALIASEDVAR, sc, DontCheckLexical, bce)) - return false; + ScopeCoordinate sc; + // We know that .generator is on the top scope chain node, as we are + // at the function end. + sc.setHops(0); + MOZ_ALWAYS_TRUE(LookupAliasedNameSlot(bce, bce->script, cx->names().dotGenerator, &sc)); + if (!EmitAliasedVarOp(cx, JSOP_GETALIASEDVAR, sc, DontCheckLexical, bce)) + return false; - // No need to check for finally blocks, etc as in EmitReturn. - if (!EmitYieldOp(cx, bce, JSOP_FINALYIELDRVAL)) - return false; + // No need to check for finally blocks, etc as in EmitReturn. + if (!EmitYieldOp(cx, bce, JSOP_FINALYIELDRVAL)) + 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 - * depend on this opcode, e.g. js_InternalInterpret. - */ + // Always end the script with a JSOP_RETRVAL. Some other parts of the codebase + // depend on this opcode, e.g. InterpreterRegs::setToEndOfScript. 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)) diff --git a/js/src/frontend/BytecodeEmitter.h b/js/src/frontend/BytecodeEmitter.h index e7a129ad1dc4..cc64fa2aeb37 100644 --- a/js/src/frontend/BytecodeEmitter.h +++ b/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 diff --git a/js/src/jit-test/tests/basic/finally-implicit-return.js b/js/src/jit-test/tests/basic/finally-implicit-return.js new file mode 100644 index 000000000000..3bbd78d3804e --- /dev/null +++ b/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);