From f144c01fea8417cfc1c12cce1c8cb8894b237b0e Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 22 Jun 2023 19:39:22 -0700 Subject: [PATCH 01/10] node-api: run finalizers directly from GC --- src/js_native_api.h | 10 +++ src/js_native_api_v8.cc | 130 ++++++++++++++++++++++++++++----------- src/js_native_api_v8.h | 32 ++++++++-- src/node_api.cc | 2 +- src/node_api_internals.h | 2 + 5 files changed, 134 insertions(+), 42 deletions(-) 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 823799a901ad83..28f4b99d12b9b4 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -57,8 +57,28 @@ (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, but it suspends use of JS from finalizer. + // If JS calls are needed, then 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; + 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 + // finalizer. + EnqueueFinalizer(finalizer); + } + } +} +namespace v8impl { namespace { template @@ -604,28 +624,72 @@ 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(); + // 1. If the finalize_callback is present, it should either delete the + // derived RefBase, or set ownership with Ownership::kRuntime. + // 2. If the finalizer is not present, the derived 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 (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 +724,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 +821,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 @@ -3310,6 +3352,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) { diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 1ac738af2adb88..df30980f0c2b52 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 true; } + virtual bool can_call_into_js() const { return !suspend_call_into_js; } static inline void HandleThrow(napi_env env, v8::Local value) { if (env->terminatedOrTerminating()) { @@ -102,9 +102,13 @@ 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); }); } + // 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 @@ -148,6 +152,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; protected: // Should not be deleted directly. Delete with `napi_env__::DeleteMe()` @@ -355,8 +360,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 +397,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/node_api.cc b/src/node_api.cc index d86e8b29b093e4..f757bcbb2ce333 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 node_env()->can_call_into_js(); + return Super::can_call_into_js() && 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 25f6b291902024..d3b2b0c09b4c08 100644 --- a/src/node_api_internals.h +++ b/src/node_api_internals.h @@ -9,6 +9,8 @@ #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); From 929a0ef3036547802f30cbf0979db5ea2e4ef857 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 23 Jun 2023 07:45:34 -0700 Subject: [PATCH 02/10] fix disabling JS in finalizers --- src/js_native_api_v8.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 28f4b99d12b9b4..498aa0de2c13d2 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -67,6 +67,7 @@ void napi_env__::InvokeFinalizerFromGC(v8impl::RefTracker* finalizer) { // 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; finalizer->Finalize(); suspend_call_into_js = saved_suspend_call_into_js; } else { From e5e2a60fa810f74c6046e6093dbb60449d774cf1 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 23 Jun 2023 07:46:08 -0700 Subject: [PATCH 03/10] add unit test for pure finalizers --- test/js-native-api/test_finalizer/binding.gyp | 12 ++ test/js-native-api/test_finalizer/test.js | 35 ++++++ .../test_finalizer/test_finalizer.c | 111 ++++++++++++++++++ 3 files changed, 158 insertions(+) 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_finalizer.c 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..5ab1afcb352428 --- /dev/null +++ b/test/js-native-api/test_finalizer/binding.gyp @@ -0,0 +1,12 @@ +{ + "targets": [ + { + "target_name": "test_finalizer", + "defines": [ "NAPI_EXPERIMENTAL" ], + "sources": [ + "../entry_point.c", + "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..145a9723344082 --- /dev/null +++ b/test/js-native-api/test_finalizer/test.js @@ -0,0 +1,35 @@ +'use strict'; +// Flags: --expose-gc + +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) { + global.gc(); + if (test_finalizer.getFinalizerCallCount() === 1) { + break; + } +} + +assert(test_finalizer.getFinalizerCallCount() === 1); + +async function runAsyncTests() { + let js_is_called = false; + (() => { + const obj = {}; + test_finalizer.addFinalizerWithJS(obj, () => { js_is_called = true; }); + })(); + await common.gcUntil('test JS finalizer', + () => (test_finalizer.getFinalizerCallCount() === 2)); + assert(js_is_called); +} +runAsyncTests(); 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..bb7be613a49cb2 --- /dev/null +++ b/test/js-native-api/test_finalizer/test_finalizer.c @@ -0,0 +1,111 @@ +#include +#include +#include +#include +#include +#include "../common.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; + ++data->finalize_count; +} + +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) { + FinalizerData* data = (FinalizerData*)finalize_data; + NODE_API_CALL_RETURN_VOID(env, node_api_post_finalizer( + env, finalizerCallingJSCallback, finalize_data, finalize_hint)); +} + +static napi_value addFinalizer(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value argv[1]; + 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, finalizerOnlyCallback, NULL, NULL)); + return NULL; +} + +// This finalizer is going to call JavaScript from finalizer +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, &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; +} + +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, &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("getFinalizerCallCount", + getFinalizerCallCount)}; + + NODE_API_CALL( + env, + napi_define_properties(env, + exports, + sizeof(descriptors) / sizeof(*descriptors), + descriptors)); + + return exports; +} +EXTERN_C_END From 41e61e10cb2ce263374881170b11fd28212a36b4 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 7 Jul 2023 07:50:34 -0700 Subject: [PATCH 04/10] add test to crash on GC state access and update docs --- doc/api/n-api.md | 36 ++++++++++++++++ src/js_native_api_v8.cc | 30 +++++++++++--- src/js_native_api_v8.h | 19 +++++++-- src/js_native_api_v8_internals.h | 6 +++ src/node_api.cc | 2 +- src/node_api_internals.h | 2 - test/js-native-api/test_finalizer/test.js | 11 ++--- .../test_finalizer/test_fatal_finalize.js | 31 ++++++++++++++ .../test_finalizer/test_finalizer.c | 41 ++++++++++++++++--- 9 files changed, 153 insertions(+), 25 deletions(-) create mode 100644 test/js-native-api/test_finalizer/test_fatal_finalize.js diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 2c45e01076d119..4b65d02c8f3305 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -5428,6 +5428,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 df30980f0c2b52..99fadf97bf2583 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 f757bcbb2ce333..d86e8b29b093e4 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)}; From f199c72f741a47e8d7648980f7f7adc8801c1f34 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 21 Jul 2023 07:21:35 -0700 Subject: [PATCH 05/10] add CheckGCAccess calls where code may access GC --- src/js_native_api_v8.cc | 96 +++++++++++++------ src/js_native_api_v8.h | 2 +- src/node_api.cc | 7 ++ .../test_finalizer/test_finalizer.c | 7 +- 4 files changed, 80 insertions(+), 32 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 08e4ef1593927d..191db139104da1 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -64,18 +64,11 @@ void napi_env__::InvokeFinalizerFromGC(v8impl::RefTracker* finalizer) { // 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 - // must call node_api_post_finalizer. - if (last_error.error_code == napi_ok && last_exception.IsEmpty()) { - auto restore_state = node::OnScopeLeave( - [this, saved = in_gc_finalizer] { in_gc_finalizer = saved; }); - in_gc_finalizer = true; - finalizer->Finalize(); - } 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 - // finalizer. - EnqueueFinalizer(finalizer); - } + // 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(); } } @@ -89,11 +82,11 @@ 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( env, (length == NAPI_AUTO_LENGTH) || length <= INT_MAX, napi_invalid_arg); - env->CheckGCAccess(); auto isolate = env->isolate; auto str_maybe = string_maker(isolate); @@ -114,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) { @@ -1480,6 +1474,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); @@ -1539,8 +1534,8 @@ 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(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Object::New(env->isolate)); @@ -1549,8 +1544,8 @@ 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(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Array::New(env->isolate)); @@ -1561,8 +1556,8 @@ napi_status NAPI_CDECL napi_create_array_with_length(napi_env env, size_t length, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Array::New(env->isolate, length)); @@ -1662,8 +1657,8 @@ napi_status NAPI_CDECL napi_create_double(napi_env env, double value, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Number::New(env->isolate, value)); @@ -1675,8 +1670,8 @@ napi_status NAPI_CDECL napi_create_int32(napi_env env, int32_t value, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::Integer::New(env->isolate, value)); @@ -1688,8 +1683,8 @@ napi_status NAPI_CDECL napi_create_uint32(napi_env env, uint32_t value, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue( v8::Integer::NewFromUnsigned(env->isolate, value)); @@ -1701,8 +1696,8 @@ napi_status NAPI_CDECL napi_create_int64(napi_env env, int64_t value, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue( v8::Number::New(env->isolate, static_cast(value))); @@ -1714,8 +1709,8 @@ napi_status NAPI_CDECL napi_create_bigint_int64(napi_env env, int64_t value, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue(v8::BigInt::New(env->isolate, value)); @@ -1727,8 +1722,8 @@ napi_status NAPI_CDECL napi_create_bigint_uint64(napi_env env, uint64_t value, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); *result = v8impl::JsValueFromV8LocalValue( v8::BigInt::NewFromUnsigned(env->isolate, value)); @@ -1742,9 +1737,9 @@ 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); - env->CheckGCAccess(); v8::Local context = env->context(); @@ -1763,8 +1758,8 @@ napi_status NAPI_CDECL napi_get_boolean(napi_env env, bool value, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); v8::Isolate* isolate = env->isolate; @@ -1781,8 +1776,8 @@ napi_status NAPI_CDECL napi_create_symbol(napi_env env, napi_value description, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); v8::Isolate* isolate = env->isolate; @@ -1804,8 +1799,8 @@ napi_status NAPI_CDECL node_api_symbol_for(napi_env env, size_t length, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, result); env->CheckGCAccess(); + CHECK_ARG(env, result); napi_value js_description_string; STATUS_CALL(napi_create_string_utf8( @@ -1850,9 +1845,9 @@ 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); - env->CheckGCAccess(); v8::Local message_value = v8impl::V8LocalValueFromJsValue(msg); RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected); @@ -1871,9 +1866,9 @@ 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); - env->CheckGCAccess(); v8::Local message_value = v8impl::V8LocalValueFromJsValue(msg); RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected); @@ -1892,9 +1887,9 @@ 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); - env->CheckGCAccess(); v8::Local message_value = v8impl::V8LocalValueFromJsValue(msg); RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected); @@ -1913,9 +1908,9 @@ 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); - env->CheckGCAccess(); v8::Local message_value = v8impl::V8LocalValueFromJsValue(msg); RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected); @@ -1935,6 +1930,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); @@ -1974,6 +1970,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)); @@ -1983,6 +1980,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)); @@ -2074,6 +2072,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()); @@ -2171,6 +2170,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); @@ -2186,6 +2186,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); @@ -2203,6 +2204,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); @@ -2227,6 +2229,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); @@ -2251,6 +2254,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); @@ -2284,6 +2288,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); @@ -2302,6 +2307,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); @@ -2321,6 +2327,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); @@ -2351,6 +2358,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); @@ -2373,6 +2381,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); @@ -2411,6 +2420,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); @@ -2452,6 +2462,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); @@ -2638,6 +2649,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); @@ -2658,6 +2670,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); @@ -2682,6 +2695,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); @@ -2700,6 +2714,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); @@ -2722,6 +2737,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); @@ -2748,6 +2764,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); @@ -2762,6 +2779,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( @@ -2775,6 +2793,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; @@ -2790,6 +2809,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( @@ -2803,6 +2823,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; @@ -2820,6 +2841,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); @@ -2898,6 +2920,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(); @@ -2909,6 +2932,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()) { @@ -2926,6 +2950,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); @@ -2978,6 +3003,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); @@ -3000,6 +3026,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); @@ -3086,6 +3113,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); @@ -3176,6 +3204,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); @@ -3192,6 +3221,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); @@ -3267,6 +3297,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); @@ -3293,6 +3324,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); @@ -3305,6 +3337,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); @@ -3351,6 +3384,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); @@ -3430,6 +3464,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); @@ -3449,6 +3484,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 99fadf97bf2583..db39686c91127e 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -228,9 +228,9 @@ 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); \ - (env)->CheckGCAccess(); \ RETURN_STATUS_IF_FALSE((env), \ (env)->can_call_into_js(), \ (env->module_api_version == NAPI_VERSION_EXPERIMENTAL \ diff --git a/src/node_api.cc b/src/node_api.cc index d86e8b29b093e4..23e9fedf8b146e 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -918,6 +918,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); @@ -951,6 +952,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 = @@ -1100,6 +1102,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); @@ -1112,6 +1115,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); @@ -1233,6 +1237,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); @@ -1263,6 +1268,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)); @@ -1317,6 +1323,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/test_finalizer.c b/test/js-native-api/test_finalizer/test_finalizer.c index 74de20254b81b3..d7394674331d73 100644 --- a/test/js-native-api/test_finalizer/test_finalizer.c +++ b/test/js-native-api/test_finalizer/test_finalizer.c @@ -14,7 +14,12 @@ static void finalizerOnlyCallback(napi_env env, void* finalize_data, void* finalize_hint) { FinalizerData* data = (FinalizerData*)finalize_data; - ++data->finalize_count; + int32_t count = ++data->finalize_count; + + // It is safe to access instance data + NODE_API_CALL_RETURN_VOID(env, napi_get_instance_data(env, &data)); + NODE_API_ASSERT( + env, count = data->finalize_count, "Expected to the same FinalizerData"); } static void finalizerCallingJSCallback(napi_env env, From 2edf5afff5939dabb17c04ccae5af3d85bd2ff00 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 4 Aug 2023 07:53:15 -0700 Subject: [PATCH 06/10] fix code and test for Linux --- .../test_finalizer/test_fatal_finalize.js | 10 +++++++--- test/js-native-api/test_finalizer/test_finalizer.c | 13 ++++++------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/test/js-native-api/test_finalizer/test_fatal_finalize.js b/test/js-native-api/test_finalizer/test_fatal_finalize.js index 80ab84385f94ec..09bf2d89f9682c 100644 --- a/test/js-native-api/test_finalizer/test_fatal_finalize.js +++ b/test/js-native-api/test_finalizer/test_fatal_finalize.js @@ -1,7 +1,7 @@ 'use strict'; +const common = require('../../common'); if (process.argv[2] === 'child') { - const common = require('../../common'); const test_finalizer = require(`./build/${common.buildType}/test_finalizer`); (() => { @@ -27,5 +27,9 @@ 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/); +if (common.isWindows) { + assert.strictEqual(child.signal, null); + assert.match(child.stderr.toString(), /Finalizer is calling a function that may affect GC state/); +} else { + assert.strictEqual(child.signal, 'SIGABRT'); +} diff --git a/test/js-native-api/test_finalizer/test_finalizer.c b/test/js-native-api/test_finalizer/test_finalizer.c index d7394674331d73..868e4f0453a9e6 100644 --- a/test/js-native-api/test_finalizer/test_finalizer.c +++ b/test/js-native-api/test_finalizer/test_finalizer.c @@ -17,8 +17,8 @@ static void finalizerOnlyCallback(napi_env env, int32_t count = ++data->finalize_count; // It is safe to access instance data - NODE_API_CALL_RETURN_VOID(env, napi_get_instance_data(env, &data)); - NODE_API_ASSERT( + 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 the same FinalizerData"); } @@ -41,7 +41,6 @@ static void finalizerCallingJSCallback(napi_env env, 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( @@ -63,7 +62,7 @@ static napi_value addFinalizer(napi_env env, napi_callback_info info) { 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_get_instance_data(env, (void**)&data)); NODE_API_CALL(env, napi_add_finalizer( env, argv[0], data, finalizerOnlyCallback, NULL, NULL)); @@ -78,7 +77,7 @@ static napi_value addFinalizerWithJS(napi_env env, napi_callback_info info) { 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_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"); @@ -96,7 +95,7 @@ static napi_value addFinalizerFailOnJS(napi_env env, napi_callback_info info) { 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_get_instance_data(env, (void**)&data)); NODE_API_CALL( env, napi_add_finalizer( @@ -111,7 +110,7 @@ static napi_value getFinalizerCallCount(napi_env env, napi_callback_info info) { 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, &data)); + 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; } From 971b8944690279bc7efefbe77dc294bef55f2df5 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 18 Aug 2023 14:25:25 -0700 Subject: [PATCH 07/10] fix JS linter error to use assert.strictEqual --- test/js-native-api/test_finalizer/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js-native-api/test_finalizer/test.js b/test/js-native-api/test_finalizer/test.js index 3d4760cb48c860..948bca1995a2cf 100644 --- a/test/js-native-api/test_finalizer/test.js +++ b/test/js-native-api/test_finalizer/test.js @@ -17,7 +17,7 @@ for (let i = 0; i < 10; ++i) { } } -assert(test_finalizer.getFinalizerCallCount() === 1); +assert.strictEqual(test_finalizer.getFinalizerCallCount(), 1); async function runAsyncTests() { let js_is_called = false; From 5ef58d375ee5830a64ab38fc7ef1d6ba75789b97 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 26 Sep 2023 21:39:37 -0700 Subject: [PATCH 08/10] Address PR feedback --- Call_while_GC.txt | 18 ++++++++++++++++++ doc/api/n-api.md | 12 ++++++------ src/js_native_api_v8.cc | 9 ++++----- test/js-native-api/test_finalizer/binding.gyp | 1 - test/js-native-api/test_finalizer/test.js | 11 +++++++++++ .../test_finalizer/test_finalizer.c | 6 ++++-- 6 files changed, 43 insertions(+), 14 deletions(-) create mode 100644 Call_while_GC.txt diff --git a/Call_while_GC.txt b/Call_while_GC.txt new file mode 100644 index 00000000000000..914ab612aeb797 --- /dev/null +++ b/Call_while_GC.txt @@ -0,0 +1,18 @@ +safe functions: +napi_get_last_error_info +napi_get_cb_info +napi_get_new_target +napi_get_version +node_api_post_finalizer +napi_adjust_external_memory +napi_set_instance_data +napi_get_instance_data +napi_add_env_cleanup_hook +napi_remove_env_cleanup_hook +napi_add_async_cleanup_hook +napi_remove_async_cleanup_hook +napi_fatal_error +napi_open_callback_scope +napi_close_callback_scope +napi_get_node_version +node_api_get_module_file_name diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 4b65d02c8f3305..d469e54aa84535 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -5456,13 +5456,13 @@ 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). +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. -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. +`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 diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 191db139104da1..044244eccfe9ea 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -661,13 +661,11 @@ void TrackedFinalizer::FinalizeCore(bool deleteMe) { // 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 - // derived RefBase, or set ownership with Ownership::kRuntime. - // 2. If the finalizer is not present, the derived RefBase can be deleted - // after the call. + // 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); - // No access to `this` after finalize_callback is called. } if (deleteMe) { @@ -2026,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); diff --git a/test/js-native-api/test_finalizer/binding.gyp b/test/js-native-api/test_finalizer/binding.gyp index 5ab1afcb352428..4c63346f30ce74 100644 --- a/test/js-native-api/test_finalizer/binding.gyp +++ b/test/js-native-api/test_finalizer/binding.gyp @@ -4,7 +4,6 @@ "target_name": "test_finalizer", "defines": [ "NAPI_EXPERIMENTAL" ], "sources": [ - "../entry_point.c", "test_finalizer.c" ] } diff --git a/test/js-native-api/test_finalizer/test.js b/test/js-native-api/test_finalizer/test.js index 948bca1995a2cf..cfbf57239c3a6d 100644 --- a/test/js-native-api/test_finalizer/test.js +++ b/test/js-native-api/test_finalizer/test.js @@ -5,6 +5,11 @@ 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); @@ -19,8 +24,14 @@ for (let i = 0; i < 10; ++i) { 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; }); diff --git a/test/js-native-api/test_finalizer/test_finalizer.c b/test/js-native-api/test_finalizer/test_finalizer.c index 868e4f0453a9e6..378781b7042f96 100644 --- a/test/js-native-api/test_finalizer/test_finalizer.c +++ b/test/js-native-api/test_finalizer/test_finalizer.c @@ -4,6 +4,7 @@ #include #include #include "../common.h" +#include "../entry_point.h" typedef struct { int32_t finalize_count; @@ -18,8 +19,9 @@ static void finalizerOnlyCallback(napi_env env, // 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 the same FinalizerData"); + NODE_API_ASSERT_RETURN_VOID(env, + count = data->finalize_count, + "Expected to be the same FinalizerData"); } static void finalizerCallingJSCallback(napi_env env, From 03d16c7d62408cda35b230b3fc86128af22ce4b5 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Wed, 27 Sep 2023 08:55:07 -0700 Subject: [PATCH 09/10] remove Call_while_GC.txt that accidentally added --- Call_while_GC.txt | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 Call_while_GC.txt diff --git a/Call_while_GC.txt b/Call_while_GC.txt deleted file mode 100644 index 914ab612aeb797..00000000000000 --- a/Call_while_GC.txt +++ /dev/null @@ -1,18 +0,0 @@ -safe functions: -napi_get_last_error_info -napi_get_cb_info -napi_get_new_target -napi_get_version -node_api_post_finalizer -napi_adjust_external_memory -napi_set_instance_data -napi_get_instance_data -napi_add_env_cleanup_hook -napi_remove_env_cleanup_hook -napi_add_async_cleanup_hook -napi_remove_async_cleanup_hook -napi_fatal_error -napi_open_callback_scope -napi_close_callback_scope -napi_get_node_version -node_api_get_module_file_name From 25bfcaf9024b1ac3bcea78f60497e14ea4718784 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Wed, 27 Sep 2023 12:32:26 -0700 Subject: [PATCH 10/10] address PR feedback --- doc/api/n-api.md | 2 +- test/js-native-api/test_finalizer/test_fatal_finalize.js | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index d469e54aa84535..af9e9db5cae80f 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -5453,7 +5453,7 @@ napi_status node_api_post_finalizer(napi_env env, Returns `napi_ok` if the API succeeded. -Schedules `napi_finalize` callback to be called asynchronously in the +Schedules a `napi_finalize` callback to be called asynchronously in the event loop. Normally, finalizers are called while the GC (garbage collector) collects diff --git a/test/js-native-api/test_finalizer/test_fatal_finalize.js b/test/js-native-api/test_finalizer/test_fatal_finalize.js index 09bf2d89f9682c..352310128a97f6 100644 --- a/test/js-native-api/test_finalizer/test_fatal_finalize.js +++ b/test/js-native-api/test_finalizer/test_fatal_finalize.js @@ -27,9 +27,5 @@ const { spawnSync } = require('child_process'); const child = spawnSync(process.execPath, [ '--expose-gc', __filename, 'child', ]); -if (common.isWindows) { - assert.strictEqual(child.signal, null); - assert.match(child.stderr.toString(), /Finalizer is calling a function that may affect GC state/); -} else { - assert.strictEqual(child.signal, 'SIGABRT'); -} +assert.strictEqual(child.signal, common.isWindows ? null : 'SIGABRT'); +assert.match(child.stderr.toString(), /Finalizer is calling a function that may affect GC state/);