Follow up on #388 review comments - #391
Open
szegedi wants to merge 1 commit into
Open
Conversation
szegedi
requested review from
IlyasShabi,
nsavoire and
r1viollet
as code owners
August 10, 2026 14:59
Overall package sizeSelf size: 2.48 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.1 | 504.33 kB | 504.33 kB | | source-map | 0.8.0 | 185.66 kB | 185.66 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
szegedi
added a commit
to szegedi/custom-labels
that referenced
this pull request
Aug 10, 2026
Ports two review nits raised on DataDog/pprof-nodejs#388, which vendors this file, and addressed there in DataDog/pprof-nodejs#391. The third comment on that PR was about deduplicating helpers against a file that has no counterpart here, so it does not apply. 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 — 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. That slot is exactly what the out-of-process OTEP-4947 reader walks to reach record_, so leaving it pointing at freed memory aims a dangling pointer 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 22, 24 and 26: 49/49 tests pass on each, and the teardown repro stays clean at N=3000.
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.
szegedi
force-pushed
the
szegedi/ctxwrap-review-followups
branch
from
August 10, 2026 15:18
c9f9fee to
8dfa9bb
Compare
nsavoire
reviewed
Aug 10, 2026
| // 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()) { |
| memcpy(&new_rec->attrs_data[current_used], appended.data(), appended.size()); | ||
| new_rec->attrs_data_size = static_cast<uint16_t>(new_used); | ||
| // The copy should've preserved valid=1 from the source record. | ||
| assert(new_rec->valid == 1); |
There was a problem hiding this comment.
AI flagged that this assert will fire in debug builds when doing invalidate() followed by large enough Append(...)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three review comments landed on #388 after it merged (@nsavoire). All three are valid; this addresses them.
1. Duplicated internal-field accessors
Correct, and it was deliberate but temporary: #388 introduced
internal-field.hhand leftwall.cc's copies alone to avoid conflicting with #387, which was in flight at the time, with a note to fold them in afterwards. #387 has landed, sowall.ccnow includes the header and its copies are gone. Same namespace and same names, so every call site is untouched.2. Register the hook in
Init(), dropg_drain_hook_registeredBetter, and it removes state rather than adding it. Module initialisation always runs with a context entered, so
AddEnvironmentCleanupHook's own CHECK is satisfied there just as it was inWrap(), andInit()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 the second case by construction, so both reasons evaporate.3. Clear the internal field before deleting the wrap
Applied, and I'd argue it is more than a nit. That slot is exactly what the out-of-process OTEP-4947 reader walks to reach
record_, so leaving it pointing at freed memory is a loaded gun aimed at a consumer we do not control. #388's comment claimed the dangling pointer "stays inert because nothing reads it" — true of our own code, but not of the reader.The drain hook now nulls slot 0 on its way through the list. Being on the live list means V8 has not collected the holder, so reading the handle there is safe. The
WeakCallbackpath cannot do this and does not need to: there the holder is the object being collected.Verification
npm testtest:js-asantest:js-asannative_wrap_fields_offsetLocal suite 115 passing,
gts check0 errors,clang-format -Werrorclean.The zero leak counts remain the load-bearing check: they confirm the drain hook still does what
node::ObjectWrap's per-instance hook used to, now that it is registered from a different place.