From 75dac35d0e66c173b3ceb5e8ecf44fe771b7cd87 Mon Sep 17 00:00:00 2001 From: Linus Groh Date: Fri, 11 Sep 2020 23:30:00 +0100 Subject: [PATCH] LibJS: Stop unwinding and reset exception for TryStatement finalizer This fixes two issues with running a TryStatement finalizer: - Temporarily store and clear the exception, if any, so we can run the finalizer block statement without it getting in our way, which could have unexpected side effects otherwise (and will likely return early somewhere). - Stop unwinding so more than one child node of the finalizer BlockStatement is executed if an exception has been thrown previously (which would have called unwind(ScopeType::Try)). Re-throwing as described above ensures we still unwind after the finalizer, if necessary. Also add some tests specifically for try/catch/finally blocks, we didn't have any! --- Libraries/LibJS/AST.cpp | 14 +- .../LibJS/Tests/try-catch-finally-nested.js | 55 ++++++ Libraries/LibJS/Tests/try-catch-finally.js | 163 ++++++++++++++++++ 3 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 Libraries/LibJS/Tests/try-catch-finally-nested.js create mode 100644 Libraries/LibJS/Tests/try-catch-finally.js diff --git a/Libraries/LibJS/AST.cpp b/Libraries/LibJS/AST.cpp index 02dcfa7006..19161b9749 100644 --- a/Libraries/LibJS/AST.cpp +++ b/Libraries/LibJS/AST.cpp @@ -1786,8 +1786,20 @@ Value TryStatement::execute(Interpreter& interpreter, GlobalObject& global_objec } } - if (m_finalizer) + if (m_finalizer) { + // Keep, if any, and then clear the current exception so we can + // execute() the finalizer without an exception in our way. + auto* previous_exception = interpreter.exception(); + interpreter.clear_exception(); + interpreter.stop_unwind(); m_finalizer->execute(interpreter, global_object); + // If we previously had an exception and the finalizer didn't + // throw a new one, restore the old one. + // FIXME: This will print debug output in throw_exception() for + // a seconds time with INTERPRETER_DEBUG enabled. + if (previous_exception && !interpreter.exception()) + interpreter.throw_exception(previous_exception); + } return js_undefined(); } diff --git a/Libraries/LibJS/Tests/try-catch-finally-nested.js b/Libraries/LibJS/Tests/try-catch-finally-nested.js new file mode 100644 index 0000000000..da9ad2d438 --- /dev/null +++ b/Libraries/LibJS/Tests/try-catch-finally-nested.js @@ -0,0 +1,55 @@ +test("Nested try/catch/finally with exceptions", () => { + // This test uses a combination of boolean "checkpoint" flags + // and expect().fail() to ensure certain code paths have been + // reached and others haven't. + var level1TryHasBeenExecuted = false; + var level1CatchHasBeenExecuted = false; + var level1FinallyHasBeenExecuted = false; + var level2TryHasBeenExecuted = false; + var level2CatchHasBeenExecuted = false; + var level3TryHasBeenExecuted = false; + var level3CatchHasBeenExecuted = false; + var level3FinallyHasBeenExecuted = false; + expect(() => { + try { + level1TryHasBeenExecuted = true; + foo(); + expect().fail(); + } catch (e) { + level1CatchHasBeenExecuted = true; + try { + level2TryHasBeenExecuted = true; + try { + level3TryHasBeenExecuted = true; + bar(); + expect().fail(); + } catch (e) { + level3CatchHasBeenExecuted = true; + } finally { + level3FinallyHasBeenExecuted = true; + baz(); + expect().fail(); + } + expect().fail(); + } catch (e) { + level2CatchHasBeenExecuted = true; + qux(); + expect().fail(); + } + expect().fail(); + } finally { + level1FinallyHasBeenExecuted = true; + throw Error("Error in final finally"); + expect().fail(); + } + expect().fail(); + }).toThrow(Error, "Error in final finally"); + expect(level1TryHasBeenExecuted).toBeTrue(); + expect(level1CatchHasBeenExecuted).toBeTrue(); + expect(level1FinallyHasBeenExecuted).toBeTrue(); + expect(level2TryHasBeenExecuted).toBeTrue(); + expect(level2CatchHasBeenExecuted).toBeTrue(); + expect(level3TryHasBeenExecuted).toBeTrue(); + expect(level3CatchHasBeenExecuted).toBeTrue(); + expect(level3FinallyHasBeenExecuted).toBeTrue(); +}); diff --git a/Libraries/LibJS/Tests/try-catch-finally.js b/Libraries/LibJS/Tests/try-catch-finally.js new file mode 100644 index 0000000000..7aca91c500 --- /dev/null +++ b/Libraries/LibJS/Tests/try-catch-finally.js @@ -0,0 +1,163 @@ +test("try/catch without exception", () => { + var tryHasBeenExecuted = false; + var catchHasBeenExecuted = false; + try { + tryHasBeenExecuted = true; + } catch (e) { + catchHasBeenExecuted = true; + } + expect(tryHasBeenExecuted).toBeTrue(); + expect(catchHasBeenExecuted).toBeFalse(); +}); + +test("try/catch with exception in try", () => { + var tryHasBeenExecuted = false; + var catchHasBeenExecuted = false; + var tryError = Error("Error in try"); + try { + tryHasBeenExecuted = true; + throw tryError; + expect().fail(); + } catch (e) { + catchHasBeenExecuted = true; + expect(e).toBe(tryError); + } + expect(tryHasBeenExecuted).toBeTrue(); + expect(catchHasBeenExecuted).toBeTrue(); +}); + +test("try/catch with exception in try and catch", () => { + var tryHasBeenExecuted = false; + var catchHasBeenExecuted = false; + var tryError = Error("Error in try"); + var catchError = Error("Error in catch"); + expect(() => { + try { + tryHasBeenExecuted = true; + throw tryError; + expect().fail(); + } catch (e) { + catchHasBeenExecuted = true; + expect(e).toBe(tryError); + throw catchError; + expect().fail(); + } + }).toThrow(Error, "Error in catch"); + expect(tryHasBeenExecuted).toBeTrue(); + expect(catchHasBeenExecuted).toBeTrue(); +}); + +test("try/catch/finally without exception", () => { + var tryHasBeenExecuted = false; + var catchHasBeenExecuted = false; + var finallyHasBeenExecuted = false; + try { + tryHasBeenExecuted = true; + } catch (e) { + catchHasBeenExecuted = true; + } finally { + finallyHasBeenExecuted = true; + } + expect(tryHasBeenExecuted).toBeTrue(); + expect(catchHasBeenExecuted).toBeFalse(); + expect(finallyHasBeenExecuted).toBeTrue(); +}); + +test("try/catch/finally with exception in try and catch", () => { + var tryHasBeenExecuted = false; + var catchHasBeenExecuted = false; + var finallyHasBeenExecuted = false; + var tryError = Error("Error in try"); + var catchError = Error("Error in catch"); + expect(() => { + try { + tryHasBeenExecuted = true; + throw tryError; + expect().fail(); + } catch (e) { + catchHasBeenExecuted = true; + expect(e).toBe(tryError); + throw catchError; + expect().fail(); + } finally { + finallyHasBeenExecuted = true; + } + }).toThrow(Error, "Error in catch"); + expect(tryHasBeenExecuted).toBeTrue(); + expect(catchHasBeenExecuted).toBeTrue(); + expect(finallyHasBeenExecuted).toBeTrue(); +}); + +test("try/catch/finally with exception in finally", () => { + var tryHasBeenExecuted = false; + var catchHasBeenExecuted = false; + var finallyHasBeenExecuted = false; + var finallyError = Error("Error in finally"); + expect(() => { + try { + tryHasBeenExecuted = true; + } catch (e) { + catchHasBeenExecuted = true; + } finally { + finallyHasBeenExecuted = true; + throw finallyError; + expect().fail(); + } + }).toThrow(Error, "Error in finally"); + expect(tryHasBeenExecuted).toBeTrue(); + expect(catchHasBeenExecuted).toBeFalse(); + expect(finallyHasBeenExecuted).toBeTrue(); +}); + +test("try/catch/finally with exception in try and finally", () => { + var tryHasBeenExecuted = false; + var catchHasBeenExecuted = false; + var finallyHasBeenExecuted = false; + var tryError = Error("Error in try"); + var finallyError = Error("Error in finally"); + expect(() => { + try { + tryHasBeenExecuted = true; + throw tryError; + expect().fail(); + } catch (e) { + catchHasBeenExecuted = true; + expect(e).toBe(tryError); + } finally { + finallyHasBeenExecuted = true; + throw finallyError; + expect().fail(); + } + }).toThrow(Error, "Error in finally"); + expect(tryHasBeenExecuted).toBeTrue(); + expect(catchHasBeenExecuted).toBeTrue(); + expect(finallyHasBeenExecuted).toBeTrue(); +}); + +test("try/catch/finally with exception in try, catch and finally", () => { + var tryHasBeenExecuted = false; + var catchHasBeenExecuted = false; + var finallyHasBeenExecuted = false; + var tryError = Error("Error in try"); + var catchError = Error("Error in catch"); + var finallyError = Error("Error in finally"); + expect(() => { + try { + tryHasBeenExecuted = true; + throw tryError; + expect().fail(); + } catch (e) { + catchHasBeenExecuted = true; + expect(e).toBe(tryError); + throw catchError; + expect().fail(); + } finally { + finallyHasBeenExecuted = true; + throw finallyError; + expect().fail(); + } + }).toThrow(Error, "Error in finally"); + expect(tryHasBeenExecuted).toBeTrue(); + expect(catchHasBeenExecuted).toBeTrue(); + expect(finallyHasBeenExecuted).toBeTrue(); +});