LibJS: Stop swallowing exceptions in finalizers

This also fixes one of the try-catch-finally tests, and adds a new one.
sakgoyal · May 2, 2024 · 27b238d · 27b238d
1 parent b4b9c4b
commit 27b238d
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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");