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

n-api: ensure scope present for finalization #33508

Closed
wants to merge 2 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented May 22, 2020

Spent some time looking at nodejs/node-addon-api#729 which was related to a crash during finalization when using node-addon-api ObjectWrap.

I think it should be fixed in core as opposed to node-addon-api as it should be possible to hit the same problem using N-API directly instead of node-addon-api.

It seems to be more subtle than the scope never being in place. The added is the simplest test that causes the problem to recreate when run under debug and without the change adding the scope.

Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson michael_dawson@ca.ibm.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 22, 2020
@mhdawson mhdawson changed the title napi: ensure scope present for finalization WIP: napi: ensure scope present for finalization May 22, 2020
@mhdawson mhdawson marked this pull request as draft May 22, 2020 02:09
@mhdawson mhdawson force-pushed the finalizer-scope branch 2 times, most recently from 9642cc5 to 6a62c2a Compare May 25, 2020 22:16
@mhdawson mhdawson changed the title WIP: napi: ensure scope present for finalization n-api: ensure scope present for finalization May 25, 2020
@mhdawson mhdawson marked this pull request as ready for review May 25, 2020 22:19
@mhdawson
Copy link
Member Author

@addaleax, @gabrielschulhof hoping you can confirm if you think this adds the scope in the right place. While investigating I checked that the status of the env when the assert is hit indicates that we can still call back to JS, which I'm assuming confirms it's ok to have a Scope/allocation.

I could see moving the scope out of the loop to reduce overhead, but left it in the loop as that seems safer in terms of memory use.

@gabrielschulhof
Copy link
Contributor

We could create a version of CallIntoModule and CallIntoModuleThrow that declares these scopes, because currently invocations of CallIntoModule and CallIntoModuleThrow are almost always preceded by scope declarations:

FinalizeBufferCallback precedes the call into the addon with a HandleScope and a ContextScope. @addaleax can we replace the ContextScope with a CallbackScope for consistency with other calls into the addon?

node/src/node_api.cc

Lines 57 to 65 in 9949a2e

v8::HandleScope handle_scope(finalizer->_env->isolate);
v8::Context::Scope context_scope(finalizer->_env->context());
finalizer->_env->CallIntoModuleThrow([&](napi_env env) {
finalizer->_finalize_callback(
env,
finalizer->_finalize_data,
finalizer->_finalize_hint);
});

In TSFN's DispatchOne we already precede the invocation of CallIntoModule with a HandleScope and a CallbackScope:

node/src/node_api.cc

Lines 303 to 313 in 9949a2e

v8::HandleScope scope(env->isolate);
CallbackScope cb_scope(this);
napi_value js_callback = nullptr;
if (!ref.IsEmpty()) {
v8::Local<v8::Function> js_cb =
v8::Local<v8::Function>::New(env->isolate, ref);
js_callback = v8impl::JsValueFromV8LocalValue(js_cb);
}
env->CallIntoModuleThrow([&](napi_env env) {
call_js_cb(env, js_callback, context, data);
});

The same is true in TSFN's Finalize:

node/src/node_api.cc

Lines 318 to 324 in 9949a2e

v8::HandleScope scope(env->isolate);
if (finalize_cb) {
CallbackScope cb_scope(this);
env->CallIntoModuleThrow([&](napi_env env) {
finalize_cb(env, finalize_data, context);
});
}

In napi_module_register_by_symbol we definitely don't need any scopes because we know the call is coming from JS:

node/src/node_api.cc

Lines 458 to 460 in 9949a2e

env->CallIntoModuleThrow([&](napi_env env) {
_exports = init(env, v8impl::JsValueFromV8LocalValue(exports));
});

In AfterThreadPoolWork we already precede the invocation with a declaration of HandleScope and CallbackScope.

node/src/node_api.cc

Lines 858 to 871 in 9949a2e

// Establish a handle scope here so that every callback doesn't have to.
// Also it is needed for the exception-handling below.
v8::HandleScope scope(_env->isolate);
CallbackScope callback_scope(this);
_env->CallIntoModule([&](napi_env env) {
_complete(env, ConvertUVErrorCode(status), _data);
}, [](napi_env env, v8::Local<v8::Value> local_err) {
// If there was an unhandled exception in the complete callback,
// report it as a fatal exception. (There is no JavaScript on the
// callstack that can possibly handle it.)
v8impl::trigger_fatal_exception(env, local_err);
});

This is the one you want to change. Would it be expensive if we declared both a HandleScope and a CallbackScope, thereby making it consistent with the other invocations where we need both?

_env->CallIntoModuleThrow([&](napi_env env) {
_finalize_callback(
env,
_finalize_data,
_finalize_hint);
});
}

In InvokeCallback we definitely don't need any scopes because we know the call is coming from JS:

env->CallIntoModuleThrow([&](napi_env env) {
result = cb(env, cbinfo_wrapper);
});

@gabrielschulhof
Copy link
Contributor

So, if we can agree in the two places that we can replace with/add a CallbackScope before the invocation, then we can add a method to napi_env called CallIntoModuleWithScopes that declares them and use that method everywhere.

@addaleax
Copy link
Member

@gabrielschulhof Just to be explicit here, CallbackScope is for asynchronous operations – Finalizers could definitely fall under that, yes.

@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label May 26, 2020
@gabrielschulhof
Copy link
Contributor

@mhdawson with #33570 in place it should be a lot easier to write the variant that declares scopes since we won't have as many variations on napi_env::CallIntoModule.

@mhdawson
Copy link
Member Author

@gabrielschulhof once #33570 lands, I'll update this PR to add CallIntoModuleWithScopes and use it in all places except for napi_module_register_by_symbol and validate that the new test fails without the scopes/passes with them.

@gabrielschulhof
Copy link
Contributor

@mhdawson it must also not be used in InvokeCallback because control is coming from JS there (last one on the list above).

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #33508 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #33508      +/-   ##
==========================================
- Coverage   96.77%   96.77%   -0.01%     
==========================================
  Files         202      201       -1     
  Lines       67129    66642     -487     
==========================================
- Hits        64966    64492     -474     
+ Misses       2163     2150      -13     
Impacted Files Coverage Δ
lib/internal/modules/esm/loader.js 88.65% <0.00%> (-0.99%) ⬇️
lib/internal/fs/streams.js 94.71% <0.00%> (-0.46%) ⬇️
lib/internal/streams/destroy.js 98.91% <0.00%> (-0.35%) ⬇️
lib/internal/util/inspect.js 96.15% <0.00%> (-0.10%) ⬇️
lib/_stream_writable.js 99.24% <0.00%> (-0.03%) ⬇️
lib/_stream_readable.js 98.57% <0.00%> (-0.02%) ⬇️
lib/internal/event_target.js
lib/_http_agent.js 99.11% <0.00%> (+<0.01%) ⬆️
lib/internal/errors.js 98.37% <0.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a1df3b...c5f3e91. Read the comment docs.

Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson
Copy link
Member Author

mhdawson commented May 29, 2020

@gabrielschulhof I spent some time looking at a common CallIntoModuleWithScopes, however, that required passing in the AsyncResource which is not available at the place I'm trying to fix and due to need for a HandleScope outside of the callInfoModule or not having a CallbackScope only 2 of the other CallIntoModule locations would have used the new method.

I think based only something like only 25% of the paths being able to use it, its not worth adding the new function and simply added the HandleScope to the path from which it was missing.

@mhdawson
Copy link
Member Author

Think this is now ready to go. Ran the new test with node_g and still passed after the latest change so should be good to go.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

LGTM

I nevertheless think we can streamline our use of scopes, but that's beyond the scope purview of this PR 🙂

@mhdawson
Copy link
Member Author

mhdawson commented Jun 8, 2020

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member Author

mhdawson commented Jun 9, 2020

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member Author

mhdawson commented Jun 9, 2020

New build since that seems to be what's needed: https://ci.nodejs.org/job/node-test-pull-request/31807/

@mhdawson
Copy link
Member Author

mhdawson commented Jun 9, 2020

Clean CI run landing.

@gabrielschulhof
Copy link
Contributor

The last build was yellow. I think this can land.

@mhdawson
Copy link
Member Author

mhdawson commented Jun 9, 2020

Landed in 362e4a1

mhdawson added a commit that referenced this pull request Jun 9, 2020
Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #33508
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson mhdawson closed this Jun 9, 2020
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Jun 9, 2020
Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs#33508
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #33508
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #33508
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Jul 8, 2020
Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs#33508
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #33508
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jul 13, 2020
codebytere pushed a commit that referenced this pull request Jul 14, 2020
Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #33508
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson mhdawson deleted the finalizer-scope branch September 14, 2020 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants