From 4f73c80fec827f5e84646080729ff53e43243767 Mon Sep 17 00:00:00 2001 From: veeshi <31014797+veeshi@users.noreply.github.com> Date: Mon, 19 Oct 2026 20:54:35 +0100 Subject: [PATCH 1/4] Run FinalizationRegistry cleanup callbacks Set the host cleanup callback, run queued cleanups after each microtask checkpoint, and drain the queue after the pre-initialization GC so the snapshot never holds queued cleanups. Fixes #327. --- runtime/engine.cpp | 6 ++ runtime/event_loop.cpp | 66 ++++++++++++++++++- runtime/event_loop.h | 11 ++++ .../expect_serve_body.txt | 1 + .../finalization-registry-snapshot.js | 16 +++++ .../expect_serve_body.txt | 1 + .../finalization-registry.js | 24 +++++++ tests/tests.cmake | 2 + 8 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 tests/e2e/finalization-registry-snapshot/expect_serve_body.txt create mode 100644 tests/e2e/finalization-registry-snapshot/finalization-registry-snapshot.js create mode 100644 tests/e2e/finalization-registry/expect_serve_body.txt create mode 100644 tests/e2e/finalization-registry/finalization-registry.js diff --git a/runtime/engine.cpp b/runtime/engine.cpp index e9da55dd..69a3b1ac 100644 --- a/runtime/engine.cpp +++ b/runtime/engine.cpp @@ -638,6 +638,12 @@ bool Engine::eval_toplevel(JS::SourceText &source, std::strin if (state() == EngineState::ScriptPreInitializing) { JS::PrepareForFullGC(cx()); JS::NonIncrementalGC(cx(), JS::GCOptions::Normal, JS::GCReason::API); + + // The collection may have queued FinalizationRegistry cleanups. Run them now, so they are + // not baked into the snapshot and run on its first resumption. + if (!core::EventLoop::drain_finalization_registry_cleanups(this)) { + return false; + } } // Ignore the first GC, but then print all others, because ideally GCs diff --git a/runtime/event_loop.cpp b/runtime/event_loop.cpp index cdcdaabf..dff9b4a5 100644 --- a/runtime/event_loop.cpp +++ b/runtime/event_loop.cpp @@ -3,6 +3,7 @@ #include "host_api.h" #include "jsapi.h" #include "jsfriendapi.h" +#include "js/GCVector.h" #include #include @@ -22,6 +23,44 @@ struct TaskQueue { static PersistentRooted queue; +// SpiderMonkey hands the embedding one cleanup function per FinalizationRegistry that has +// records of dead targets, and leaves calling it to the embedding (HTML's +// HostEnqueueFinalizationRegistryCleanupJob). The functions are queued here and run by the event +// loop, as a task after the microtask checkpoint. +using CleanupFunctions = JS::GCVector; +static PersistentRooted finalization_registry_cleanups; + +// Called during a collection, so it must not collect: it only appends to the queue. If the +// append fails, that registry's records stay queued in SpiderMonkey and are offered again after +// a later collection. +static void queue_finalization_registry_cleanup(JSFunction *do_cleanup, JSObject *, void *) { + (void)finalization_registry_cleanups.get().append(do_cleanup); +} + +// Calls every queued cleanup function and returns whether any ran. An exception thrown by a +// callback is reported and cleared, and does not fail the event loop, as in a browser. +static bool run_finalization_registry_cleanups(api::Engine *engine) { + if (finalization_registry_cleanups.get().empty()) { + return false; + } + JSContext *cx = engine->cx(); + JS::Rooted cleanups(cx); + std::swap(cleanups.get(), finalization_registry_cleanups.get()); + JS::RootedFunction cleanup(cx); + JS::RootedValue rval(cx); + for (JSFunction *fn : cleanups) { + cleanup = fn; + JS::RootedObject cleanup_obj(cx, JS_GetFunctionObject(cleanup)); + JS::ExposeObjectToActiveJS(cleanup_obj); + JSAutoRealm ar(cx, cleanup_obj); + if (!JS_CallFunction(cx, nullptr, cleanup, JS::HandleValueArray::empty(), &rval)) { + api::Engine::dump_pending_exception("running a FinalizationRegistry callback"); + JS_ClearPendingException(cx); + } + } + return true; +} + namespace core { void EventLoop::queue_async_task(const RefPtr& task) { @@ -70,6 +109,13 @@ bool EventLoop::run_event_loop(api::Engine *engine, double total_compute) { exit_event_loop(); return false; } + + // Once the microtasks are done, run the FinalizationRegistry cleanups that a collection + // queued, then go round again for the microtasks they queued. + if (run_finalization_registry_cleanups(engine)) { + continue; + } + // if there is no interest in the event loop at all, just run one tick if (interest_complete()) { exit_event_loop(); @@ -97,6 +143,24 @@ bool EventLoop::run_event_loop(api::Engine *engine, double total_compute) { } } -void EventLoop::init(JSContext *cx) { queue.init(cx); } +bool EventLoop::drain_finalization_registry_cleanups(api::Engine *engine) { + JSContext *cx = engine->cx(); + for (unsigned round = 0; round < MAX_CLEANUP_DRAIN_ROUNDS; round++) { + if (!run_finalization_registry_cleanups(engine)) { + return true; + } + js::RunJobs(cx); + if (JS_IsExceptionPending(cx)) { + return false; + } + } + return true; +} + +void EventLoop::init(JSContext *cx) { + queue.init(cx); + finalization_registry_cleanups.init(cx); + JS::SetHostCleanupFinalizationRegistryCallback(cx, queue_finalization_registry_cleanup, nullptr); +} } // namespace core diff --git a/runtime/event_loop.h b/runtime/event_loop.h index acca516c..18fab78e 100644 --- a/runtime/event_loop.h +++ b/runtime/event_loop.h @@ -24,6 +24,17 @@ class EventLoop { */ static bool run_event_loop(api::Engine *engine, double total_compute); + /** + * Run the FinalizationRegistry cleanups that collections have queued, and the microtasks they + * queue, until none are left. Used before a Wizer snapshot is taken, so that the snapshot holds + * an empty queue. A cleanup that triggers a collection that queues more is run in a further + * round, up to MAX_CLEANUP_DRAIN_ROUNDS; anything still queued then stays queued, and runs in + * the first event loop after the snapshot is resumed. Returns false if an exception is pending. + */ + static bool drain_finalization_registry_cleanups(api::Engine *engine); + + static constexpr unsigned MAX_CLEANUP_DRAIN_ROUNDS = 16; + static void incr_event_loop_interest(); static void decr_event_loop_interest(); diff --git a/tests/e2e/finalization-registry-snapshot/expect_serve_body.txt b/tests/e2e/finalization-registry-snapshot/expect_serve_body.txt new file mode 100644 index 00000000..b78b9739 --- /dev/null +++ b/tests/e2e/finalization-registry-snapshot/expect_serve_body.txt @@ -0,0 +1 @@ +finalized 1 \ No newline at end of file diff --git a/tests/e2e/finalization-registry-snapshot/finalization-registry-snapshot.js b/tests/e2e/finalization-registry-snapshot/finalization-registry-snapshot.js new file mode 100644 index 00000000..85bc45cd --- /dev/null +++ b/tests/e2e/finalization-registry-snapshot/finalization-registry-snapshot.js @@ -0,0 +1,16 @@ +// The collection that runs before the Wizer snapshot is taken finds the target registered below +// dead. Its cleanup must have run before the snapshot, not be baked into it and run after it is +// resumed: the handler answers synchronously, before the first event loop of the resumed +// instance, so it sees the count only if the cleanup ran during pre-initialization. +let finalized = 0; +const registry = new FinalizationRegistry(() => { + finalized += 1; +}); + +// In a function, so that no top-level frame still refers to the target. +function register() { + registry.register({}, 0); +} +register(); + +addEventListener("fetch", (event) => event.respondWith(new Response(`finalized ${finalized}`))); diff --git a/tests/e2e/finalization-registry/expect_serve_body.txt b/tests/e2e/finalization-registry/expect_serve_body.txt new file mode 100644 index 00000000..ba763258 --- /dev/null +++ b/tests/e2e/finalization-registry/expect_serve_body.txt @@ -0,0 +1 @@ +finalized \ No newline at end of file diff --git a/tests/e2e/finalization-registry/finalization-registry.js b/tests/e2e/finalization-registry/finalization-registry.js new file mode 100644 index 00000000..4b3686d7 --- /dev/null +++ b/tests/e2e/finalization-registry/finalization-registry.js @@ -0,0 +1,24 @@ +// A FinalizationRegistry callback runs once a collection has found its target dead. The +// collection is started by allocation, and the callback runs in the event loop, after the +// microtask checkpoint that follows it. +let finalized = 0; +const registry = new FinalizationRegistry(() => { + finalized += 1; +}); + +async function handle() { + registry.register({}, 0); + + // Allocate until a collection has run and its cleanup has been called, giving up after a + // bounded number of attempts. + for (let i = 0; i < 1000 && finalized === 0; i++) { + const buffer = new Uint8Array(2 << 20); + buffer[0] = 1; + // Wait for a task, so that the cleanups queued by a collection can run. + await new Promise((resolve) => setTimeout(resolve, 0)); + } + + return new Response(finalized > 0 ? "finalized" : "not finalized"); +} + +addEventListener("fetch", (event) => event.respondWith(handle())); diff --git a/tests/tests.cmake b/tests/tests.cmake index d7d6b086..f3e74279 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -61,6 +61,8 @@ test_e2e(teed-stream-as-outgoing-body) test_e2e(init-script) test_e2e(no-init-location) test_e2e(init-location) +test_e2e(finalization-registry) +test_e2e(finalization-registry-snapshot) integration_tests( blob From 66537a007523481b6bf4abc09bc6cb90e8d55eec Mon Sep 17 00:00:00 2001 From: veeshi <31014797+veeshi@users.noreply.github.com> Date: Mon, 19 Oct 2026 21:27:23 +0100 Subject: [PATCH 2/4] Name the unused cleanup callback parameters for clang-tidy --- runtime/event_loop.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/runtime/event_loop.cpp b/runtime/event_loop.cpp index dff9b4a5..3c7d369b 100644 --- a/runtime/event_loop.cpp +++ b/runtime/event_loop.cpp @@ -33,7 +33,9 @@ static PersistentRooted finalization_registry_cleanups; // Called during a collection, so it must not collect: it only appends to the queue. If the // append fails, that registry's records stay queued in SpiderMonkey and are offered again after // a later collection. -static void queue_finalization_registry_cleanup(JSFunction *do_cleanup, JSObject *, void *) { +static void queue_finalization_registry_cleanup(JSFunction *do_cleanup, + JSObject * /*incumbent_global*/, + void * /*data*/) { (void)finalization_registry_cleanups.get().append(do_cleanup); } From d48f2f69f40fab8831f33ee1ef9c438fb5aeb722 Mon Sep 17 00:00:00 2001 From: veeshi <31014797+veeshi@users.noreply.github.com> Date: Sun, 25 Oct 2026 18:21:35 +0000 Subject: [PATCH 3/4] Check for a TLA rejection again after running FinalizationRegistry cleanups A cleanup run after the pre-snapshot collection, or a microtask it queues, can reject the top-level await promise. Repeat the rejection check after draining the cleanups, so that a failing module is not snapshotted, and add an e2e test in which a cleanup rejects a pending top-level await. --- runtime/engine.cpp | 23 +++++++++++++++---- .../expect_wizer_fail | 0 .../expect_wizer_stderr.txt | 2 ++ .../finalization-registry-tla-err.js | 15 ++++++++++++ tests/tests.cmake | 1 + 5 files changed, 36 insertions(+), 5 deletions(-) create mode 100644 tests/e2e/finalization-registry-tla-err/expect_wizer_fail create mode 100644 tests/e2e/finalization-registry-tla-err/expect_wizer_stderr.txt create mode 100644 tests/e2e/finalization-registry-tla-err/finalization-registry-tla-err.js diff --git a/runtime/engine.cpp b/runtime/engine.cpp index 69a3b1ac..e4957b9c 100644 --- a/runtime/engine.cpp +++ b/runtime/engine.cpp @@ -601,14 +601,21 @@ bool Engine::eval_toplevel(JS::SourceText &source, std::strin // TLA rejections during pre-initialization are treated as top-level exceptions. // TLA may remain unresolved, in which case it will continue tasks at runtime. // Rejections after pre-intialization remain unhandled rejections for now. - if (tla_promise.isObject()) { + auto tla_rejected = [&]() { + if (!tla_promise.isObject()) { + return false; + } RootedObject promise_obj(cx(), &tla_promise.toObject()); - JS::PromiseState state = JS::GetPromiseState(promise_obj); - if (state == JS::PromiseState::Rejected) { - RootedValue err(cx(), JS::GetPromiseResult(promise_obj)); - JS_SetPendingException(cx(), err); + if (JS::GetPromiseState(promise_obj) != JS::PromiseState::Rejected) { return false; } + RootedValue err(cx(), JS::GetPromiseResult(promise_obj)); + JS_SetPendingException(cx(), err); + return true; + }; + + if (tla_rejected()) { + return false; } // Report any promise rejections that weren't handled before snapshotting. @@ -644,6 +651,12 @@ bool Engine::eval_toplevel(JS::SourceText &source, std::strin if (!core::EventLoop::drain_finalization_registry_cleanups(this)) { return false; } + + // A cleanup, or a microtask it queued, may have rejected the TLA promise. Check again, so + // that a failing module is not snapshotted. + if (tla_rejected()) { + return false; + } } // Ignore the first GC, but then print all others, because ideally GCs diff --git a/tests/e2e/finalization-registry-tla-err/expect_wizer_fail b/tests/e2e/finalization-registry-tla-err/expect_wizer_fail new file mode 100644 index 00000000..e69de29b diff --git a/tests/e2e/finalization-registry-tla-err/expect_wizer_stderr.txt b/tests/e2e/finalization-registry-tla-err/expect_wizer_stderr.txt new file mode 100644 index 00000000..99544c94 --- /dev/null +++ b/tests/e2e/finalization-registry-tla-err/expect_wizer_stderr.txt @@ -0,0 +1,2 @@ +Exception while evaluating top-level script +e2e/finalization-registry-tla-err/finalization-registry-tla-err.js:6:10 Error: rejected by a FinalizationRegistry cleanup diff --git a/tests/e2e/finalization-registry-tla-err/finalization-registry-tla-err.js b/tests/e2e/finalization-registry-tla-err/finalization-registry-tla-err.js new file mode 100644 index 00000000..68e79bec --- /dev/null +++ b/tests/e2e/finalization-registry-tla-err/finalization-registry-tla-err.js @@ -0,0 +1,15 @@ +// The collection that runs before the Wizer snapshot is taken finds the target registered below +// dead, and its cleanup rejects the pending top-level await. Pre-initialization must fail, as it +// does for any TLA rejection, rather than snapshot a failed module. +let reject; +const registry = new FinalizationRegistry(() => { + reject(new Error('rejected by a FinalizationRegistry cleanup')); +}); + +// In a function, so that no top-level frame still refers to the target. +function register() { + registry.register({}, 0); +} +register(); + +await new Promise((_, _reject) => void (reject = _reject)); diff --git a/tests/tests.cmake b/tests/tests.cmake index f3e74279..3f938343 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -63,6 +63,7 @@ test_e2e(no-init-location) test_e2e(init-location) test_e2e(finalization-registry) test_e2e(finalization-registry-snapshot) +test_e2e(finalization-registry-tla-err) integration_tests( blob From a394fe33af55cd6c7c9b92064695ed0ade3dff01 Mon Sep 17 00:00:00 2001 From: veeshi <31014797+veeshi@users.noreply.github.com> Date: Sun, 25 Oct 2026 18:21:35 +0000 Subject: [PATCH 4/4] Treat a failed FinalizationRegistry cleanup append as fatal Follow SpiderMonkey's shell (ShellCleanupFinalizationRegistryCallback): the append can only fail on allocation failure, so crash in an AutoEnterOOMUnsafeRegion instead of ignoring it. Reword the comment to say the callback is called during GC, so it must not trigger another collection. --- runtime/event_loop.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/runtime/event_loop.cpp b/runtime/event_loop.cpp index 3c7d369b..dabadd3e 100644 --- a/runtime/event_loop.cpp +++ b/runtime/event_loop.cpp @@ -4,6 +4,7 @@ #include "jsapi.h" #include "jsfriendapi.h" #include "js/GCVector.h" +#include "js/Utility.h" #include #include @@ -30,13 +31,15 @@ static PersistentRooted queue; using CleanupFunctions = JS::GCVector; static PersistentRooted finalization_registry_cleanups; -// Called during a collection, so it must not collect: it only appends to the queue. If the -// append fails, that registry's records stay queued in SpiderMonkey and are offered again after -// a later collection. +// Called during GC, so this must not trigger another collection: it only appends to the queue. +// An append can only fail on allocation failure, which is fatal, as in SpiderMonkey's shell. static void queue_finalization_registry_cleanup(JSFunction *do_cleanup, JSObject * /*incumbent_global*/, void * /*data*/) { - (void)finalization_registry_cleanups.get().append(do_cleanup); + js::AutoEnterOOMUnsafeRegion oom_unsafe; + if (!finalization_registry_cleanups.get().append(do_cleanup)) { + oom_unsafe.crash("queue_finalization_registry_cleanup"); + } } // Calls every queued cleanup function and returns whether any ran. An exception thrown by a