Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid creating a Napi handle scope within a finalizer #13870

Merged
merged 10 commits into from
Sep 12, 2024

Conversation

Jarred-Sumner
Copy link
Collaborator

What does this PR do?

This makes napi handle scopes optional, and we ignore them when they're not set.

Fixes https://discord.com/channels/876711213126520882/1282959007589601280/1282959007589601280

This mitigates a crash where a napi package creates a handle scope within a finalizer. Any object allocation within that finalizer would still fail, but the act of creating a handle scope at all inside a finalizer fails since it is an allocation.

How did you verify your code works?

I verified the crash is fixed manually.

This needs a test. The test would do something like:

  • Create a napi ref wrapping an object
  • The napi ref's finalizer function should open a handle scope
  • Allocate repeatedly until the napi ref's finalizer is called

@Jarred-Sumner Jarred-Sumner requested a review from 190n September 11, 2024 01:47
@190n
Copy link
Contributor

190n commented Sep 11, 2024

It looks like nodejs/node-addon-api#1409 will (once it lands in a release) also fix this, as they now intentionally defer the execution of finalizers instead of relying on Node.js to do it:

#if (defined(NAPI_EXPERIMENTAL) &&                                             \
     defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER))
template <napi_finalize finalizer>
inline void PostFinalizerWrapper(node_api_nogc_env nogc_env,
                                 void* data,
                                 void* hint) {
  napi_status status = node_api_post_finalizer(nogc_env, finalizer, data, hint);
  NAPI_FATAL_IF_FAILED(
      status, "PostFinalizerWrapper", "node_api_post_finalizer failed");
}
#else
template <napi_finalize finalizer>
inline void PostFinalizerWrapper(napi_env env, void* data, void* hint) {
  finalizer(env, data, hint);
}
#endif

But this solution will do for now. I'll write a test -- I might see if we can pass a JS callback into the NAPI functions that calls Bun.gc instead of trying to trigger it via a bunch of allocations, as that will make the other GC tests faster too. But I'll have to make sure that that does actually reproduce GC bugs.

Copy link
Contributor

@190n 190n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a test

@190n 190n self-requested a review September 11, 2024 23:51
Copy link
Contributor

@190n 190n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tests crash node

@Jarred-Sumner Jarred-Sumner merged commit c489970 into main Sep 12, 2024
45 of 46 checks passed
@Jarred-Sumner Jarred-Sumner deleted the jarred/v8-handle branch September 12, 2024 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants