From bd98e5baba78b2fb0b3eb02f6468a4f2090363da 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. PR-URL: https://github.com/nodejs/node/pull/45401 Reviewed-By: Michael Dawson --- doc/api/n-api.md | 41 ++++++++---------------------- src/js_native_api.h | 2 +- src/js_native_api_v8.cc | 56 +++++++++++++++++++++++------------------ 3 files changed, 43 insertions(+), 56 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 9c3c4588b49b28..933dfff50ee494 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -2382,12 +2382,7 @@ is used to pass external data through JavaScript code, so it can be retrieved later by native code using [`napi_get_value_external`][]. The API adds a `napi_finalize` callback which will be called when the JavaScript -object just created is ready for garbage collection. It is similar to -`napi_wrap()` except that: - -* the native data cannot be retrieved later using `napi_unwrap()`, -* nor can it be removed later using `napi_remove_wrap()`, and -* the object created by the API can be used with `napi_wrap()`. +object just created has been garbage collected. The created value is not an object, and therefore does not support additional properties. It is considered a distinct value type: calling `napi_typeof()` with @@ -2441,12 +2436,7 @@ managed. The caller must ensure that the byte buffer remains valid until the finalize callback is called. The API adds a `napi_finalize` callback which will be called when the JavaScript -object just created is ready for garbage collection. It is similar to -`napi_wrap()` except that: - -* the native data cannot be retrieved later using `napi_unwrap()`, -* nor can it be removed later using `napi_remove_wrap()`, and -* the object created by the API can be used with `napi_wrap()`. +object just created has been garbage collected. JavaScript `ArrayBuffer`s are described in [Section 24.1][] of the ECMAScript Language Specification. @@ -2497,12 +2487,7 @@ backed by the passed in buffer. While this is still a fully-supported data structure, in most cases using a `TypedArray` will suffice. The API adds a `napi_finalize` callback which will be called when the JavaScript -object just created is ready for garbage collection. It is similar to -`napi_wrap()` except that: - -* the native data cannot be retrieved later using `napi_unwrap()`, -* nor can it be removed later using `napi_remove_wrap()`, and -* the object created by the API can be used with `napi_wrap()`. +object just created has been garbage collected. For Node.js >=4 `Buffers` are `Uint8Array`s. @@ -5141,7 +5126,7 @@ napi_status napi_wrap(napi_env env, * `[in] native_object`: The native instance that will be wrapped in the JavaScript object. * `[in] finalize_cb`: Optional native callback that can be used to free the - native instance when the JavaScript object is ready for garbage-collection. + native instance when the JavaScript object has been garbage-collected. [`napi_finalize`][] provides more details. * `[in] finalize_hint`: Optional contextual hint that is passed to the finalize callback. @@ -5303,7 +5288,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); @@ -5312,10 +5297,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 has been garbage-collected. [`napi_finalize`][] provides more details. * `[in] finalize_hint`: Optional contextual hint that is passed to the finalize callback. @@ -5324,14 +5308,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: - -* 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()`. +in `js_object` has been garbage-collected. + +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 157ed6e77fae30..2203605afa9c65 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -401,9 +401,6 @@ class FunctionCallbackWrapper : public CallbackWrapperBase { } }; -enum WrapType { retrievable, anonymous }; - -template inline napi_status Wrap(napi_env env, napi_value js_object, void* native_object, @@ -419,17 +416,11 @@ 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; if (result != nullptr) { @@ -458,12 +449,10 @@ inline napi_status Wrap(napi_env env, 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); } @@ -2289,7 +2278,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); } @@ -3110,12 +3099,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); + RETURN_STATUS_IF_FALSE(env, v8_value->IsObject(), napi_invalid_arg); + + // Create a self-deleting reference if the optional out-param result is not + // set. + v8impl::Ownership ownership = result == nullptr + ? v8impl::Ownership::kRuntime + : v8impl::Ownership::kUserland; + v8impl::Reference* reference = v8impl::Reference::New( + env, v8_value, 0, ownership, 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,