From 27b238d9af21ebbe03b7ad619ee9cbac5a780123 Mon Sep 17 00:00:00 2001
From: Hendiadyoin1 <leon.a@serenityos.org>
Date: Thu, 11 Apr 2024 11:12:21 +0200
Subject: [PATCH] LibJS: Stop swallowing exceptions in finalizers

This also fixes one of the try-catch-finally tests, and adds a new one.
---
 .../Libraries/LibJS/Bytecode/Interpreter.cpp  |  5 ----
 .../LibJS/Tests/try-catch-finally-nested.js   | 15 +++++++++++
 .../LibJS/Tests/try-finally-break.js          | 26 +++++++++++--------
 3 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp
index 44b9b749753..f28b715cdda 100644
--- a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp
+++ b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp
@@ -410,11 +410,6 @@ void Interpreter::run_bytecode()
                 }
                 if (finalizer) {
                     m_current_block = finalizer;
-                    // If an exception was thrown inside the corresponding `catch` block, we need to rethrow it
-                    // from the `finally` block. But if the exception is from the `try` block, and has already been
-                    // handled by `catch`, we swallow it.
-                    if (!unwind_context.handler_called)
-                        reg(Register::exception()) = {};
                     goto start;
                 }
                 // An unwind context with no handler or finalizer? We have nowhere to jump, and continuing on will make us crash on the next `Call` to a non-native function if there's an exception! So let's crash here instead.
diff --git a/Userland/Libraries/LibJS/Tests/try-catch-finally-nested.js b/Userland/Libraries/LibJS/Tests/try-catch-finally-nested.js
index 7614b054838..1a7d8de157a 100644
--- a/Userland/Libraries/LibJS/Tests/try-catch-finally-nested.js
+++ b/Userland/Libraries/LibJS/Tests/try-catch-finally-nested.js
@@ -103,3 +103,18 @@ test("Deeply nested try/catch/finally with return in inner context", () => {
     })();
     expect(success).toBe(7);
 });
+
+test("Nested try/finally/catch with exception in inner context ", () => {
+    success = 0;
+    try {
+        try {
+            throw Error("Error in inner try");
+        } finally {
+            success += 1;
+        }
+        expect.fail();
+    } catch (e) {
+        success += 1;
+    }
+    expect(success).toBe(2);
+});
diff --git a/Userland/Libraries/LibJS/Tests/try-finally-break.js b/Userland/Libraries/LibJS/Tests/try-finally-break.js
index 0c34565682d..76c4cf72ed5 100644
--- a/Userland/Libraries/LibJS/Tests/try-finally-break.js
+++ b/Userland/Libraries/LibJS/Tests/try-finally-break.js
@@ -342,19 +342,23 @@ test("labelled break in finally overrides labelled break in try", () => {
 
 test("Throw while breaking", () => {
     const executionOrder = [];
-    try {
-        for (const i = 1337; ; expect().fail("Jumped to for loop update block")) {
-            try {
-                executionOrder.push(1);
-                break;
-            } finally {
-                executionOrder.push(2);
-                throw 1;
+    expect(() => {
+        try {
+            for (const i = 1337; ; expect().fail("Jumped to for loop update block")) {
+                try {
+                    executionOrder.push(1);
+                    break;
+                } finally {
+                    executionOrder.push(2);
+                    throw Error(1);
+                }
             }
+        } finally {
+            executionOrder.push(3);
         }
-    } finally {
-        executionOrder.push(3);
-    }
+        expect().fail("Running code after for loop");
+    }).toThrowWithMessage(Error, 1);
+
     expect(() => {
         i;
     }).toThrowWithMessage(ReferenceError, "'i' is not defined");