From 4e8654e31b32d6fe870c6dcbfb694376f86c4ba7 Mon Sep 17 00:00:00 2001
From: Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
Date: Tue, 26 Sep 2023 14:48:53 +0200
Subject: [PATCH] LibWeb: Use JS::HeapFunction for HTML::Timer callback

Before the completion_steps for timer were casted from JS::SafeFunction
to Function in HTML::Timer constructor, which is incorrect because then
callback's captured GC-allocated objects are not protected from being
deallocated. Let's modify HTML::Timer to use JS::HeapFunction for the
callback instead.
---
 Userland/Libraries/LibWeb/HTML/Timer.cpp            |  8 +++++---
 Userland/Libraries/LibWeb/HTML/Timer.h              |  5 +++--
 .../LibWeb/HTML/WindowOrWorkerGlobalScope.cpp       | 13 +++++++++----
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/Userland/Libraries/LibWeb/HTML/Timer.cpp b/Userland/Libraries/LibWeb/HTML/Timer.cpp
index 1cdee1f405..e9d41fef29 100644
--- a/Userland/Libraries/LibWeb/HTML/Timer.cpp
+++ b/Userland/Libraries/LibWeb/HTML/Timer.cpp
@@ -13,16 +13,17 @@ namespace Web::HTML {
 
 JS::NonnullGCPtr<Timer> Timer::create(JS::Object& window_or_worker_global_scope, i32 milliseconds, Function<void()> callback, i32 id)
 {
-    return window_or_worker_global_scope.heap().allocate_without_realm<Timer>(window_or_worker_global_scope, milliseconds, move(callback), id);
+    auto heap_function_callback = JS::create_heap_function(window_or_worker_global_scope.heap(), move(callback));
+    return window_or_worker_global_scope.heap().allocate_without_realm<Timer>(window_or_worker_global_scope, milliseconds, heap_function_callback, id);
 }
 
-Timer::Timer(JS::Object& window_or_worker_global_scope, i32 milliseconds, Function<void()> callback, i32 id)
+Timer::Timer(JS::Object& window_or_worker_global_scope, i32 milliseconds, JS::NonnullGCPtr<JS::HeapFunction<void()>> callback, i32 id)
     : m_window_or_worker_global_scope(window_or_worker_global_scope)
     , m_callback(move(callback))
     , m_id(id)
 {
     m_timer = Core::Timer::create_single_shot(milliseconds, [this] {
-        m_callback();
+        m_callback->function()();
     }).release_value_but_fixme_should_propagate_errors();
 }
 
@@ -30,6 +31,7 @@ void Timer::visit_edges(Cell::Visitor& visitor)
 {
     Base::visit_edges(visitor);
     visitor.visit(m_window_or_worker_global_scope.ptr());
+    visitor.visit(m_callback);
 }
 
 Timer::~Timer()
diff --git a/Userland/Libraries/LibWeb/HTML/Timer.h b/Userland/Libraries/LibWeb/HTML/Timer.h
index 0889443155..5ea7fe3e4d 100644
--- a/Userland/Libraries/LibWeb/HTML/Timer.h
+++ b/Userland/Libraries/LibWeb/HTML/Timer.h
@@ -12,6 +12,7 @@
 #include <LibCore/Forward.h>
 #include <LibJS/Heap/Cell.h>
 #include <LibJS/Heap/GCPtr.h>
+#include <LibJS/Heap/HeapFunction.h>
 #include <LibWeb/Forward.h>
 
 namespace Web::HTML {
@@ -27,13 +28,13 @@ public:
     void stop();
 
 private:
-    Timer(JS::Object& window, i32 milliseconds, Function<void()> callback, i32 id);
+    Timer(JS::Object& window, i32 milliseconds, JS::NonnullGCPtr<JS::HeapFunction<void()>> callback, i32 id);
 
     virtual void visit_edges(Cell::Visitor&) override;
 
     RefPtr<Core::Timer> m_timer;
     JS::NonnullGCPtr<JS::Object> m_window_or_worker_global_scope;
-    Function<void()> m_callback;
+    JS::NonnullGCPtr<JS::HeapFunction<void()>> m_callback;
     i32 m_id { 0 };
 };
 
diff --git a/Userland/Libraries/LibWeb/HTML/WindowOrWorkerGlobalScope.cpp b/Userland/Libraries/LibWeb/HTML/WindowOrWorkerGlobalScope.cpp
index ea4c48ac84..72e2beda60 100644
--- a/Userland/Libraries/LibWeb/HTML/WindowOrWorkerGlobalScope.cpp
+++ b/Userland/Libraries/LibWeb/HTML/WindowOrWorkerGlobalScope.cpp
@@ -11,6 +11,7 @@
 #include <AK/String.h>
 #include <AK/Utf8View.h>
 #include <AK/Vector.h>
+#include <LibJS/Heap/HeapFunction.h>
 #include <LibTextCodec/Decoder.h>
 #include <LibWeb/Bindings/MainThreadVM.h>
 #include <LibWeb/Fetch/FetchMethod.h>
@@ -226,8 +227,10 @@ i32 WindowOrWorkerGlobalScopeMixin::run_timer_initialization_steps(TimerHandler
     // 7. Let initiating script be the active script.
     auto const* initiating_script = Web::Bindings::active_script();
 
+    auto& vm = this_impl().vm();
+
     // 8. Let task be a task that runs the following substeps:
-    JS::SafeFunction<void()> task = [this, handler = move(handler), timeout, arguments = move(arguments), repeat, id, initiating_script]() mutable {
+    auto task = JS::create_heap_function(vm.heap(), Function<void()>([this, handler = move(handler), timeout, arguments = move(arguments), repeat, id, initiating_script]() {
         // 1. If id does not exist in global's map of active timers, then abort these steps.
         if (!m_timers.contains(id))
             return;
@@ -288,14 +291,16 @@ i32 WindowOrWorkerGlobalScopeMixin::run_timer_initialization_steps(TimerHandler
             m_timers.remove(id);
             break;
         }
-    };
+    }));
 
     // FIXME: 9. Increment nesting level by one.
     // FIXME: 10. Set task's timer nesting level to nesting level.
 
     // 11. Let completionStep be an algorithm step which queues a global task on the timer task source given global to run task.
-    JS::SafeFunction<void()> completion_step = [this, task = move(task)]() mutable {
-        queue_global_task(Task::Source::TimerTask, this_impl(), move(task));
+    Function<void()> completion_step = [this, task = move(task)]() mutable {
+        queue_global_task(Task::Source::TimerTask, this_impl(), [task] {
+            task->function()();
+        });
     };
 
     // 12. Run steps after a timeout given global, "setTimeout/setInterval", timeout, completionStep, and id.