Skip to content

Commit

Permalink
n-api: delete callback bundle via reference
Browse files Browse the repository at this point in the history
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: #29479
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
Gabriel Schulhof authored and BridgeAR committed Sep 25, 2019
1 parent b1509e8 commit 8474b82
Showing 1 changed file with 6 additions and 18 deletions.
24 changes: 6 additions & 18 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Value> 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<v8::Value> handle; // Die with this JavaScript object

private:
static void WeakCallback(v8::WeakCallbackInfo<CallbackBundle> 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
Expand Down Expand Up @@ -580,6 +563,11 @@ class SetterCallbackWrapper
const v8::Local<v8::Value>& _value;
};

static void DeleteCallbackBundle(napi_env env, void* data, void* hint) {
CallbackBundle* bundle = static_cast<CallbackBundle*>(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
Expand All @@ -591,7 +579,7 @@ v8::Local<v8::Value> CreateFunctionCallbackData(napi_env env,
bundle->cb_data = data;
bundle->env = env;
v8::Local<v8::Value> cbdata = v8::External::New(env->isolate, bundle);
bundle->BindLifecycleTo(env->isolate, cbdata);
Reference::New(env, cbdata, 0, true, DeleteCallbackBundle, bundle, nullptr);

return cbdata;
}
Expand Down

0 comments on commit 8474b82

Please sign in to comment.