From 4677a016bc65c0d863867d1a781a4477ca4d131d Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 10:51:04 +0200 Subject: [PATCH] fix(wall): hold the PCP holder in a Global, not a Persistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #385. v8::Persistent has no destruction behaviour: the handle leaks unless every path clears it by hand. v8::Global releases it in its own destructor, and is what every other handle in this file already uses — ContextPtr, cpedKey_, wrapObjectTemplate_, jsArray_. The Persistent introduced in #385 was the odd one out. Nothing was leaking in practice, since ~PersistentContextPtr always reset the handle explicitly, but relying on that is exactly the footgun the V8 docs warn about. Switching to Global makes the release structural, so the explicit Reset goes away with it. Historically the manual handle was justified: before #261 removed instance reuse, PersistentContextPtr recycled itself through a freelist and needed ClearWeak/Reset to unregister and re-register the same object. With reuse gone a handle lives exactly as long as its PCP, so there is nothing left for Persistent's manual semantics to buy. Also correct the destructor comment. It claimed the reset was "a no-op when we got here from WeakCallback itself", which is backwards: V8 requires a weak callback to reset the handle, so that path is precisely where the release is load-bearing. Verified on Node 20, 24 and 26 — the last is where AsyncContextFrame is on by default and PCPs are actually created. 163 passing, ASAN exit 0 with no leaks and no aborts on 20 and 24. --- bindings/profilers/wall.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index dee88687..6f33179b 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -161,7 +161,7 @@ class PersistentContextPtr { // Weak handle on the holder object. Owns this PCP: when V8 collects the // holder, WeakCallback deletes us. - v8::Persistent handle_; + v8::Global handle_; friend class WallProfiler; @@ -214,11 +214,6 @@ PersistentContextPtr::~PersistentContextPtr() { if (next_ != nullptr) next_->pprev_ = pprev_; profiler_->recordContextRelease(); } - // Cancels the weak callback when we're deleted by ~WallProfiler rather than - // by V8; a no-op when we got here from WeakCallback itself. The holder - // object's internal field is left dangling either way, but nothing reads it - // once the owning profiler is gone. - handle_.Reset(); } // Maximum number of rounds in the GetV8ToEpochOffset