From 7cbede4342cb0b9ea085e9b213014fb43582443d Mon Sep 17 00:00:00 2001
From: Linus Groh <mail@linusgroh.de>
Date: Tue, 13 Apr 2021 00:57:17 +0200
Subject: LibJS: Fix return value of TryStatement with finalizer

Previously we would always return the result of executing the finalizer,
however the spec dictates the finalizer result must only be returned for
a non-normal completion.
I added some more comments along the way, which should make it more
clear what's going on - the unwinding and exception flow isn't super
straightforward here.
---
 Userland/Libraries/LibJS/AST.cpp                   | 29 ++++++++++++++++------
 .../LibJS/Tests/try-catch-finally-return.js        | 12 +++++++++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp
index 98204761f0..721a24b047 100644
--- a/Userland/Libraries/LibJS/AST.cpp
+++ b/Userland/Libraries/LibJS/AST.cpp
@@ -1988,14 +1988,29 @@ Value TryStatement::execute(Interpreter& interpreter, GlobalObject& global_objec
         // execute() the finalizer without an exception in our way.
         auto* previous_exception = interpreter.exception();
         interpreter.vm().clear_exception();
+
+        // Remember what scope type we were unwinding to, and temporarily
+        // clear it as well (e.g. return from handler).
+        auto unwind_until = interpreter.vm().unwind_until();
         interpreter.vm().stop_unwind();
-        result = 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 second time with m_should_log_exceptions enabled.
-        if (previous_exception && !interpreter.exception())
-            interpreter.vm().throw_exception(previous_exception);
+
+        auto finalizer_result = m_finalizer->execute(interpreter, global_object);
+        if (interpreter.vm().should_unwind()) {
+            // This was NOT a 'normal' completion (e.g. return from finalizer).
+            result = finalizer_result;
+        } else {
+            // Continue unwinding to whatever we found ourselves unwinding
+            // to when the finalizer was entered (e.g. return from handler,
+            // which is unaffected by normal completion from finalizer).
+            interpreter.vm().unwind(unwind_until);
+
+            // 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 second time with m_should_log_exceptions enabled.
+            if (previous_exception && !interpreter.exception())
+                interpreter.vm().throw_exception(previous_exception);
+        }
     }
 
     return result;
diff --git a/Userland/Libraries/LibJS/Tests/try-catch-finally-return.js b/Userland/Libraries/LibJS/Tests/try-catch-finally-return.js
index b64ea1120e..e9d5e6c037 100644
--- a/Userland/Libraries/LibJS/Tests/try-catch-finally-return.js
+++ b/Userland/Libraries/LibJS/Tests/try-catch-finally-return.js
@@ -32,3 +32,15 @@ test("return from finally block", () => {
     }
     expect(foo()).toBe("baz");
 });
+
+test("return from catch block with empty finally", () => {
+    function foo() {
+        try {
+            throw "foo";
+        } catch {
+            return "bar";
+        } finally {
+        }
+    }
+    expect(foo()).toBe("bar");
+});
-- 
cgit v1.2.3