From 8dfa9bbd1d387f60f405bdbba911e298d616e006 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Mon, 10 Aug 2026 16:59:03 +0200 Subject: [PATCH 1/2] Follow up on #388 review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review nits from #388, all valid. Deduplicate the internal-field accessors. wall.cc had its own copies of GetAlignedPointerFromInternalField / SetAlignedPointerInInternalField; #388 added the same pair in internal-field.hh and deliberately left wall.cc alone to avoid conflicting with #387, which was in flight. #387 has landed, so wall.cc now includes the header and its copies are gone. Same namespace and names, so every call site is unchanged. Register the drain hook in Init() rather than lazily on first Wrap(), which removes g_drain_hook_registered entirely. Module initialisation always runs with a context entered, so AddEnvironmentCleanupHook's CHECK is satisfied there too, and Init() runs exactly once per isolate — which is the lifetime the hook should match. The flag existed only to make the lazy registration idempotent and to re-arm after an isolate was torn down and recreated on the same thread; Init() running again on the new isolate covers that by construction. Clear the holder's internal field before freeing the CtxWrap it points at. The drain hook now nulls slot 0 on its way through the list. This matters more than a tidiness nit: that slot is exactly what the out-of-process OTEP-4947 reader walks, so leaving it pointing at freed memory is a loaded gun aimed at a consumer we do not control. Being on the live list means V8 has not collected the holder, so reading the handle there is safe; the WeakCallback path cannot do this and does not need to, since there the holder is the object being collected. Verified on Node 20, 24 and 26: ASAN exit 0 with zero leaks and zero aborts on 20 and 24, 165 passing on 24 and 26, the teardown regression test passing where the OTEP block runs, the original repro clean at N=3000 and N=10000, and the published native_wrap_fields_offset still 0. --- bindings/otel-thread-ctx.cc | 38 +++++++++++++++++++------------------ bindings/profilers/wall.cc | 21 +------------------- 2 files changed, 21 insertions(+), 38 deletions(-) diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index 69d7aa43..8ef326c3 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -292,29 +292,34 @@ static_assert(offsetof(CtxWrap, record_) == 0, // wall profiler uses for its active-profiler pointer. // `otel_thread_ctx_nodejs_v1` above is thread-local for the same reason. thread_local CtxWrap* g_live_ctx_wraps = nullptr; -// Whether DrainLiveCtxWraps is registered for the current isolate. Cleared by -// the drain itself so an isolate torn down and re-created on the same thread -// re-registers, matching how `undefined_addr` gates ResetDiscoveryStruct. -thread_local bool g_drain_hook_registered = false; - -// Delete every CtxWrap V8 has not collected yet. This is the teardown -// deletion that node::ObjectWrap's per-instance cleanup hook used to provide; -// without it the records would simply leak at exit. Registered once per -// isolate from Wrap(), which runs inside a JS constructor call where a -// context is entered, so AddEnvironmentCleanupHook's own CHECK is satisfied, -// and never removed — it fires exactly once, at teardown, while the -// Environment is still alive. -void DrainLiveCtxWraps(void* /*arg*/) { + +// Delete every CtxWrap V8 has not collected yet. This is the teardown deletion +// that node::ObjectWrap's per-instance cleanup hook used to provide; without it +// the records would simply leak at exit. Registered once per isolate from +// Init(), which runs at module initialisation with a context entered, so +// AddEnvironmentCleanupHook's own CHECK is satisfied, and never removed — it +// fires exactly once, at teardown, while the Environment is still alive. +void DrainLiveCtxWraps(void* arg) { + auto* isolate = static_cast(arg); + v8::HandleScope scope(isolate); CtxWrap* p = g_live_ctx_wraps; while (p != nullptr) { CtxWrap* next = p->next_; p->pprev_ = nullptr; p->next_ = nullptr; + // Clear the holder's internal field (containing p as pointer value), so + // nothing can reach a dangling CtxWrap through it including the + // out-of-process reader, which walks this slot. Being on the live list + // means V8 has not collected the holder, so the handle is safe to read + // here; the WeakCallback path cannot do this and does not need to, + // since there the holder is the thing being collected. + if (!p->handle_.IsEmpty()) { + SetAlignedPointerInInternalField(p->handle_.Get(isolate), 0, nullptr); + } delete p; p = next; } g_live_ctx_wraps = nullptr; - g_drain_hook_registered = false; } CtxWrap::~CtxWrap() { @@ -334,10 +339,6 @@ void CtxWrap::WeakCallback(const v8::WeakCallbackInfo& data) { void CtxWrap::Wrap(Local holder) { Isolate* isolate = Isolate::GetCurrent(); - if (!g_drain_hook_registered) { - node::AddEnvironmentCleanupHook(isolate, DrainLiveCtxWraps, nullptr); - g_drain_hook_registered = true; - } SetAlignedPointerInInternalField(holder, 0, this); handle_.Reset(isolate, holder); handle_.SetWeak(this, &WeakCallback, v8::WeakCallbackType::kParameter); @@ -690,6 +691,7 @@ void CtxWrap::DebugBytes(const FunctionCallbackInfo& args) { void CtxWrap::Init(Local exports) { Isolate* isolate = Isolate::GetCurrent(); + node::AddEnvironmentCleanupHook(isolate, DrainLiveCtxWraps, isolate); Local context = isolate->GetCurrentContext(); Local tpl = FunctionTemplate::New(isolate, New); diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index 6f33179b..b6983ddd 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -27,6 +27,7 @@ #include #include +#include "internal-field.hh" #include "map-get.hh" #include "per-isolate-data.hh" #include "translate-time-profile.hh" @@ -106,26 +107,6 @@ void SetContextPtr(ContextPtr& contextPtr, } } -inline void* GetAlignedPointerFromInternalField(Object* object, int index) { -#if NODE_MAJOR_VERSION >= 26 - return object->GetAlignedPointerFromInternalField( - index, kEmbedderDataTypeTagDefault); -#else - return object->GetAlignedPointerFromInternalField(index); -#endif -} - -inline void SetAlignedPointerInInternalField(Local object, - int index, - void* value) { -#if NODE_MAJOR_VERSION >= 26 - object->SetAlignedPointerInInternalField( - index, value, kEmbedderDataTypeTagDefault); -#else - object->SetAlignedPointerInInternalField(index, value); -#endif -} - // Deliberately not a node::ObjectWrap. That base registers a per-instance // environment cleanup hook in its constructor and calls // RemoveEnvironmentCleanupHook from its destructor, which CHECKs that the From 4597fb1b1671d782c70dc13bffcc9fa12d8ec7ce Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Tue, 11 Aug 2026 10:36:32 +0200 Subject: [PATCH 2/2] Add the zero-out-internal-field logic to PCP too --- bindings/profilers/wall.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index b6983ddd..fe85c876 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -701,10 +701,14 @@ WallProfiler::~WallProfiler() { // internal-field pointer in the wrap object stays inert even if V8 later // GCs the wrap.) auto* p = liveContextPtrHead_; + auto isolate = Isolate::GetCurrent(); while (p != nullptr) { auto* next = p->next_; p->pprev_ = nullptr; p->next_ = nullptr; + if (isolate != nullptr && !p->handle_.IsEmpty()) { + SetAlignedPointerInInternalField(p->handle_.Get(isolate), 0, nullptr); + } delete p; p = next; }