n-api: ensure scope present for finalization

[mhdawson]

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 [email protected]

Checklist

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

[mhdawson]

@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]

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?

node/src/js_native_api_v8.cc

Lines 270 to 276 in 9949a2e

	_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:

node/src/js_native_api_v8.cc

Lines 478 to 480 in 9949a2e

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

[gabrielschulhof]

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]

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

[gabrielschulhof]

@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]

@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]

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

[codecov-commenter]

Codecov Report

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

@@            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.

[mhdawson]

@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]

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.

[gabrielschulhof]

LGTM

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

[mhdawson]

Resume build: https://ci.nodejs.org/job/node-test-pull-request/31772/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31778/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31804/

[mhdawson]

Resume build: https://ci.nodejs.org/job/node-test-pull-request/31804/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31807/

[mhdawson]

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

[mhdawson]

Clean CI run landing.

[gabrielschulhof]

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

[mhdawson]

Landed in 362e4a1