From db2a07fcd69c9c2e87a60d3a613b1f6706fd0d95 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 22 Jun 2023 19:39:22 -0700 Subject: [PATCH] node-api: run finalizers directly from GC PR-URL: https://github.com/nodejs/node/pull/42651 Reviewed-By: Gabriel Schulhof Reviewed-By: Michael Dawson --- doc/api/n-api.md | 36 ++++ src/js_native_api.h | 10 + src/js_native_api_v8.cc | 184 ++++++++++++++---- src/js_native_api_v8.h | 43 +++- src/js_native_api_v8_internals.h | 6 + src/node_api.cc | 7 + test/js-native-api/test_finalizer/binding.gyp | 11 ++ test/js-native-api/test_finalizer/test.js | 43 ++++ .../test_finalizer/test_fatal_finalize.js | 31 +++ .../test_finalizer/test_finalizer.c | 146 ++++++++++++++ 10 files changed, 477 insertions(+), 40 deletions(-) create mode 100644 test/js-native-api/test_finalizer/binding.gyp create mode 100644 test/js-native-api/test_finalizer/test.js create mode 100644 test/js-native-api/test_finalizer/test_fatal_finalize.js create mode 100644 test/js-native-api/test_finalizer/test_finalizer.c diff --git a/doc/api/n-api.md b/doc/api/n-api.md index feb9fa6061ca4e..5a01035acd9c8f 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -5424,6 +5424,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 a `napi_finalize` callback to be called asynchronously in the +event loop. + +Normally, finalizers are called while the GC (garbage collector) collects +objects. At that point calling any Node-API that may cause changes in the GC +state will be disabled and will crash Node.js. + +`node_api_post_finalizer` helps to work around this limitation by allowing the +add-on to defer calls to such Node-APIs to a point in time 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.h b/src/js_native_api.h index 3aa0ee5c1c5c5c..eb3c139a9deb41 100644 --- a/src/js_native_api.h +++ b/src/js_native_api.h @@ -517,6 +517,16 @@ NAPI_EXTERN napi_status NAPI_CDECL napi_add_finalizer(napi_env env, #endif // NAPI_VERSION >= 5 +#ifdef NAPI_EXPERIMENTAL + +NAPI_EXTERN napi_status NAPI_CDECL +node_api_post_finalizer(napi_env env, + napi_finalize finalize_cb, + void* finalize_data, + void* finalize_hint); + +#endif // NAPI_EXPERIMENTAL + #if NAPI_VERSION >= 6 // BigInt diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 92f765eb0a99f9..f08294a26a4392 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -57,8 +57,22 @@ (out) = v8::type::New((buffer), (byte_offset), (length)); \ } while (0) -namespace v8impl { +void napi_env__::InvokeFinalizerFromGC(v8impl::RefTracker* finalizer) { + if (module_api_version != NAPI_VERSION_EXPERIMENTAL) { + EnqueueFinalizer(finalizer); + } else { + // The experimental code calls finalizers immediately to release native + // 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 + // can call node_api_post_finalizer. + auto restore_state = node::OnScopeLeave( + [this, saved = in_gc_finalizer] { in_gc_finalizer = saved; }); + in_gc_finalizer = true; + finalizer->Finalize(); + } +} +namespace v8impl { namespace { template @@ -68,6 +82,7 @@ napi_status NewString(napi_env env, napi_value* result, StringMaker string_maker) { CHECK_ENV(env); + env->CheckGCAccess(); if (length > 0) CHECK_ARG(env, str); CHECK_ARG(env, result); RETURN_STATUS_IF_FALSE( @@ -92,6 +107,7 @@ napi_status NewExternalString(napi_env env, StringMaker string_maker) { napi_status status; #if defined(V8_ENABLE_SANDBOX) + env->CheckGCAccess(); status = create_api(env, str, length, result); if (status == napi_ok) { if (copied != nullptr) { @@ -604,28 +620,70 @@ void Finalizer::ResetFinalizer() { finalize_hint_ = nullptr; } -// Wrapper around v8impl::Persistent that implements reference counting. -RefBase::RefBase(napi_env env, - uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) +TrackedFinalizer::TrackedFinalizer(napi_env env, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) : Finalizer(env, finalize_callback, finalize_data, finalize_hint), - refcount_(initial_refcount), - ownership_(ownership) { + RefTracker() { Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist); } -// When a RefBase is being deleted, it may have been queued to call its +TrackedFinalizer* TrackedFinalizer::New(napi_env env, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) { + return new TrackedFinalizer( + env, finalize_callback, finalize_data, finalize_hint); +} + +// When a TrackedFinalizer is being deleted, it may have been queued to call its // finalizer. -RefBase::~RefBase() { +TrackedFinalizer::~TrackedFinalizer() { // Remove from the env's tracked list. Unlink(); // Try to remove the finalizer from the scheduled second pass callback. env_->DequeueFinalizer(this); } +void TrackedFinalizer::Finalize() { + FinalizeCore(/*deleteMe:*/ true); +} + +void TrackedFinalizer::FinalizeCore(bool deleteMe) { + // Swap out the field finalize_callback so that it can not be accidentally + // called more than once. + napi_finalize finalize_callback = finalize_callback_; + void* finalize_data = finalize_data_; + void* finalize_hint = finalize_hint_; + ResetFinalizer(); + + // Either the RefBase is going to be deleted in the finalize_callback or not, + // it should be removed from the tracked list. + Unlink(); + // If the finalize_callback is present, it should either delete the + // derived RefBase, or the RefBase ownership was set to Ownership::kRuntime + // and the deleteMe parameter is true. + if (finalize_callback != nullptr) { + env_->CallFinalizer(finalize_callback, finalize_data, finalize_hint); + } + + if (deleteMe) { + delete this; + } +} + +// Wrapper around v8impl::Persistent that implements reference counting. +RefBase::RefBase(napi_env env, + uint32_t initial_refcount, + Ownership ownership, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) + : TrackedFinalizer(env, finalize_callback, finalize_data, finalize_hint), + refcount_(initial_refcount), + ownership_(ownership) {} + RefBase* RefBase::New(napi_env env, uint32_t initial_refcount, Ownership ownership, @@ -660,31 +718,9 @@ uint32_t RefBase::RefCount() { } void RefBase::Finalize() { - Ownership ownership = ownership_; - // Swap out the field finalize_callback so that it can not be accidentally - // called more than once. - napi_finalize finalize_callback = finalize_callback_; - void* finalize_data = finalize_data_; - void* finalize_hint = finalize_hint_; - ResetFinalizer(); - - // Either the RefBase is going to be deleted in the finalize_callback or not, - // it should be removed from the tracked list. - Unlink(); - // 1. If the finalize_callback is present, it should either delete the - // RefBase, or set ownership with Ownership::kRuntime. - // 2. If the finalizer is not present, the RefBase can be deleted after the - // call. - if (finalize_callback != nullptr) { - env_->CallFinalizer(finalize_callback, finalize_data, finalize_hint); - // No access to `this` after finalize_callback is called. - } - // If the RefBase is not Ownership::kRuntime, userland code should delete it. - // Now delete it if it is Ownership::kRuntime. - if (ownership == Ownership::kRuntime) { - delete this; - } + // Delete it if it is Ownership::kRuntime. + FinalizeCore(/*deleteMe:*/ ownership_ == Ownership::kRuntime); } template @@ -779,7 +815,7 @@ void Reference::WeakCallback(const v8::WeakCallbackInfo& data) { Reference* reference = data.GetParameter(); // The reference must be reset during the weak callback as the API protocol. reference->persistent_.Reset(); - reference->env_->EnqueueFinalizer(reference); + reference->env_->InvokeFinalizerFromGC(reference); } } // end of namespace v8impl @@ -1436,6 +1472,7 @@ napi_status NAPI_CDECL napi_is_array(napi_env env, napi_value value, bool* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -1495,6 +1532,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); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Object::New(env->isolate)); @@ -1504,6 +1542,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); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Array::New(env->isolate)); @@ -1515,6 +1554,7 @@ napi_status NAPI_CDECL napi_create_array_with_length(napi_env env, size_t length, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = @@ -1615,6 +1655,7 @@ napi_status NAPI_CDECL napi_create_double(napi_env env, double value, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = @@ -1627,6 +1668,7 @@ napi_status NAPI_CDECL napi_create_int32(napi_env env, int32_t value, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = @@ -1639,6 +1681,7 @@ napi_status NAPI_CDECL napi_create_uint32(napi_env env, uint32_t value, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue( @@ -1651,6 +1694,7 @@ napi_status NAPI_CDECL napi_create_int64(napi_env env, int64_t value, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue( @@ -1663,6 +1707,7 @@ napi_status NAPI_CDECL napi_create_bigint_int64(napi_env env, int64_t value, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = @@ -1675,6 +1720,7 @@ napi_status NAPI_CDECL napi_create_bigint_uint64(napi_env env, uint64_t value, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue( @@ -1689,6 +1735,7 @@ napi_status NAPI_CDECL napi_create_bigint_words(napi_env env, const uint64_t* words, napi_value* result) { NAPI_PREAMBLE(env); + env->CheckGCAccess(); CHECK_ARG(env, words); CHECK_ARG(env, result); @@ -1709,6 +1756,7 @@ napi_status NAPI_CDECL napi_get_boolean(napi_env env, bool value, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); v8::Isolate* isolate = env->isolate; @@ -1726,6 +1774,7 @@ napi_status NAPI_CDECL napi_create_symbol(napi_env env, napi_value description, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); v8::Isolate* isolate = env->isolate; @@ -1748,6 +1797,7 @@ napi_status NAPI_CDECL node_api_symbol_for(napi_env env, size_t length, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); napi_value js_description_string; @@ -1793,6 +1843,7 @@ napi_status NAPI_CDECL napi_create_error(napi_env env, napi_value msg, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, msg); CHECK_ARG(env, result); @@ -1813,6 +1864,7 @@ napi_status NAPI_CDECL napi_create_type_error(napi_env env, napi_value msg, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, msg); CHECK_ARG(env, result); @@ -1833,6 +1885,7 @@ napi_status NAPI_CDECL napi_create_range_error(napi_env env, napi_value msg, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, msg); CHECK_ARG(env, result); @@ -1853,6 +1906,7 @@ napi_status NAPI_CDECL node_api_create_syntax_error(napi_env env, napi_value msg, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, msg); CHECK_ARG(env, result); @@ -1874,6 +1928,7 @@ napi_status NAPI_CDECL napi_typeof(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -1913,6 +1968,7 @@ napi_status NAPI_CDECL napi_typeof(napi_env env, napi_status NAPI_CDECL napi_get_undefined(napi_env env, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Undefined(env->isolate)); @@ -1922,6 +1978,7 @@ napi_status NAPI_CDECL napi_get_undefined(napi_env env, napi_value* result) { napi_status NAPI_CDECL napi_get_null(napi_env env, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Null(env->isolate)); @@ -1967,6 +2024,7 @@ napi_status NAPI_CDECL napi_get_new_target(napi_env env, CHECK_ENV(env); CHECK_ARG(env, cbinfo); CHECK_ARG(env, result); + env->CheckGCAccess(); v8impl::CallbackWrapper* info = reinterpret_cast(cbinfo); @@ -2013,6 +2071,7 @@ napi_status NAPI_CDECL napi_call_function(napi_env env, napi_status NAPI_CDECL napi_get_global(napi_env env, napi_value* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(env->context()->Global()); @@ -2110,6 +2169,7 @@ napi_status NAPI_CDECL napi_is_error(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot // throw JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2125,6 +2185,7 @@ napi_status NAPI_CDECL napi_get_value_double(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2142,6 +2203,7 @@ napi_status NAPI_CDECL napi_get_value_int32(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2166,6 +2228,7 @@ napi_status NAPI_CDECL napi_get_value_uint32(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2190,6 +2253,7 @@ napi_status NAPI_CDECL napi_get_value_int64(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2223,6 +2287,7 @@ napi_status NAPI_CDECL napi_get_value_bigint_int64(napi_env env, int64_t* result, bool* lossless) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); CHECK_ARG(env, lossless); @@ -2241,6 +2306,7 @@ napi_status NAPI_CDECL napi_get_value_bigint_uint64(napi_env env, uint64_t* result, bool* lossless) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); CHECK_ARG(env, lossless); @@ -2260,6 +2326,7 @@ napi_status NAPI_CDECL napi_get_value_bigint_words(napi_env env, size_t* word_count, uint64_t* words) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, word_count); @@ -2290,6 +2357,7 @@ napi_status NAPI_CDECL napi_get_value_bool(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2312,6 +2380,7 @@ napi_status NAPI_CDECL napi_get_value_bool(napi_env env, napi_status NAPI_CDECL napi_get_value_string_latin1( napi_env env, napi_value value, char* buf, size_t bufsize, size_t* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); v8::Local val = v8impl::V8LocalValueFromJsValue(value); @@ -2350,6 +2419,7 @@ napi_status NAPI_CDECL napi_get_value_string_latin1( napi_status NAPI_CDECL napi_get_value_string_utf8( napi_env env, napi_value value, char* buf, size_t bufsize, size_t* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); v8::Local val = v8impl::V8LocalValueFromJsValue(value); @@ -2391,6 +2461,7 @@ napi_status NAPI_CDECL napi_get_value_string_utf16(napi_env env, size_t bufsize, size_t* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); v8::Local val = v8impl::V8LocalValueFromJsValue(value); @@ -2577,6 +2648,7 @@ napi_status NAPI_CDECL napi_get_value_external(napi_env env, napi_value value, void** result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2597,6 +2669,7 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2621,6 +2694,7 @@ napi_status NAPI_CDECL napi_delete_reference(napi_env env, napi_ref ref) { // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, ref); delete reinterpret_cast(ref); @@ -2639,6 +2713,7 @@ napi_status NAPI_CDECL napi_reference_ref(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, ref); v8impl::Reference* reference = reinterpret_cast(ref); @@ -2661,6 +2736,7 @@ napi_status NAPI_CDECL napi_reference_unref(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, ref); v8impl::Reference* reference = reinterpret_cast(ref); @@ -2687,6 +2763,7 @@ napi_status NAPI_CDECL napi_get_reference_value(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, ref); CHECK_ARG(env, result); @@ -2701,6 +2778,7 @@ napi_status NAPI_CDECL napi_open_handle_scope(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsHandleScopeFromV8HandleScope( @@ -2714,6 +2792,7 @@ napi_status NAPI_CDECL napi_close_handle_scope(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, scope); if (env->open_handle_scopes == 0) { return napi_handle_scope_mismatch; @@ -2729,6 +2808,7 @@ napi_status NAPI_CDECL napi_open_escapable_handle_scope( // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = v8impl::JsEscapableHandleScopeFromV8EscapableHandleScope( @@ -2742,6 +2822,7 @@ napi_status NAPI_CDECL napi_close_escapable_handle_scope( // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, scope); if (env->open_handle_scopes == 0) { return napi_handle_scope_mismatch; @@ -2759,6 +2840,7 @@ napi_status NAPI_CDECL napi_escape_handle(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, scope); CHECK_ARG(env, escapee); CHECK_ARG(env, result); @@ -2837,6 +2919,7 @@ napi_status NAPI_CDECL napi_is_exception_pending(napi_env env, bool* result) { // NAPI_PREAMBLE is not used here: this function must execute when there is a // pending exception. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); *result = !env->last_exception.IsEmpty(); @@ -2848,6 +2931,7 @@ napi_status NAPI_CDECL napi_get_and_clear_last_exception(napi_env env, // NAPI_PREAMBLE is not used here: this function must execute when there is a // pending exception. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, result); if (env->last_exception.IsEmpty()) { @@ -2865,6 +2949,7 @@ napi_status NAPI_CDECL napi_is_arraybuffer(napi_env env, napi_value value, bool* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2917,6 +3002,7 @@ napi_status NAPI_CDECL napi_get_arraybuffer_info(napi_env env, void** data, size_t* byte_length) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, arraybuffer); v8::Local value = v8impl::V8LocalValueFromJsValue(arraybuffer); @@ -2939,6 +3025,7 @@ napi_status NAPI_CDECL napi_is_typedarray(napi_env env, napi_value value, bool* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -3025,6 +3112,7 @@ napi_status NAPI_CDECL napi_get_typedarray_info(napi_env env, napi_value* arraybuffer, size_t* byte_offset) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, typedarray); v8::Local value = v8impl::V8LocalValueFromJsValue(typedarray); @@ -3115,6 +3203,7 @@ napi_status NAPI_CDECL napi_is_dataview(napi_env env, napi_value value, bool* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -3131,6 +3220,7 @@ napi_status NAPI_CDECL napi_get_dataview_info(napi_env env, napi_value* arraybuffer, size_t* byte_offset) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, dataview); v8::Local value = v8impl::V8LocalValueFromJsValue(dataview); @@ -3206,6 +3296,7 @@ napi_status NAPI_CDECL napi_is_promise(napi_env env, napi_value value, bool* is_promise) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, is_promise); @@ -3232,6 +3323,7 @@ napi_status NAPI_CDECL napi_is_date(napi_env env, napi_value value, bool* is_date) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, is_date); @@ -3244,6 +3336,7 @@ napi_status NAPI_CDECL napi_get_date_value(napi_env env, napi_value value, double* result) { NAPI_PREAMBLE(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -3290,6 +3383,7 @@ napi_status NAPI_CDECL napi_add_finalizer(napi_env env, // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw // JS exceptions. CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, js_object); CHECK_ARG(env, finalize_cb); @@ -3310,6 +3404,20 @@ napi_status NAPI_CDECL napi_add_finalizer(napi_env env, return napi_clear_last_error(env); } +#ifdef NAPI_EXPERIMENTAL + +napi_status NAPI_CDECL node_api_post_finalizer(napi_env env, + napi_finalize finalize_cb, + void* finalize_data, + void* finalize_hint) { + CHECK_ENV(env); + env->EnqueueFinalizer(v8impl::TrackedFinalizer::New( + env, finalize_cb, finalize_data, finalize_hint)); + return napi_clear_last_error(env); +} + +#endif + napi_status NAPI_CDECL napi_adjust_external_memory(napi_env env, int64_t change_in_bytes, int64_t* adjusted_value) { @@ -3355,6 +3463,7 @@ napi_status NAPI_CDECL napi_get_instance_data(napi_env env, void** data) { napi_status NAPI_CDECL napi_detach_arraybuffer(napi_env env, napi_value arraybuffer) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, arraybuffer); v8::Local value = v8impl::V8LocalValueFromJsValue(arraybuffer); @@ -3374,6 +3483,7 @@ napi_status NAPI_CDECL napi_is_detached_arraybuffer(napi_env env, napi_value arraybuffer, bool* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, arraybuffer); CHECK_ARG(env, result); diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 1ac738af2adb88..db39686c91127e 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -105,6 +105,9 @@ struct napi_env__ { CallIntoModule([&](napi_env env) { cb(env, data, hint); }); } + // Invoke finalizer from V8 garbage collector. + void InvokeFinalizerFromGC(v8impl::RefTracker* finalizer); + // Enqueue the finalizer to the napi_env's own queue of the second pass // weak callback. // Implementation should drain the queue at the time it is safe to call @@ -130,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; @@ -148,6 +164,7 @@ struct napi_env__ { int refs = 1; void* instance_data = nullptr; int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; + bool in_gc_finalizer = false; protected: // Should not be deleted directly. Delete with `napi_env__::DeleteMe()` @@ -211,6 +228,7 @@ inline napi_status napi_set_last_error(napi_env env, // NAPI_PREAMBLE is not wrapped in do..while: try_catch must have function scope #define NAPI_PREAMBLE(env) \ CHECK_ENV((env)); \ + (env)->CheckGCAccess(); \ RETURN_STATUS_IF_FALSE( \ (env), (env)->last_exception.IsEmpty(), napi_pending_exception); \ RETURN_STATUS_IF_FALSE((env), \ @@ -355,8 +373,28 @@ enum class Ownership { kUserland, }; -// Wrapper around Finalizer that implements reference counting. -class RefBase : public Finalizer, public RefTracker { +// Wrapper around Finalizer that can be tracked. +class TrackedFinalizer : public Finalizer, public RefTracker { + protected: + TrackedFinalizer(napi_env env, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); + + public: + static TrackedFinalizer* New(napi_env env, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); + ~TrackedFinalizer() override; + + protected: + void Finalize() override; + void FinalizeCore(bool deleteMe); +}; + +// Wrapper around TrackedFinalizer that implements reference counting. +class RefBase : public TrackedFinalizer { protected: RefBase(napi_env env, uint32_t initial_refcount, @@ -372,7 +410,6 @@ class RefBase : public Finalizer, public RefTracker { napi_finalize finalize_callback, void* finalize_data, void* finalize_hint); - virtual ~RefBase(); void* Data(); uint32_t Ref(); 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 368f05f3f4a261..7285b71f54231f 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -917,6 +917,7 @@ napi_status NAPI_CDECL napi_async_init(napi_env env, napi_value async_resource_name, napi_async_context* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, async_resource_name); CHECK_ARG(env, result); @@ -950,6 +951,7 @@ napi_status NAPI_CDECL napi_async_init(napi_env env, napi_status NAPI_CDECL napi_async_destroy(napi_env env, napi_async_context async_context) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, async_context); v8impl::AsyncContext* node_async_context = @@ -1099,6 +1101,7 @@ napi_status NAPI_CDECL napi_is_buffer(napi_env env, napi_value value, bool* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -1111,6 +1114,7 @@ napi_status NAPI_CDECL napi_get_buffer_info(napi_env env, void** data, size_t* length) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, value); v8::Local buffer = v8impl::V8LocalValueFromJsValue(value); @@ -1232,6 +1236,7 @@ napi_create_async_work(napi_env env, void* data, napi_async_work* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, execute); CHECK_ARG(env, result); @@ -1262,6 +1267,7 @@ napi_create_async_work(napi_env env, napi_status NAPI_CDECL napi_delete_async_work(napi_env env, napi_async_work work) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, work); uvimpl::Work::Delete(reinterpret_cast(work)); @@ -1316,6 +1322,7 @@ napi_create_threadsafe_function(napi_env env, napi_threadsafe_function_call_js call_js_cb, napi_threadsafe_function* result) { CHECK_ENV(env); + env->CheckGCAccess(); CHECK_ARG(env, async_resource_name); RETURN_STATUS_IF_FALSE(env, initial_thread_count > 0, napi_invalid_arg); CHECK_ARG(env, result); diff --git a/test/js-native-api/test_finalizer/binding.gyp b/test/js-native-api/test_finalizer/binding.gyp new file mode 100644 index 00000000000000..4c63346f30ce74 --- /dev/null +++ b/test/js-native-api/test_finalizer/binding.gyp @@ -0,0 +1,11 @@ +{ + "targets": [ + { + "target_name": "test_finalizer", + "defines": [ "NAPI_EXPERIMENTAL" ], + "sources": [ + "test_finalizer.c" + ] + } + ] +} diff --git a/test/js-native-api/test_finalizer/test.js b/test/js-native-api/test_finalizer/test.js new file mode 100644 index 00000000000000..cfbf57239c3a6d --- /dev/null +++ b/test/js-native-api/test_finalizer/test.js @@ -0,0 +1,43 @@ +'use strict'; +// Flags: --expose-gc + +const common = require('../../common'); +const test_finalizer = require(`./build/${common.buildType}/test_finalizer`); +const assert = require('assert'); + +// The goal of this test is to show that we can run "pure" finalizers in the +// current JS loop tick. Thus, we do not use common.gcUntil function works +// asynchronously using micro tasks. +// We use IIFE for the obj scope instead of {} to be compatible with +// non-V8 JS engines that do not support scoped variables. +(() => { + const obj = {}; + test_finalizer.addFinalizer(obj); +})(); + +for (let i = 0; i < 10; ++i) { + global.gc(); + if (test_finalizer.getFinalizerCallCount() === 1) { + break; + } +} + +assert.strictEqual(test_finalizer.getFinalizerCallCount(), 1); + +// The finalizer that access JS cannot run synchronously. They are run in the +// next JS loop tick. Thus, we must use common.gcUntil. +async function runAsyncTests() { + // We do not use common.mustCall() because we want to see the finalizer + // called in response to GC and not as a part of env destruction. + let js_is_called = false; + // We use IIFE for the obj scope instead of {} to be compatible with + // non-V8 JS engines that do not support scoped variables. + (() => { + const obj = {}; + test_finalizer.addFinalizerWithJS(obj, () => { js_is_called = true; }); + })(); + await common.gcUntil('ensure JS finalizer called', + () => (test_finalizer.getFinalizerCallCount() === 2)); + assert(js_is_called); +} +runAsyncTests(); 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..352310128a97f6 --- /dev/null +++ b/test/js-native-api/test_finalizer/test_fatal_finalize.js @@ -0,0 +1,31 @@ +'use strict'; +const common = require('../../common'); + +if (process.argv[2] === 'child') { + 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, common.isWindows ? null : 'SIGABRT'); +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 new file mode 100644 index 00000000000000..378781b7042f96 --- /dev/null +++ b/test/js-native-api/test_finalizer/test_finalizer.c @@ -0,0 +1,146 @@ +#include +#include +#include +#include +#include +#include "../common.h" +#include "../entry_point.h" + +typedef struct { + int32_t finalize_count; + napi_ref js_func; +} FinalizerData; + +static void finalizerOnlyCallback(napi_env env, + void* finalize_data, + void* finalize_hint) { + FinalizerData* data = (FinalizerData*)finalize_data; + int32_t count = ++data->finalize_count; + + // It is safe to access instance data + NODE_API_CALL_RETURN_VOID(env, napi_get_instance_data(env, (void**)&data)); + NODE_API_ASSERT_RETURN_VOID(env, + count = data->finalize_count, + "Expected to be the same FinalizerData"); +} + +static void finalizerCallingJSCallback(napi_env env, + void* finalize_data, + 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_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_delete_reference(env, data->js_func)); + data->js_func = NULL; + ++data->finalize_count; +} + +// Schedule async finalizer to run JavaScript-touching code. +static void finalizerWithJSCallback(napi_env env, + void* finalize_data, + void* 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] = {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, (void**)&data)); + NODE_API_CALL(env, + napi_add_finalizer( + env, argv[0], data, finalizerOnlyCallback, NULL, NULL)); + return NULL; +} + +// 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}; + napi_valuetype arg_type; + 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, (void**)&data)); + NODE_API_CALL(env, napi_typeof(env, argv[1], &arg_type)); + NODE_API_ASSERT( + env, arg_type == napi_function, "Expected function as the second arg"); + NODE_API_CALL(env, napi_create_reference(env, argv[1], 1, &data->js_func)); + NODE_API_CALL(env, + napi_add_finalizer( + env, argv[0], data, finalizerWithJSCallback, NULL, NULL)); + 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, (void**)&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]; + FinalizerData* data; + napi_value result; + + NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); + NODE_API_CALL(env, napi_get_instance_data(env, (void**)&data)); + NODE_API_CALL(env, napi_create_int32(env, data->finalize_count, &result)); + return result; +} + +static void finalizeData(napi_env env, void* data, void* hint) { + free(data); +} + +EXTERN_C_START +napi_value Init(napi_env env, napi_value exports) { + FinalizerData* data = (FinalizerData*)malloc(sizeof(FinalizerData)); + NODE_API_ASSERT(env, data != NULL, "Failed to allocate memory"); + memset(data, 0, sizeof(FinalizerData)); + NODE_API_CALL(env, napi_set_instance_data(env, data, finalizeData, NULL)); + 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)}; + + NODE_API_CALL( + env, + napi_define_properties(env, + exports, + sizeof(descriptors) / sizeof(*descriptors), + descriptors)); + + return exports; +} +EXTERN_C_END