From 1aea747fafb97df9708a6c5d1e1345e6cc94bd00 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Fri, 6 Sep 2019 14:09:00 -0700 Subject: [PATCH] n-api: delete callback bundle via reference We should strive to use weak persistent references consistently throughout the code, since using `v8::Persistent` directly results in having to change many sites in the code when the way we use it changes. N-API uses `v8impl::Reference` internally when maintaining a weak persistent reference is necessary. So far, `v8impl::CallbackBundle` was using `v8::Persistent` directly in order to weakly reference the JS function backed by a N-API callback. The change introduced here reduces `v8impl::CallbackBundle` to a simple structure and uses a `v8impl::Reference` to weakly reference the N-API callback with which it is associated. The structure is freed by the `napi_finalize` callback of the `v8impl::Reference`. This brings N-API use of `v8::Persistent` completely under the `v8impl::Reference` umbrella, rendering our use of weak references consistent. PR-URL: https://github.com/nodejs/node/pull/29479 Reviewed-By: Ben Noordhuis Reviewed-By: Anna Henningsen --- src/js_native_api_v8.cc | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index fd8728d30f6cd9..a87af79a890832 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -379,27 +379,10 @@ inline static napi_status Unwrap(napi_env env, // Ref: benchmark/misc/function_call // Discussion (incl. perf. data): https://github.com/nodejs/node/pull/21072 struct CallbackBundle { - // Bind the lifecycle of `this` C++ object to a JavaScript object. - // We never delete a CallbackBundle C++ object directly. - void BindLifecycleTo(v8::Isolate* isolate, v8::Local target) { - handle.Reset(isolate, target); - handle.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); - } - napi_env env; // Necessary to invoke C++ NAPI callback void* cb_data; // The user provided callback data napi_callback function_or_getter; napi_callback setter; - v8impl::Persistent handle; // Die with this JavaScript object - - private: - static void WeakCallback(v8::WeakCallbackInfo const& info) { - // Use the "WeakCallback mechanism" to delete the C++ `bundle` object. - // This will be called when the v8::External containing `this` pointer - // is being GC-ed. - CallbackBundle* bundle = info.GetParameter(); - delete bundle; - } }; // Base class extended by classes that wrap V8 function and property callback @@ -580,6 +563,11 @@ class SetterCallbackWrapper const v8::Local& _value; }; +static void DeleteCallbackBundle(napi_env env, void* data, void* hint) { + CallbackBundle* bundle = static_cast(data); + delete bundle; +} + // Creates an object to be made available to the static function callback // wrapper, used to retrieve the native callback function and data pointer. static @@ -591,7 +579,7 @@ v8::Local CreateFunctionCallbackData(napi_env env, bundle->cb_data = data; bundle->env = env; v8::Local cbdata = v8::External::New(env->isolate, bundle); - bundle->BindLifecycleTo(env->isolate, cbdata); + Reference::New(env, cbdata, 0, true, DeleteCallbackBundle, bundle, nullptr); return cbdata; }