From 51a91771b827c4477576d40e62ecf7aac537b8d0 Mon Sep 17 00:00:00 2001
From: Andreas Kling <andreas@ladybird.org>
Date: Thu, 23 Jan 2025 09:59:22 +0100
Subject: [PATCH] LibJS+LibGC: Run FinalizationRegistry cleanup host hook
 *after* GC

Before this change, it was possible for a second GC to get triggered
in the middle of a first GC, due to allocations happening in the
FinalizationRegistry cleanup host hook. To avoid this causing problems,
we add a "post-GC task" mechanism and use that to invoke the host hook
once all other GC activity is finished, and we've unset the "collecting
garbage" flag.

Note that the test included here only fails reliably when running with
the -g flag (collect garbage after each allocation).

Fixes #3051
---
 Libraries/LibGC/Heap.cpp                      | 38 ++++++++++++-------
 Libraries/LibGC/Heap.h                        |  4 ++
 .../LibJS/Runtime/FinalizationRegistry.cpp    |  9 ++++-
 .../Crash/JS/finalization-registry-basic.html | 19 ++++++++++
 4 files changed, 55 insertions(+), 15 deletions(-)
 create mode 100644 Tests/LibWeb/Crash/JS/finalization-registry-basic.html

diff --git a/Libraries/LibGC/Heap.cpp b/Libraries/LibGC/Heap.cpp
index 99b5643e03e..3c907b608cb 100644
--- a/Libraries/LibGC/Heap.cpp
+++ b/Libraries/LibGC/Heap.cpp
@@ -240,23 +240,35 @@ AK::JsonObject Heap::dump_graph()
 void Heap::collect_garbage(CollectionType collection_type, bool print_report)
 {
     VERIFY(!m_collecting_garbage);
-    TemporaryChange change(m_collecting_garbage, true);
 
-    Core::ElapsedTimer collection_measurement_timer;
-    if (print_report)
-        collection_measurement_timer.start();
+    {
+        TemporaryChange change(m_collecting_garbage, true);
 
-    if (collection_type == CollectionType::CollectGarbage) {
-        if (m_gc_deferrals) {
-            m_should_gc_when_deferral_ends = true;
-            return;
+        Core::ElapsedTimer collection_measurement_timer;
+        if (print_report)
+            collection_measurement_timer.start();
+
+        if (collection_type == CollectionType::CollectGarbage) {
+            if (m_gc_deferrals) {
+                m_should_gc_when_deferral_ends = true;
+                return;
+            }
+            HashMap<Cell*, HeapRoot> roots;
+            gather_roots(roots);
+            mark_live_cells(roots);
         }
-        HashMap<Cell*, HeapRoot> roots;
-        gather_roots(roots);
-        mark_live_cells(roots);
+        finalize_unmarked_cells();
+        sweep_dead_cells(print_report, collection_measurement_timer);
     }
-    finalize_unmarked_cells();
-    sweep_dead_cells(print_report, collection_measurement_timer);
+
+    auto tasks = move(m_post_gc_tasks);
+    for (auto& task : tasks)
+        task();
+}
+
+void Heap::enqueue_post_gc_task(AK::Function<void()> task)
+{
+    m_post_gc_tasks.append(move(task));
 }
 
 void Heap::gather_roots(HashMap<Cell*, HeapRoot>& roots)
diff --git a/Libraries/LibGC/Heap.h b/Libraries/LibGC/Heap.h
index e9275ca5507..5b39c1b6f80 100644
--- a/Libraries/LibGC/Heap.h
+++ b/Libraries/LibGC/Heap.h
@@ -76,6 +76,8 @@ public:
 
     bool is_gc_deferred() const { return m_gc_deferrals > 0; }
 
+    void enqueue_post_gc_task(AK::Function<void()>);
+
 private:
     friend class MarkingVisitor;
     friend class GraphConstructorVisitor;
@@ -151,6 +153,8 @@ private:
     bool m_collecting_garbage { false };
     StackInfo m_stack_info;
     AK::Function<void(HashMap<Cell*, GC::HeapRoot>&)> m_gather_embedder_roots;
+
+    Vector<AK::Function<void()>> m_post_gc_tasks;
 } SWIFT_IMMORTAL_REFERENCE;
 
 inline void Heap::did_create_root(Badge<RootImpl>, RootImpl& impl)
diff --git a/Libraries/LibJS/Runtime/FinalizationRegistry.cpp b/Libraries/LibJS/Runtime/FinalizationRegistry.cpp
index 47e03a64607..2f03549141f 100644
--- a/Libraries/LibJS/Runtime/FinalizationRegistry.cpp
+++ b/Libraries/LibJS/Runtime/FinalizationRegistry.cpp
@@ -57,8 +57,13 @@ void FinalizationRegistry::remove_dead_cells(Badge<GC::Heap>)
         any_cells_were_removed = true;
         break;
     }
-    if (any_cells_were_removed)
-        vm().host_enqueue_finalization_registry_cleanup_job(*this);
+    if (any_cells_were_removed) {
+        // NOTE: We make a GC::Root here to ensure that the FinalizationRegistry stays alive
+        //       even if a subsequent GC is triggered before the callback has a chance to run.
+        heap().enqueue_post_gc_task([that = GC::make_root(this)]() {
+            that->vm().host_enqueue_finalization_registry_cleanup_job(*that);
+        });
+    }
 }
 
 // 9.13 CleanupFinalizationRegistry ( finalizationRegistry ), https://tc39.es/ecma262/#sec-cleanup-finalization-registry
diff --git a/Tests/LibWeb/Crash/JS/finalization-registry-basic.html b/Tests/LibWeb/Crash/JS/finalization-registry-basic.html
new file mode 100644
index 00000000000..5287da46dc6
--- /dev/null
+++ b/Tests/LibWeb/Crash/JS/finalization-registry-basic.html
@@ -0,0 +1,19 @@
+<script src="../include.js"></script>
+<script>
+
+// NOTE: This test is only reliable when GC'ing after each allocation.
+
+const registry = new FinalizationRegistry((heldValue) => {
+    console.log(heldValue);
+});
+
+const makeGarbage = () => {
+    registry.register(new String("ok"), "hello");
+};
+makeGarbage();
+
+if (window.internals !== undefined)
+    internals.gc();
+
+println("PASS");
+</script>