Skip to content

Commit f66e514

Browse files
authored
fix(wall): don't derive PersistentContextPtr from node::ObjectWrap (#385)
* fix(wall): don't derive PersistentContextPtr from node::ObjectWrap node::ObjectWrap registers a per-instance environment cleanup hook in its constructor and calls RemoveEnvironmentCleanupHook from its destructor. That teardown path CHECKs the Environment is still alive: node[650]: void node::RemoveEnvironmentCleanupHook( v8::Isolate*, CleanupHook, void*) at ../src/api/hooks.cc:142 Assertion failed: (env) != nullptr 3: node::RemoveEnvironmentCleanupHook(...) 4: node::ObjectWrap::RemoveCleanupHook() 5: node::ObjectWrap::~ObjectWrap() 6: dd::PersistentContextPtr::~PersistentContextPtr() 8: node::ObjectWrap::WeakCallback(...) A PCP is owned by a weak V8 handle, so V8 decides when it dies — and V8 runs weak callbacks during isolate teardown, after the Environment is gone. The CHECK then aborts the process with SIGABRT. The wrapper only ever needed two things from the base class: the internal-field pointer that GetContextPtrSignalSafe reads, and a weak handle to hang the object's lifetime on. Neither needs a cleanup hook — ~WallProfiler already walks the live list and deletes any PCP V8 has not collected, which is what keeps LSAN quiet at exit. So hold the weak Persistent directly and drop the base class. ~PersistentContextPtr resets the handle, which cancels the weak callback when ~WallProfiler is the one doing the deleting and is a no-op when we arrived from the callback itself. Reproduced on main under ASAN (which perturbs GC timing enough to make it deterministic) as an abort during teardown after the Time Profiler tests; the full ASAN suite goes from exit 134 to 158 passing with no leaks reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(wall): tag the internal-field store for Node 26 Node 26 requires an EmbedderDataTypeTag on Object::SetAlignedPointerInInternalField: error: no matching function for call to 'v8::Object::SetAlignedPointerInInternalField(int, dd::PersistentContextPtr*)' note: candidate: 'void v8::Object::SetAlignedPointerInInternalField( int, void*, v8::EmbedderDataTypeTag)' note: candidate expects 3 arguments, 2 provided node::ObjectWrap::Wrap hid this: its header handles the tag internally, so taking over the store exposed the version difference. Add the setter counterpart to the existing GetAlignedPointerFromInternalField helper and use it, so both ends of the internal-field access agree on kEmbedderDataTypeTagDefault. Verified on Node 20, 24 and 26 (the last is where AsyncContextFrame is on by default, so it actually exercises the PCP path): builds clean, 158 passing, ASAN exit 0 with no leaks or aborts.
1 parent aac1e50 commit f66e514

1 file changed

Lines changed: 58 additions & 15 deletions

File tree

bindings/profilers/wall.cc

Lines changed: 58 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,41 @@ void SetContextPtr(ContextPtr& contextPtr,
106106
}
107107
}
108108

109-
class PersistentContextPtr : public node::ObjectWrap {
109+
inline void* GetAlignedPointerFromInternalField(Object* object, int index) {
110+
#if NODE_MAJOR_VERSION >= 26
111+
return object->GetAlignedPointerFromInternalField(
112+
index, kEmbedderDataTypeTagDefault);
113+
#else
114+
return object->GetAlignedPointerFromInternalField(index);
115+
#endif
116+
}
117+
118+
inline void SetAlignedPointerInInternalField(Local<Object> object,
119+
int index,
120+
void* value) {
121+
#if NODE_MAJOR_VERSION >= 26
122+
object->SetAlignedPointerInInternalField(
123+
index, value, kEmbedderDataTypeTagDefault);
124+
#else
125+
object->SetAlignedPointerInInternalField(index, value);
126+
#endif
127+
}
128+
129+
// Deliberately not a node::ObjectWrap. That base registers a per-instance
130+
// environment cleanup hook in its constructor and calls
131+
// RemoveEnvironmentCleanupHook from its destructor, which CHECKs that the
132+
// Environment is still alive:
133+
//
134+
// node[650]: void node::RemoveEnvironmentCleanupHook(...) hooks.cc:142
135+
// Assertion failed: (env) != nullptr
136+
//
137+
// A PCP is owned by a weak V8 handle, and V8 runs weak callbacks during
138+
// isolate teardown — after the Environment has been torn down — so that CHECK
139+
// fires and aborts the process. All we need from a wrapper is the
140+
// internal-field pointer and the weak handle, and ~WallProfiler already
141+
// deletes whatever is still on the live list, so the cleanup hook the base
142+
// class installs has nothing left to do.
143+
class PersistentContextPtr {
110144
ContextPtr context;
111145
// Back-pointer to the WallProfiler that created this PCP. Guaranteed to be
112146
// a valid pointer whenever pprev_ != nullptr — ~WallProfiler nulls pprev_
@@ -125,8 +159,17 @@ class PersistentContextPtr : public node::ObjectWrap {
125159
PersistentContextPtr** pprev_ = nullptr;
126160
PersistentContextPtr* next_ = nullptr;
127161

162+
// Weak handle on the holder object. Owns this PCP: when V8 collects the
163+
// holder, WeakCallback deletes us.
164+
v8::Persistent<v8::Object> handle_;
165+
128166
friend class WallProfiler;
129167

168+
static void WeakCallback(
169+
const v8::WeakCallbackInfo<PersistentContextPtr>& data) {
170+
delete data.GetParameter();
171+
}
172+
130173
public:
131174
PersistentContextPtr(WallProfiler* profiler, Local<Object> wrap);
132175

@@ -139,14 +182,18 @@ class PersistentContextPtr : public node::ObjectWrap {
139182
ContextPtr Get() const { return context; }
140183

141184
static PersistentContextPtr* Unwrap(Local<Object> wrap) {
142-
return node::ObjectWrap::Unwrap<PersistentContextPtr>(wrap);
185+
return static_cast<PersistentContextPtr*>(
186+
GetAlignedPointerFromInternalField(*wrap, 0));
143187
}
144188
};
145189

146190
PersistentContextPtr::PersistentContextPtr(WallProfiler* profiler,
147191
Local<Object> wrap)
148192
: profiler_(profiler) {
149-
Wrap(wrap);
193+
auto* isolate = Isolate::GetCurrent();
194+
SetAlignedPointerInInternalField(wrap, 0, this);
195+
handle_.Reset(isolate, wrap);
196+
handle_.SetWeak(this, &WeakCallback, v8::WeakCallbackType::kParameter);
150197
// Splice ourselves at the head of profiler's live list.
151198
auto** headSlot = profiler->liveContextPtrHeadSlot();
152199
next_ = *headSlot;
@@ -167,15 +214,11 @@ PersistentContextPtr::~PersistentContextPtr() {
167214
if (next_ != nullptr) next_->pprev_ = pprev_;
168215
profiler_->recordContextRelease();
169216
}
170-
}
171-
172-
inline void* GetAlignedPointerFromInternalField(Object* object, int index) {
173-
#if NODE_MAJOR_VERSION >= 26
174-
return object->GetAlignedPointerFromInternalField(
175-
index, kEmbedderDataTypeTagDefault);
176-
#else
177-
return object->GetAlignedPointerFromInternalField(index);
178-
#endif
217+
// Cancels the weak callback when we're deleted by ~WallProfiler rather than
218+
// by V8; a no-op when we got here from WeakCallback itself. The holder
219+
// object's internal field is left dangling either way, but nothing reads it
220+
// once the owning profiler is gone.
221+
handle_.Reset();
179222
}
180223

181224
// Maximum number of rounds in the GetV8ToEpochOffset
@@ -678,9 +721,9 @@ WallProfiler::~WallProfiler() {
678721
// Delete every PCP still live in the CPED map. ~PCP would normally unlink
679722
// itself via pprev_/next_, but we're tearing down the list we point into —
680723
// so null pprev_ first to signal "already detached" and let ~PCP skip the
681-
// unlink. (~ObjectWrap will still clear V8's weak callback during delete,
682-
// so the dangling internal-field pointer in the wrap object stays inert
683-
// even if V8 later GCs the wrap.)
724+
// unlink. (~PCP still resets its weak handle during delete, so the dangling
725+
// internal-field pointer in the wrap object stays inert even if V8 later
726+
// GCs the wrap.)
684727
auto* p = liveContextPtrHead_;
685728
while (p != nullptr) {
686729
auto* next = p->next_;

0 commit comments

Comments
 (0)