From 77df31fa4ec0ffb299300414680d490c5ffed228 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Thu, 10 Nov 2022 08:04:37 +0000 Subject: [PATCH] node-api: disambiguate napi_add_finalizer napi_add_finalizer doesn't support any operations related to napi_wrap. Remove the ambiguous statements in the doc about napi_wrap and avoid reusing the v8impl::Wrap call. --- doc/api/n-api.md | 16 +++----- src/js_native_api.h | 2 +- src/js_native_api_v8.cc | 81 ++++++++++++++++++++++------------------- 3 files changed, 49 insertions(+), 50 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index b0b89de1c75bf6..f656e385186e67 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -5285,7 +5285,7 @@ napiVersion: 5 ```c napi_status napi_add_finalizer(napi_env env, napi_value js_object, - void* native_object, + void* finalize_data, napi_finalize finalize_cb, void* finalize_hint, napi_ref* result); @@ -5294,10 +5294,9 @@ napi_status napi_add_finalizer(napi_env env, * `[in] env`: The environment that the API is invoked under. * `[in] js_object`: The JavaScript object to which the native data will be attached. -* `[in] native_object`: The native data that will be attached to the JavaScript - object. +* `[in] finalize_data`: Optional data to be passed to `finalize_cb`. * `[in] finalize_cb`: Native callback that will be used to free the - native data when the JavaScript object is ready for garbage-collection. + native data when the JavaScript object is been garbage-collected. [`napi_finalize`][] provides more details. * `[in] finalize_hint`: Optional contextual hint that is passed to the finalize callback. @@ -5306,14 +5305,9 @@ napi_status napi_add_finalizer(napi_env env, Returns `napi_ok` if the API succeeded. Adds a `napi_finalize` callback which will be called when the JavaScript object -in `js_object` is ready for garbage collection. This API is similar to -`napi_wrap()` except that: +in `js_object` is been garbage-collected. -* the native data cannot be retrieved later using `napi_unwrap()`, -* nor can it be removed later using `napi_remove_wrap()`, and -* the API can be called multiple times with different data items in order to - attach each of them to the JavaScript object, and -* the object manipulated by the API can be used with `napi_wrap()`. +This API can be called multiple times on a single JavaScript object. _Caution_: The optional returned reference (if obtained) should be deleted via [`napi_delete_reference`][] ONLY in response to the finalize callback diff --git a/src/js_native_api.h b/src/js_native_api.h index d11a2c5a18cf16..b72f5980dc0785 100644 --- a/src/js_native_api.h +++ b/src/js_native_api.h @@ -492,7 +492,7 @@ NAPI_EXTERN napi_status NAPI_CDECL napi_get_date_value(napi_env env, // Add finalizer for pointer NAPI_EXTERN napi_status NAPI_CDECL napi_add_finalizer(napi_env env, napi_value js_object, - void* native_object, + void* finalize_data, napi_finalize finalize_cb, void* finalize_hint, napi_ref* result); diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 9000175d9d869a..3555d368136840 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -392,9 +392,6 @@ class FunctionCallbackWrapper : public CallbackWrapperBase { } }; -enum WrapType { retrievable, anonymous }; - -template inline napi_status Wrap(napi_env env, napi_value js_object, void* native_object, @@ -410,47 +407,36 @@ inline napi_status Wrap(napi_env env, RETURN_STATUS_IF_FALSE(env, value->IsObject(), napi_invalid_arg); v8::Local obj = value.As(); - if (wrap_type == retrievable) { - // If we've already wrapped this object, we error out. - RETURN_STATUS_IF_FALSE( - env, - !obj->HasPrivate(context, NAPI_PRIVATE_KEY(context, wrapper)) - .FromJust(), - napi_invalid_arg); - } else if (wrap_type == anonymous) { - // If no finalize callback is provided, we error out. - CHECK_ARG(env, finalize_cb); - } + // If we've already wrapped this object, we error out. + RETURN_STATUS_IF_FALSE( + env, + !obj->HasPrivate(context, NAPI_PRIVATE_KEY(context, wrapper)).FromJust(), + napi_invalid_arg); - v8impl::Reference* reference = nullptr; + // Create a self-deleting reference if the optional out-param result is not + // set. + bool delete_self = true; if (result != nullptr) { // The returned reference should be deleted via napi_delete_reference() // ONLY in response to the finalize callback invocation. (If it is deleted // before then, then the finalize callback will never be invoked.) // Therefore a finalize callback is required when returning a reference. CHECK_ARG(env, finalize_cb); - reference = v8impl::Reference::New( - env, obj, 0, false, finalize_cb, native_object, finalize_hint); + delete_self = false; + } + + v8impl::Reference* reference = v8impl::Reference::New( + env, obj, 0, delete_self, finalize_cb, native_object, finalize_hint); + + if (result != nullptr) { *result = reinterpret_cast(reference); - } else { - // Create a self-deleting reference. - reference = v8impl::Reference::New( - env, - obj, - 0, - true, - finalize_cb, - native_object, - finalize_cb == nullptr ? nullptr : finalize_hint); - } - - if (wrap_type == retrievable) { - CHECK(obj->SetPrivate(context, - NAPI_PRIVATE_KEY(context, wrapper), - v8::External::New(env->isolate, reference)) - .FromJust()); } + CHECK(obj->SetPrivate(context, + NAPI_PRIVATE_KEY(context, wrapper), + v8::External::New(env->isolate, reference)) + .FromJust()); + return GET_RETURN_STATUS(env); } @@ -2360,7 +2346,7 @@ napi_status NAPI_CDECL napi_wrap(napi_env env, napi_finalize finalize_cb, void* finalize_hint, napi_ref* result) { - return v8impl::Wrap( + return v8impl::Wrap( env, js_object, native_object, finalize_cb, finalize_hint, result); } @@ -3176,12 +3162,31 @@ napi_status NAPI_CDECL napi_run_script(napi_env env, napi_status NAPI_CDECL napi_add_finalizer(napi_env env, napi_value js_object, - void* native_object, + void* finalize_data, napi_finalize finalize_cb, void* finalize_hint, napi_ref* result) { - return v8impl::Wrap( - env, js_object, native_object, finalize_cb, finalize_hint, result); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); + CHECK_ARG(env, js_object); + CHECK_ARG(env, finalize_cb); + + v8::Local v8_value = v8impl::V8LocalValueFromJsValue(js_object); + if (!(v8_value->IsObject() || v8_value->IsFunction())) { + return napi_set_last_error(env, napi_invalid_arg); + } + + // Create a self-deleting reference if the optional out-param result is not + // set. + bool delete_self = result == nullptr; + v8impl::Reference* reference = v8impl::Reference::New( + env, v8_value, 0, delete_self, finalize_cb, finalize_data, finalize_hint); + + if (result != nullptr) { + *result = reinterpret_cast(reference); + } + return napi_clear_last_error(env); } napi_status NAPI_CDECL napi_adjust_external_memory(napi_env env,