diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 873c8bdce4a492..439c9f7e0e267a 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -5468,6 +5468,42 @@ invocation. If it is deleted before then, then the finalize callback may never be invoked. Therefore, when obtaining a reference a finalize callback is also required in order to enable correct disposal of the reference. +#### `node_api_post_finalizer` + + + +> Stability: 1 - Experimental + +```c +napi_status node_api_post_finalizer(napi_env env, + napi_finalize finalize_cb, + void* finalize_data, + void* finalize_hint); +``` + +* `[in] env`: The environment that the API is invoked under. +* `[in] finalize_cb`: Native callback that will be used to free the + native data when the JavaScript object has been garbage-collected. + [`napi_finalize`][] provides more details. +* `[in] finalize_data`: Optional data to be passed to `finalize_cb`. +* `[in] finalize_hint`: Optional contextual hint that is passed to the + finalize callback. + +Returns `napi_ok` if the API succeeded. + +Schedules `napi_finalize` callback to be called asynchronously in the +event loop. + +This API must be called inside of a finalizer if it must call any code +that may affect the state of GC (garbage collector). + +The finalizers are called while GC collects objects. At that point of time +calling any API that may cause changes in GC state will cause unpredictable +behavior and crashes. The `node_api_post_finalizer` helps to work around +this limitation by running code outside of the GC finalization. + ## Simple asynchronous operations Addon modules often need to leverage async helpers from libuv as part of their diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 498aa0de2c13d2..08e4ef1593927d 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -62,14 +62,14 @@ void napi_env__::InvokeFinalizerFromGC(v8impl::RefTracker* finalizer) { EnqueueFinalizer(finalizer); } else { // The experimental code calls finalizers immediately to release native - // objects as soon as possible, but it suspends use of JS from finalizer. - // If JS calls are needed, then the finalizer code must call - // node_api_post_finalizer. + // objects as soon as possible. In that state any code that may affect GC + // state causes a fatal error. To work around this issue the finalizer code + // must call node_api_post_finalizer. if (last_error.error_code == napi_ok && last_exception.IsEmpty()) { - bool saved_suspend_call_into_js = suspend_call_into_js; - suspend_call_into_js = true; + auto restore_state = node::OnScopeLeave( + [this, saved = in_gc_finalizer] { in_gc_finalizer = saved; }); + in_gc_finalizer = true; finalizer->Finalize(); - suspend_call_into_js = saved_suspend_call_into_js; } else { // The finalizers can be run in the middle of JS or C++ code. // That code may be in an error state. In that case use the asynchronous @@ -93,6 +93,7 @@ napi_status NewString(napi_env env, CHECK_ARG(env, result); RETURN_STATUS_IF_FALSE( env, (length == NAPI_AUTO_LENGTH) || length <= INT_MAX, napi_invalid_arg); + env->CheckGCAccess(); auto isolate = env->isolate; auto str_maybe = string_maker(isolate); @@ -1539,6 +1540,7 @@ napi_status NAPI_CDECL napi_get_prototype(napi_env env, napi_status NAPI_CDECL napi_create_object(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue(v8::Object::New(env->isolate)); @@ -1548,6 +1550,7 @@ napi_status NAPI_CDECL napi_create_object(napi_env env, napi_value* result) { napi_status NAPI_CDECL napi_create_array(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue(v8::Array::New(env->isolate)); @@ -1559,6 +1562,7 @@ napi_status NAPI_CDECL napi_create_array_with_length(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue(v8::Array::New(env->isolate, length)); @@ -1659,6 +1663,7 @@ napi_status NAPI_CDECL napi_create_double(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue(v8::Number::New(env->isolate, value)); @@ -1671,6 +1676,7 @@ napi_status NAPI_CDECL napi_create_int32(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue(v8::Integer::New(env->isolate, value)); @@ -1683,6 +1689,7 @@ napi_status NAPI_CDECL napi_create_uint32(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue( v8::Integer::NewFromUnsigned(env->isolate, value)); @@ -1695,6 +1702,7 @@ napi_status NAPI_CDECL napi_create_int64(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue( v8::Number::New(env->isolate, static_cast(value))); @@ -1707,6 +1715,7 @@ napi_status NAPI_CDECL napi_create_bigint_int64(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue(v8::BigInt::New(env->isolate, value)); @@ -1719,6 +1728,7 @@ napi_status NAPI_CDECL napi_create_bigint_uint64(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); *result = v8impl::JsValueFromV8LocalValue( v8::BigInt::NewFromUnsigned(env->isolate, value)); @@ -1734,6 +1744,7 @@ napi_status NAPI_CDECL napi_create_bigint_words(napi_env env, NAPI_PREAMBLE(env); CHECK_ARG(env, words); CHECK_ARG(env, result); + env->CheckGCAccess(); v8::Local context = env->context(); @@ -1753,6 +1764,7 @@ napi_status NAPI_CDECL napi_get_boolean(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); v8::Isolate* isolate = env->isolate; @@ -1770,6 +1782,7 @@ napi_status NAPI_CDECL napi_create_symbol(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); v8::Isolate* isolate = env->isolate; @@ -1792,6 +1805,7 @@ napi_status NAPI_CDECL node_api_symbol_for(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, result); + env->CheckGCAccess(); napi_value js_description_string; STATUS_CALL(napi_create_string_utf8( @@ -1838,6 +1852,7 @@ napi_status NAPI_CDECL napi_create_error(napi_env env, CHECK_ENV(env); CHECK_ARG(env, msg); CHECK_ARG(env, result); + env->CheckGCAccess(); v8::Local message_value = v8impl::V8LocalValueFromJsValue(msg); RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected); @@ -1858,6 +1873,7 @@ napi_status NAPI_CDECL napi_create_type_error(napi_env env, CHECK_ENV(env); CHECK_ARG(env, msg); CHECK_ARG(env, result); + env->CheckGCAccess(); v8::Local message_value = v8impl::V8LocalValueFromJsValue(msg); RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected); @@ -1878,6 +1894,7 @@ napi_status NAPI_CDECL napi_create_range_error(napi_env env, CHECK_ENV(env); CHECK_ARG(env, msg); CHECK_ARG(env, result); + env->CheckGCAccess(); v8::Local message_value = v8impl::V8LocalValueFromJsValue(msg); RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected); @@ -1898,6 +1915,7 @@ napi_status NAPI_CDECL node_api_create_syntax_error(napi_env env, CHECK_ENV(env); CHECK_ARG(env, msg); CHECK_ARG(env, result); + env->CheckGCAccess(); v8::Local message_value = v8impl::V8LocalValueFromJsValue(msg); RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected); diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index ac9dc50f69c64f..3c58b84c749360 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -68,7 +68,7 @@ struct napi_env__ { if (--refs == 0) DeleteMe(); } - virtual bool can_call_into_js() const { return !suspend_call_into_js; } + virtual bool can_call_into_js() const { return true; } static inline void HandleThrow(napi_env env, v8::Local value) { if (env->terminatedOrTerminating()) { @@ -102,7 +102,6 @@ struct napi_env__ { // Call finalizer immediately. virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) { v8::HandleScope handle_scope(isolate); - v8::Context::Scope context_scope(context()); CallIntoModule([&](napi_env env) { cb(env, data, hint); }); } @@ -134,6 +133,19 @@ struct napi_env__ { delete this; } + void CheckGCAccess() { + if (module_api_version == NAPI_VERSION_EXPERIMENTAL && in_gc_finalizer) { + v8impl::OnFatalError( + nullptr, + "Finalizer is calling a function that may affect GC state.\n" + "The finalizers are run directly from GC and must not affect GC " + "state.\n" + "Use `node_api_post_finalizer` from inside of the finalizer to work " + "around this issue.\n" + "It schedules the call as a new task in the event loop."); + } + } + v8::Isolate* const isolate; // Shortcut for context()->GetIsolate() v8impl::Persistent context_persistent; @@ -152,7 +164,7 @@ struct napi_env__ { int refs = 1; void* instance_data = nullptr; int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; - bool suspend_call_into_js = false; + bool in_gc_finalizer = false; protected: // Should not be deleted directly. Delete with `napi_env__::DeleteMe()` @@ -218,6 +230,7 @@ inline napi_status napi_set_last_error(napi_env env, CHECK_ENV((env)); \ RETURN_STATUS_IF_FALSE( \ (env), (env)->last_exception.IsEmpty(), napi_pending_exception); \ + (env)->CheckGCAccess(); \ RETURN_STATUS_IF_FALSE((env), \ (env)->can_call_into_js(), \ (env->module_api_version == NAPI_VERSION_EXPERIMENTAL \ diff --git a/src/js_native_api_v8_internals.h b/src/js_native_api_v8_internals.h index 4f1b94d3d0c9d7..7bf3bf0fa7e1a2 100644 --- a/src/js_native_api_v8_internals.h +++ b/src/js_native_api_v8_internals.h @@ -17,6 +17,7 @@ #include "env.h" #include "gtest/gtest_prod.h" +#include "node_errors.h" #include "node_internals.h" #define NAPI_ARRAYSIZE(array) node::arraysize((array)) @@ -34,6 +35,11 @@ using Persistent = v8::Global; using PersistentToLocal = node::PersistentToLocal; +[[noreturn]] inline void OnFatalError(const char* location, + const char* message) { + node::OnFatalError(location, message); +} + } // end of namespace v8impl #endif // SRC_JS_NATIVE_API_V8_INTERNALS_H_ diff --git a/src/node_api.cc b/src/node_api.cc index ae92b9d8ee0af9..1f1cfb916a7f1e 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -33,7 +33,7 @@ void node_napi_env__::DeleteMe() { } bool node_napi_env__::can_call_into_js() const { - return Super::can_call_into_js() && node_env()->can_call_into_js(); + return node_env()->can_call_into_js(); } void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) { diff --git a/src/node_api_internals.h b/src/node_api_internals.h index d3b2b0c09b4c08..25f6b291902024 100644 --- a/src/node_api_internals.h +++ b/src/node_api_internals.h @@ -9,8 +9,6 @@ #include "util-inl.h" struct node_napi_env__ : public napi_env__ { - using Super = napi_env__; - node_napi_env__(v8::Local context, const std::string& module_filename, int32_t module_api_version); diff --git a/test/js-native-api/test_finalizer/test.js b/test/js-native-api/test_finalizer/test.js index 145a9723344082..3d4760cb48c860 100644 --- a/test/js-native-api/test_finalizer/test.js +++ b/test/js-native-api/test_finalizer/test.js @@ -5,15 +5,12 @@ const common = require('../../common'); const test_finalizer = require(`./build/${common.buildType}/test_finalizer`); const assert = require('assert'); -function addFinalizer() { - // Define obj in a function context to let it GC-collected. +(() => { const obj = {}; test_finalizer.addFinalizer(obj); -} - -addFinalizer(); +})(); -for (let i = 0; i < 1000; ++i) { +for (let i = 0; i < 10; ++i) { global.gc(); if (test_finalizer.getFinalizerCallCount() === 1) { break; @@ -28,7 +25,7 @@ async function runAsyncTests() { const obj = {}; test_finalizer.addFinalizerWithJS(obj, () => { js_is_called = true; }); })(); - await common.gcUntil('test JS finalizer', + await common.gcUntil('ensure JS finalizer called', () => (test_finalizer.getFinalizerCallCount() === 2)); assert(js_is_called); } diff --git a/test/js-native-api/test_finalizer/test_fatal_finalize.js b/test/js-native-api/test_finalizer/test_fatal_finalize.js new file mode 100644 index 00000000000000..80ab84385f94ec --- /dev/null +++ b/test/js-native-api/test_finalizer/test_fatal_finalize.js @@ -0,0 +1,31 @@ +'use strict'; + +if (process.argv[2] === 'child') { + const common = require('../../common'); + const test_finalizer = require(`./build/${common.buildType}/test_finalizer`); + + (() => { + const obj = {}; + test_finalizer.addFinalizerFailOnJS(obj); + })(); + + // Collect garbage 10 times. At least one of those should throw the exception + // and cause the whole process to bail with it, its text printed to stderr and + // asserted by the parent process to match expectations. + let gcCount = 10; + (function gcLoop() { + global.gc(); + if (--gcCount > 0) { + setImmediate(() => gcLoop()); + } + })(); + return; +} + +const assert = require('assert'); +const { spawnSync } = require('child_process'); +const child = spawnSync(process.execPath, [ + '--expose-gc', __filename, 'child', +]); +assert.strictEqual(child.signal, null); +assert.match(child.stderr.toString(), /Finalizer is calling a function that may affect GC state/); diff --git a/test/js-native-api/test_finalizer/test_finalizer.c b/test/js-native-api/test_finalizer/test_finalizer.c index bb7be613a49cb2..74de20254b81b3 100644 --- a/test/js-native-api/test_finalizer/test_finalizer.c +++ b/test/js-native-api/test_finalizer/test_finalizer.c @@ -22,9 +22,11 @@ static void finalizerCallingJSCallback(napi_env env, void* finalize_hint) { napi_value js_func, undefined; FinalizerData* data = (FinalizerData*)finalize_data; - NODE_API_CALL_RETURN_VOID(env, napi_get_reference_value(env, data->js_func, &js_func)); + NODE_API_CALL_RETURN_VOID( + env, napi_get_reference_value(env, data->js_func, &js_func)); NODE_API_CALL_RETURN_VOID(env, napi_get_undefined(env, &undefined)); - NODE_API_CALL_RETURN_VOID(env, napi_call_function(env, undefined, js_func, 0, NULL, NULL)); + NODE_API_CALL_RETURN_VOID( + env, napi_call_function(env, undefined, js_func, 0, NULL, NULL)); NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, data->js_func)); data->js_func = NULL; ++data->finalize_count; @@ -35,13 +37,24 @@ static void finalizerWithJSCallback(napi_env env, void* finalize_data, void* finalize_hint) { FinalizerData* data = (FinalizerData*)finalize_data; - NODE_API_CALL_RETURN_VOID(env, node_api_post_finalizer( - env, finalizerCallingJSCallback, finalize_data, finalize_hint)); + NODE_API_CALL_RETURN_VOID( + env, + node_api_post_finalizer( + env, finalizerCallingJSCallback, finalize_data, finalize_hint)); +} + +static void finalizerWithFailedJSCallback(napi_env env, + void* finalize_data, + void* finalize_hint) { + napi_value obj; + FinalizerData* data = (FinalizerData*)finalize_data; + ++data->finalize_count; + NODE_API_CALL_RETURN_VOID(env, napi_create_object(env, &obj)); } static napi_value addFinalizer(napi_env env, napi_callback_info info) { size_t argc = 1; - napi_value argv[1]; + napi_value argv[1] = {0}; FinalizerData* data; NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); @@ -52,7 +65,7 @@ static napi_value addFinalizer(napi_env env, napi_callback_info info) { return NULL; } -// This finalizer is going to call JavaScript from finalizer +// This finalizer is going to call JavaScript from finalizer and succeed. static napi_value addFinalizerWithJS(napi_env env, napi_callback_info info) { size_t argc = 2; napi_value argv[2] = {0}; @@ -71,6 +84,21 @@ static napi_value addFinalizerWithJS(napi_env env, napi_callback_info info) { return NULL; } +// This finalizer is going to call JavaScript from finalizer and fail. +static napi_value addFinalizerFailOnJS(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value argv[1] = {0}; + FinalizerData* data; + + NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); + NODE_API_CALL(env, napi_get_instance_data(env, &data)); + NODE_API_CALL( + env, + napi_add_finalizer( + env, argv[0], data, finalizerWithFailedJSCallback, NULL, NULL)); + return NULL; +} + static napi_value getFinalizerCallCount(napi_env env, napi_callback_info info) { size_t argc = 1; napi_value argv[1]; @@ -96,6 +124,7 @@ napi_value Init(napi_env env, napi_value exports) { napi_property_descriptor descriptors[] = { DECLARE_NODE_API_PROPERTY("addFinalizer", addFinalizer), DECLARE_NODE_API_PROPERTY("addFinalizerWithJS", addFinalizerWithJS), + DECLARE_NODE_API_PROPERTY("addFinalizerFailOnJS", addFinalizerFailOnJS), DECLARE_NODE_API_PROPERTY("getFinalizerCallCount", getFinalizerCallCount)};