From f883bedcebab1a4cc54f0b1849cbb0c11c8377a6 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Fri, 13 Dec 2024 14:49:45 +0000 Subject: [PATCH] node-api: allow napi_delete_reference in finalizers `napi_delete_reference` must be called immediately for a `napi_reference` returned from `napi_wrap` in the corresponding finalizer, in order to clean up the `napi_reference` timely. `napi_delete_reference` is safe to be invoked during GC. PR-URL: https://github.com/nodejs/node/pull/55620 Reviewed-By: Gabriel Schulhof Reviewed-By: Michael Dawson Reviewed-By: James M Snell Reviewed-By: Vladimir Morozov --- src/js_native_api_v8.cc | 4 +- .../6_object_wrap/6_object_wrap.cc | 37 +++++++++++++++++-- test/js-native-api/6_object_wrap/binding.gyp | 7 ++++ test/js-native-api/6_object_wrap/myobject.h | 4 +- .../6_object_wrap/test-basic-finalizer.js | 24 ++++++++++++ 5 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 test/js-native-api/6_object_wrap/test-basic-finalizer.js diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index d2334f65023161..3159cd7f69b6f4 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -2769,10 +2769,12 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env, // Deletes a reference. The referenced value is released, and may be GC'd unless // there are other references to it. +// For a napi_reference returned from `napi_wrap`, this must be called in the +// finalizer. 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_NOT_IN_GC(env); + CHECK_ENV(env); CHECK_ARG(env, ref); delete reinterpret_cast(ref); diff --git a/test/js-native-api/6_object_wrap/6_object_wrap.cc b/test/js-native-api/6_object_wrap/6_object_wrap.cc index 49b1241fb38caa..8a380e3caa20bb 100644 --- a/test/js-native-api/6_object_wrap/6_object_wrap.cc +++ b/test/js-native-api/6_object_wrap/6_object_wrap.cc @@ -3,6 +3,8 @@ #include "assert.h" #include "myobject.h" +typedef int32_t FinalizerData; + napi_ref MyObject::constructor; MyObject::MyObject(double value) @@ -10,10 +12,16 @@ MyObject::MyObject(double value) MyObject::~MyObject() { napi_delete_reference(env_, wrapper_); } -void MyObject::Destructor( - napi_env env, void* nativeObject, void* /*finalize_hint*/) { +void MyObject::Destructor(node_api_basic_env env, + void* nativeObject, + void* /*finalize_hint*/) { MyObject* obj = static_cast(nativeObject); delete obj; + + FinalizerData* data; + NODE_API_BASIC_CALL_RETURN_VOID( + env, napi_get_instance_data(env, reinterpret_cast(&data))); + *data += 1; } void MyObject::Init(napi_env env, napi_value exports) { @@ -154,7 +162,7 @@ napi_value MyObject::Multiply(napi_env env, napi_callback_info info) { } // This finalizer should never be invoked. -void ObjectWrapDanglingReferenceFinalizer(napi_env env, +void ObjectWrapDanglingReferenceFinalizer(node_api_basic_env env, void* finalize_data, void* finalize_hint) { assert(0 && "unreachable"); @@ -198,8 +206,30 @@ napi_value ObjectWrapDanglingReferenceTest(napi_env env, return ret; } +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, nullptr, nullptr)); + NODE_API_CALL(env, + napi_get_instance_data(env, reinterpret_cast(&data))); + NODE_API_CALL(env, napi_create_int32(env, *data, &result)); + return result; +} + +static void finalizeData(napi_env env, void* data, void* hint) { + delete reinterpret_cast(data); +} + EXTERN_C_START napi_value Init(napi_env env, napi_value exports) { + FinalizerData* data = new FinalizerData; + *data = 0; + NODE_API_CALL(env, napi_set_instance_data(env, data, finalizeData, nullptr)); + MyObject::Init(env, exports); napi_property_descriptor descriptors[] = { @@ -207,6 +237,7 @@ napi_value Init(napi_env env, napi_value exports) { ObjectWrapDanglingReference), DECLARE_NODE_API_PROPERTY("objectWrapDanglingReferenceTest", ObjectWrapDanglingReferenceTest), + DECLARE_NODE_API_PROPERTY("getFinalizerCallCount", GetFinalizerCallCount), }; NODE_API_CALL( diff --git a/test/js-native-api/6_object_wrap/binding.gyp b/test/js-native-api/6_object_wrap/binding.gyp index 44c9c3f837b4a6..2be24c9ec171a9 100644 --- a/test/js-native-api/6_object_wrap/binding.gyp +++ b/test/js-native-api/6_object_wrap/binding.gyp @@ -5,6 +5,13 @@ "sources": [ "6_object_wrap.cc" ] + }, + { + "target_name": "6_object_wrap_basic_finalizer", + "defines": [ "NAPI_EXPERIMENTAL" ], + "sources": [ + "6_object_wrap.cc" + ] } ] } diff --git a/test/js-native-api/6_object_wrap/myobject.h b/test/js-native-api/6_object_wrap/myobject.h index 337180598bc042..0faff676b4d992 100644 --- a/test/js-native-api/6_object_wrap/myobject.h +++ b/test/js-native-api/6_object_wrap/myobject.h @@ -6,7 +6,9 @@ class MyObject { public: static void Init(napi_env env, napi_value exports); - static void Destructor(napi_env env, void* nativeObject, void* finalize_hint); + static void Destructor(node_api_basic_env env, + void* nativeObject, + void* finalize_hint); private: explicit MyObject(double value_ = 0); diff --git a/test/js-native-api/6_object_wrap/test-basic-finalizer.js b/test/js-native-api/6_object_wrap/test-basic-finalizer.js new file mode 100644 index 00000000000000..46b5672c5fa4b9 --- /dev/null +++ b/test/js-native-api/6_object_wrap/test-basic-finalizer.js @@ -0,0 +1,24 @@ +// Flags: --expose-gc + +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const addon = require(`./build/${common.buildType}/6_object_wrap_basic_finalizer`); + +// This test verifies that ObjectWrap can be correctly finalized with a node_api_basic_finalizer +// in the current JS loop tick +(() => { + let obj = new addon.MyObject(9); + obj = null; + // Silent eslint about unused variables. + assert.strictEqual(obj, null); +})(); + +for (let i = 0; i < 10; ++i) { + global.gc(); + if (addon.getFinalizerCallCount() === 1) { + break; + } +} + +assert.strictEqual(addon.getFinalizerCallCount(), 1);