Skip to content

src: let Environments on one isolate share a cleanup hook - #65777

Open
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:fix/embedder-cleanup-hook-registry
Open

src: let Environments on one isolate share a cleanup hook#65777
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:fix/embedder-cleanup-hook-registry

Conversation

@codebytere

@codebytere codebytere commented Sep 4, 2026

Copy link
Copy Markdown
Member

The process-global registry that #63985 put behind AddEnvironmentCleanupHook() / RemoveEnvironmentCleanupHook() is keyed on {isolate, fun, arg} with a CHECK that insertion succeeds, and the Environment is left out of the key on purpose. Two Environments that share an isolate and register the same hook abort on the second registration. The Node-API docs only forbid registering the same pair twice in one environment, and an addon calling napi_add_env_cleanup_hook(env, hook, NULL) from every environment it is loaded into is normal; Electron loads addons into several Environments on Blink's isolate (subframes, same-process child windows).

The registry is now a multimap keyed on arg whose entries carry the Environment. Adding the same hook twice to one Environment still aborts as documented. Removal prefers the current Environment's registration and otherwise takes the matching one from another Environment, so removing during GC without a current context keeps working, and it only looks at the current context when the isolate passed in is the calling thread's current one. Since the entry to drop after a hook has run can no longer be found by {isolate, fun, arg} alone, CleanupHookThunkRun() marks its entry as running and erases exactly that entry afterwards; removing a running entry (a hook removing itself, as ~ObjectWrap() does) is a no-op and the hook may register itself again, which keeps the use-after-free from #65630 fixed without caching the fields.

Tests: EnvironmentTest.SameCleanupHookInTwoEnvironmentsOnOneIsolate (aborted before), RemoveCleanupHookOfOtherEnvironmentOnSameIsolate (pins the cross-Environment fallback), CleanupHookRemovesItselfWhileRunning (kept as a guard for the #65630 case). test/addons, test/node-api and test/js-native-api pass.

Refs: #63985
Refs: #65630


Disclosure: the code, tests and this description were written by Claude Code, directed and reviewed by @codebytere.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 4, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codebytere
codebytere force-pushed the fix/embedder-cleanup-hook-registry branch from c16a038 to 61f728b Compare September 4, 2026 08:47
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.87097% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (ccd648e) to head (df3574c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/api/hooks.cc 83.87% 0 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65777      +/-   ##
==========================================
- Coverage   90.14%   90.13%   -0.01%     
==========================================
  Files         769      769              
  Lines      261626   261640      +14     
  Branches    49663    49674      +11     
==========================================
- Hits       235832   235826       -6     
- Misses      16805    16817      +12     
- Partials     8989     8997       +8     
Files with missing lines Coverage Δ
src/api/hooks.cc 87.42% <83.87%> (-0.78%) ⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

The registry behind `AddEnvironmentCleanupHook()` is keyed on
{isolate, fun, arg} and asserts that every insertion is unique. Two
Environments on one isolate that register the same hook, which the
Node-API documentation allows per environment, abort the process on
the second `napi_add_env_cleanup_hook()`.

Key the registry on `arg` only and tell entries apart by Environment:
adding the same hook to one Environment twice still aborts as
documented, and removal prefers the current Environment's registration,
falling back to a matching one from another Environment when there is
no current context. Because the entry to remove after a hook has run can
no longer be found by {isolate, fun, arg} alone, `CleanupHookThunkRun()`
marks its entry as running and erases exactly that entry afterwards; a
removal of a running entry (a hook removing itself, as `~ObjectWrap()`
does) is a no-op, which keeps the use-after-free fixed by nodejs#65630 fixed.

Refs: nodejs#63985
Refs: nodejs#65630
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
@codebytere codebytere changed the title src: fix cleanup hook registry for shared isolates and self-removal src: let Environments on one isolate share a cleanup hook Sep 4, 2026
@codebytere
codebytere force-pushed the fix/embedder-cleanup-hook-registry branch from 61f728b to df3574c Compare September 4, 2026 17:19
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants