From 9f06e130a277ef47327807830cdb631b2753b62c Mon Sep 17 00:00:00 2001
From: Andreas Kling <kling@serenityos.org>
Date: Fri, 21 Jul 2023 17:30:07 +0200
Subject: [PATCH] LibJS/Bytecode: Keep saved return value in call frame
 register

This fixes an issue where returning inside a `try` block and then
calling a function inside `finally` would clobber the saved return
value from the `try` block.

Note that we didn't need to change the base of register allocation,
since it was already 1 too high.

With this fixed, https://microsoft.com/edge loads in bytecode mode. :^)

Thanks to Luke for reducing the issue!
---
 .../Libraries/LibJS/Bytecode/Interpreter.cpp     | 16 +++++++---------
 Userland/Libraries/LibJS/Bytecode/Interpreter.h  |  2 +-
 Userland/Libraries/LibJS/Bytecode/Register.h     |  7 +++++++
 .../Libraries/LibJS/Tests/try-return-finally.js  | 11 +++++++++++
 4 files changed, 26 insertions(+), 10 deletions(-)
 create mode 100644 Userland/Libraries/LibJS/Tests/try-return-finally.js

diff --git a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp
index 9efb741ca7..c818d9c527 100644
--- a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp
+++ b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp
@@ -46,8 +46,6 @@ void Interpreter::visit_edges(Cell::Visitor& visitor)
 {
     if (m_return_value.has_value())
         visitor.visit(*m_return_value);
-    if (m_saved_return_value.has_value())
-        visitor.visit(*m_saved_return_value);
     if (m_saved_exception.has_value())
         visitor.visit(*m_saved_exception);
     for (auto& frame : m_call_frames) {
@@ -277,8 +275,7 @@ Interpreter::ValueAndFrame Interpreter::run_and_return_frame(Realm& realm, Execu
         if (!unwind_contexts().is_empty() && !will_yield) {
             auto& unwind_context = unwind_contexts().last();
             if (unwind_context.executable == m_current_executable && unwind_context.finalizer) {
-                m_saved_return_value = m_return_value;
-                m_return_value = {};
+                reg(Register::saved_return_value()) = m_return_value.release_value();
                 m_current_block = unwind_context.finalizer;
                 // the unwind_context will be pop'ed when entering the finally block
                 continue;
@@ -308,13 +305,15 @@ Interpreter::ValueAndFrame Interpreter::run_and_return_frame(Realm& realm, Execu
         }
     }
 
+    auto saved_return_value = reg(Register::saved_return_value());
+
     auto frame = pop_call_frame();
 
     Value return_value = js_undefined();
     if (m_return_value.has_value()) {
         return_value = m_return_value.release_value();
-    } else if (m_saved_return_value.has_value() && !m_saved_exception.has_value()) {
-        return_value = m_saved_return_value.release_value();
+    } else if (!saved_return_value.is_empty()) {
+        return_value = saved_return_value;
     }
 
     // NOTE: The return value from a called function is put into $0 in the caller context.
@@ -335,7 +334,6 @@ Interpreter::ValueAndFrame Interpreter::run_and_return_frame(Realm& realm, Execu
     if (m_saved_exception.has_value()) {
         Value thrown_value = m_saved_exception.value();
         m_saved_exception = {};
-        m_saved_return_value = {};
         if (auto* call_frame = frame.get_pointer<NonnullOwnPtr<CallFrame>>())
             return { throw_completion(thrown_value), move(*call_frame) };
         return { throw_completion(thrown_value), nullptr };
@@ -366,8 +364,8 @@ ThrowCompletionOr<void> Interpreter::continue_pending_unwind(Label const& resume
         return throw_completion(m_saved_exception.release_value());
     }
 
-    if (m_saved_return_value.has_value()) {
-        do_return(m_saved_return_value.release_value());
+    if (!saved_return_value().is_empty()) {
+        do_return(saved_return_value());
         return {};
     }
 
diff --git a/Userland/Libraries/LibJS/Bytecode/Interpreter.h b/Userland/Libraries/LibJS/Bytecode/Interpreter.h
index bb0de83e13..dfd57fe936 100644
--- a/Userland/Libraries/LibJS/Bytecode/Interpreter.h
+++ b/Userland/Libraries/LibJS/Bytecode/Interpreter.h
@@ -61,6 +61,7 @@ public:
     ValueAndFrame run_and_return_frame(Realm&, Bytecode::Executable&, Bytecode::BasicBlock const* entry_point, CallFrame* = nullptr);
 
     ALWAYS_INLINE Value& accumulator() { return reg(Register::accumulator()); }
+    ALWAYS_INLINE Value& saved_return_value() { return reg(Register::saved_return_value()); }
     Value& reg(Register const& r) { return registers()[r.index()]; }
 
     auto& saved_lexical_environment_stack() { return call_frame().saved_lexical_environments; }
@@ -119,7 +120,6 @@ private:
     Optional<BasicBlock const*> m_pending_jump;
     BasicBlock const* m_scheduled_jump { nullptr };
     Optional<Value> m_return_value;
-    Optional<Value> m_saved_return_value;
     Optional<Value> m_saved_exception;
     Executable* m_current_executable { nullptr };
     OwnPtr<JS::Interpreter> m_ast_interpreter;
diff --git a/Userland/Libraries/LibJS/Bytecode/Register.h b/Userland/Libraries/LibJS/Bytecode/Register.h
index eaebdd46ca..44c5aceaad 100644
--- a/Userland/Libraries/LibJS/Bytecode/Register.h
+++ b/Userland/Libraries/LibJS/Bytecode/Register.h
@@ -19,6 +19,13 @@ public:
         return Register(accumulator_index);
     }
 
+    constexpr static u32 saved_return_value_index = 1;
+
+    static constexpr Register saved_return_value()
+    {
+        return Register(saved_return_value_index);
+    }
+
     constexpr explicit Register(u32 index)
         : m_index(index)
     {
diff --git a/Userland/Libraries/LibJS/Tests/try-return-finally.js b/Userland/Libraries/LibJS/Tests/try-return-finally.js
new file mode 100644
index 0000000000..b134fc88ef
--- /dev/null
+++ b/Userland/Libraries/LibJS/Tests/try-return-finally.js
@@ -0,0 +1,11 @@
+test("return from try followed by finally with function call inside", () => {
+    let value = (() => {
+        try {
+            return 1;
+        } finally {
+            (() => {})();
+        }
+    })();
+
+    expect(value).toBe(1);
+});