From 07190b829652f353fc094b71ef70ade14bc39d4d Mon Sep 17 00:00:00 2001 From: Arshia Ghafoori Date: Fri, 31 Jul 2026 13:09:04 +0000 Subject: [PATCH] fix: drain the platform's fallback foreground-task queue (ECO-415 follow-up) V8 posts some of its own deferred work -- most importantly Heap::PostFinalizationRegistryCleanupTaskIfNeeded's cleanup task, queued after a GC finds a JSFinalizationRegistry with dead targets -- via v8::TaskRunner::PostNonNestableTask on the runner EdgeV8Platform hands back for the isolate. That runner forwards to the guest's own enqueue callback when one is bound (BindForegroundTaskTarget), but nothing requires the guest to bind one; a guest may drive everything through unofficial_napi_process_microtasks instead, as edgejs does. Tasks posted with no guest target bound fall back to the stock default-platform runner (EdgeV8Platform::ForegroundTaskRunner::PostTaskCommon's fallback branch), and until now nothing ever pumped that runner's queue: the tasks were posted and then silently never ran. GC still correctly collected the dead targets, but the FinalizationRegistry callbacks that were supposed to fire afterward never did -- confirmed via a standalone V8 harness (same prebuilt binary, stock platform + PumpMessageLoop: 95% finalized) against the unpatched bridge (0/120000 finalized under identical GC pressure, edgejs's own WeakRef/FinalizationRegistry-based AbortSignal cleanup leaking as a result). Add EdgeV8Platform::PumpPendingForegroundTasks(isolate), which drains the fallback platform's message loop, and call it from DrainMicrotasksForEnv alongside the existing microtask checkpoint -- the same point already pumped reliably by every guest event-loop tick, regardless of whether that guest ever wires up its own foreground-task hook. Co-Authored-By: Claude Sonnet 5 --- v8/src/edge_v8_platform.cc | 14 ++++++++++++++ v8/src/edge_v8_platform.h | 1 + v8/src/unofficial_napi.cc | 28 ++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/v8/src/edge_v8_platform.cc b/v8/src/edge_v8_platform.cc index f6406e4..d9ddee5 100644 --- a/v8/src/edge_v8_platform.cc +++ b/v8/src/edge_v8_platform.cc @@ -489,6 +489,20 @@ void EdgeV8Platform::ClearForegroundTaskTarget(v8::Isolate* isolate, napi_env en state->runner->ClearTarget(env); } +void EdgeV8Platform::PumpPendingForegroundTasks(v8::Isolate* isolate) { + if (fallback_ == nullptr || isolate == nullptr) return; + // ForegroundTaskRunner::PostTaskCommon forwards to the guest's bound + // enqueue callback when one is set, but falls back to fallback_'s own + // GetForegroundTaskRunner() when no guest target is bound (e.g. the guest + // drives everything through unofficial_napi_process_microtasks and never + // calls BindForegroundTaskTarget). Nothing else pumps that fallback + // runner's queue, so tasks routed there -- including V8-internal work like + // Heap::PostFinalizationRegistryCleanupTaskIfNeeded's cleanup task -- would + // otherwise be posted and never run. Drain it explicitly. + while (v8::platform::PumpMessageLoop(fallback_.get(), isolate)) { + } +} + int EdgeV8Platform::NumberOfWorkerThreads() { return fallback_ != nullptr ? fallback_->NumberOfWorkerThreads() : 0; } diff --git a/v8/src/edge_v8_platform.h b/v8/src/edge_v8_platform.h index 04b346c..707fa32 100644 --- a/v8/src/edge_v8_platform.h +++ b/v8/src/edge_v8_platform.h @@ -33,6 +33,7 @@ class EdgeV8Platform final : public v8::Platform { void ClearForegroundTaskTarget(v8::Isolate* isolate, napi_env env); void AddPendingForegroundTask(const std::shared_ptr& state); void CompletePendingForegroundTask(const std::shared_ptr& state); + void PumpPendingForegroundTasks(v8::Isolate* isolate); int NumberOfWorkerThreads() override; std::shared_ptr GetForegroundTaskRunner( diff --git a/v8/src/unofficial_napi.cc b/v8/src/unofficial_napi.cc index d4facde..1c695f7 100644 --- a/v8/src/unofficial_napi.cc +++ b/v8/src/unofficial_napi.cc @@ -2142,6 +2142,32 @@ napi_status NAPI_CDECL unofficial_napi_set_prepare_stack_trace_callback( return napi_ok; } +// V8 posts some of its own deferred foreground work (most notably +// Heap::PostFinalizationRegistryCleanupTaskIfNeeded's cleanup task, run after +// GC finds a JSFinalizationRegistry with dead targets) via +// v8::TaskRunner::PostNonNestableTask on the runner EdgeV8Platform hands back +// for the isolate. That runner forwards to the guest's own enqueue callback +// when one is bound (see EdgeV8Platform::BindForegroundTaskTarget), but the +// guest is not required to bind one -- it may drive everything through +// unofficial_napi_process_microtasks instead. Tasks posted with no guest +// target bound fall back to the stock default-platform runner, and nothing +// else ever pumps that runner's queue, so without this they are posted and +// then never run: V8 correctly collects the dead targets, but their +// FinalizationRegistry callbacks never fire. Pump it here, at the same point +// microtasks are already checkpointed, so this deferred work always gets a +// chance to run regardless of whether the guest wired up its own hook. +void PumpPlatformForegroundTasks(napi_env env) { + if (env == nullptr || env->isolate == nullptr) return; + EdgeV8Platform* platform = nullptr; + { + std::lock_guard lock(g_runtime_mu); + platform = g_runtime.platform.get(); + } + if (platform != nullptr) { + platform->PumpPendingForegroundTasks(env->isolate); + } +} + void DrainMicrotasksForEnv(napi_env env) { if (env == nullptr || env->isolate == nullptr) return; env->DrainFinalizerQueue(); @@ -2151,11 +2177,13 @@ void DrainMicrotasksForEnv(napi_env env) { if (queue != nullptr) { queue->PerformCheckpoint(env->isolate); env->DrainFinalizerQueue(); + PumpPlatformForegroundTasks(env); return; } } env->isolate->PerformMicrotaskCheckpoint(); env->DrainFinalizerQueue(); + PumpPlatformForegroundTasks(env); } napi_status NAPI_CDECL unofficial_napi_request_gc_for_testing(napi_env env) {