n-api: clean up binding creation

* Remove dead code for `GetterCallbackWrapper` and
  `SetterCallbackWrapper`.
* Factor out creation of new `v8::Function`s.
* Factor out creation of new `v8::FunctionTemplate`s.
* Turn `CallbackBundle` into a class, internalizing creation of new
  instances and garbage collection.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: #36170
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

Author: Gabriel Schulhof

src/js_native_api_v8.cc

@@ -418,18 +418,36 @@ inline static napi_status Unwrap(napi_env env,
 // calling through N-API.
 // Ref: benchmark/misc/function_call
 // Discussion (incl. perf. data): https://github.com/nodejs/node/pull/21072
-struct CallbackBundle {
+class CallbackBundle {
+ public:
+  // Creates an object to be made available to the static function callback
+  // wrapper, used to retrieve the native callback function and data pointer.
+  static inline v8::Local<v8::Value>
+  New(napi_env env, napi_callback cb, void* data) {
+    CallbackBundle* bundle = new CallbackBundle();
+    bundle->cb = cb;
+    bundle->cb_data = data;
+    bundle->env = env;
+
+    v8::Local<v8::Value> cbdata = v8::External::New(env->isolate, bundle);
+    Reference::New(env, cbdata, 0, true, Delete, bundle, nullptr);
+    return cbdata;
+  }
   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;
+  napi_callback  cb;
+ private:
+  static void Delete(napi_env env, void* data, void* hint) {
+    CallbackBundle* bundle = static_cast<CallbackBundle*>(data);
+    delete bundle;
+  }
 };
 
 // Base class extended by classes that wrap V8 function and property callback
 // info.
 class CallbackWrapper {
  public:
-  CallbackWrapper(napi_value this_arg, size_t args_length, void* data)
+  inline CallbackWrapper(napi_value this_arg, size_t args_length, void* data)
       : _this(this_arg), _args_length(args_length), _data(data) {}
 
   virtual napi_value GetNewTarget() = 0;
@@ -448,10 +466,10 @@ class CallbackWrapper {
   void* _data;
 };
 
-template <typename Info, napi_callback CallbackBundle::*FunctionField>
 class CallbackWrapperBase : public CallbackWrapper {
  public:
-  CallbackWrapperBase(const Info& cbinfo, const size_t args_length)
+  inline CallbackWrapperBase(const v8::FunctionCallbackInfo<v8::Value>& cbinfo,
+                             const size_t args_length)
       : CallbackWrapper(JsValueFromV8LocalValue(cbinfo.This()),
                         args_length,
                         nullptr),
@@ -461,16 +479,14 @@ class CallbackWrapperBase : public CallbackWrapper {
     _data = _bundle->cb_data;
   }
 
-  napi_value GetNewTarget() override { return nullptr; }
-
  protected:
-  void InvokeCallback() {
+  inline void InvokeCallback() {
     napi_callback_info cbinfo_wrapper = reinterpret_cast<napi_callback_info>(
         static_cast<CallbackWrapper*>(this));
 
     // All other pointers we need are stored in `_bundle`
     napi_env env = _bundle->env;
-    napi_callback cb = _bundle->*FunctionField;
+    napi_callback cb = _bundle->cb;
 
     napi_value result;
     env->CallIntoModule([&](napi_env env) {
@@ -482,19 +498,45 @@ class CallbackWrapperBase : public CallbackWrapper {
     }
   }
 
-  const Info& _cbinfo;
+  const v8::FunctionCallbackInfo<v8::Value>& _cbinfo;
   CallbackBundle* _bundle;
 };
 
 class FunctionCallbackWrapper
-    : public CallbackWrapperBase<v8::FunctionCallbackInfo<v8::Value>,
-                                 &CallbackBundle::function_or_getter> {
+    : public CallbackWrapperBase {
  public:
   static void Invoke(const v8::FunctionCallbackInfo<v8::Value>& info) {
     FunctionCallbackWrapper cbwrapper(info);
     cbwrapper.InvokeCallback();
   }
 
+  static inline napi_status NewFunction(napi_env env,
+                                        napi_callback cb,
+                                        void* cb_data,
+                                        v8::Local<v8::Function>* result) {
+    v8::Local<v8::Value> cbdata = v8impl::CallbackBundle::New(env, cb, cb_data);
+    RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
+
+    v8::MaybeLocal<v8::Function> maybe_function =
+        v8::Function::New(env->context(), Invoke, cbdata);
+    CHECK_MAYBE_EMPTY(env, maybe_function, napi_generic_failure);
+
+    *result = maybe_function.ToLocalChecked();
+    return napi_clear_last_error(env);
+  }
+
+  static inline napi_status NewTemplate(napi_env env,
+                    napi_callback cb,
+                    void* cb_data,
+                    v8::Local<v8::FunctionTemplate>* result,
+                    v8::Local<v8::Signature> sig = v8::Local<v8::Signature>()) {
+    v8::Local<v8::Value> cbdata = v8impl::CallbackBundle::New(env, cb, cb_data);
+    RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
+
+    *result = v8::FunctionTemplate::New(env->isolate, Invoke, cbdata, sig);
+    return napi_clear_last_error(env);
+  }
+
   explicit FunctionCallbackWrapper(
       const v8::FunctionCallbackInfo<v8::Value>& cbinfo)
       : CallbackWrapperBase(cbinfo, cbinfo.Length()) {}
@@ -532,98 +574,6 @@ class FunctionCallbackWrapper
   }
 };
 
-class GetterCallbackWrapper
-    : public CallbackWrapperBase<v8::PropertyCallbackInfo<v8::Value>,
-                                 &CallbackBundle::function_or_getter> {
- public:
-  static void Invoke(v8::Local<v8::Name> property,
-                     const v8::PropertyCallbackInfo<v8::Value>& info) {
-    GetterCallbackWrapper cbwrapper(info);
-    cbwrapper.InvokeCallback();
-  }
-
-  explicit GetterCallbackWrapper(
-      const v8::PropertyCallbackInfo<v8::Value>& cbinfo)
-      : CallbackWrapperBase(cbinfo, 0) {}
-
-  /*virtual*/
-  void Args(napi_value* buffer, size_t buffer_length) override {
-    if (buffer_length > 0) {
-      napi_value undefined =
-          v8impl::JsValueFromV8LocalValue(v8::Undefined(_cbinfo.GetIsolate()));
-      for (size_t i = 0; i < buffer_length; i += 1) {
-        buffer[i] = undefined;
-      }
-    }
-  }
-
-  /*virtual*/
-  void SetReturnValue(napi_value value) override {
-    v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
-    _cbinfo.GetReturnValue().Set(val);
-  }
-};
-
-class SetterCallbackWrapper
-    : public CallbackWrapperBase<v8::PropertyCallbackInfo<void>,
-                                 &CallbackBundle::setter> {
- public:
-  static void Invoke(v8::Local<v8::Name> property,
-                     v8::Local<v8::Value> value,
-                     const v8::PropertyCallbackInfo<void>& info) {
-    SetterCallbackWrapper cbwrapper(info, value);
-    cbwrapper.InvokeCallback();
-  }
-
-  SetterCallbackWrapper(const v8::PropertyCallbackInfo<void>& cbinfo,
-                        const v8::Local<v8::Value>& value)
-      : CallbackWrapperBase(cbinfo, 1), _value(value) {}
-
-  /*virtual*/
-  void Args(napi_value* buffer, size_t buffer_length) override {
-    if (buffer_length > 0) {
-      buffer[0] = v8impl::JsValueFromV8LocalValue(_value);
-
-      if (buffer_length > 1) {
-        napi_value undefined = v8impl::JsValueFromV8LocalValue(
-            v8::Undefined(_cbinfo.GetIsolate()));
-        for (size_t i = 1; i < buffer_length; i += 1) {
-          buffer[i] = undefined;
-        }
-      }
-    }
-  }
-
-  /*virtual*/
-  void SetReturnValue(napi_value value) override {
-    // Ignore any value returned from a setter callback.
-  }
-
- private:
-  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
-v8::Local<v8::Value> CreateFunctionCallbackData(napi_env env,
-                                                napi_callback cb,
-                                                void* data) {
-  CallbackBundle* bundle = new CallbackBundle();
-  bundle->function_or_getter = cb;
-  bundle->cb_data = data;
-  bundle->env = env;
-  v8::Local<v8::Value> cbdata = v8::External::New(env->isolate, bundle);
-  Reference::New(env, cbdata, 0, true, DeleteCallbackBundle, bundle, nullptr);
-
-  return cbdata;
-}
-
 enum WrapType {
   retrievable,
   anonymous
@@ -745,22 +695,12 @@ napi_status napi_create_function(napi_env env,
   CHECK_ARG(env, result);
   CHECK_ARG(env, cb);
 
-  v8::Isolate* isolate = env->isolate;
   v8::Local<v8::Function> return_value;
-  v8::EscapableHandleScope scope(isolate);
-  v8::Local<v8::Value> cbdata =
-      v8impl::CreateFunctionCallbackData(env, cb, callback_data);
-
-  RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
-
-  v8::Local<v8::Context> context = env->context();
-  v8::MaybeLocal<v8::Function> maybe_function =
-      v8::Function::New(context,
-                        v8impl::FunctionCallbackWrapper::Invoke,
-                        cbdata);
-  CHECK_MAYBE_EMPTY(env, maybe_function, napi_generic_failure);
-
-  return_value = scope.Escape(maybe_function.ToLocalChecked());
+  v8::EscapableHandleScope scope(env->isolate);
+  v8::Local<v8::Function> fn;
+  STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction(
+      env, cb, callback_data, &fn));
+  return_value = scope.Escape(fn);
 
   if (utf8name != nullptr) {
     v8::Local<v8::String> name_string;
@@ -792,13 +732,9 @@ napi_status napi_define_class(napi_env env,
   v8::Isolate* isolate = env->isolate;
 
   v8::EscapableHandleScope scope(isolate);
-  v8::Local<v8::Value> cbdata =
-      v8impl::CreateFunctionCallbackData(env, constructor, callback_data);
-
-  RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
-
-  v8::Local<v8::FunctionTemplate> tpl = v8::FunctionTemplate::New(
-      isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
+  v8::Local<v8::FunctionTemplate> tpl;
+  STATUS_CALL(v8impl::FunctionCallbackWrapper::NewTemplate(
+      env, constructor, callback_data, &tpl));
 
   v8::Local<v8::String> name_string;
   CHECK_NEW_FROM_UTF8_LEN(env, name_string, utf8name, length);
@@ -828,18 +764,12 @@ napi_status napi_define_class(napi_env env,
       v8::Local<v8::FunctionTemplate> getter_tpl;
       v8::Local<v8::FunctionTemplate> setter_tpl;
       if (p->getter != nullptr) {
-        v8::Local<v8::Value> getter_data =
-            v8impl::CreateFunctionCallbackData(env, p->getter, p->data);
-
-        getter_tpl = v8::FunctionTemplate::New(
-            isolate, v8impl::FunctionCallbackWrapper::Invoke, getter_data);
+        STATUS_CALL(v8impl::FunctionCallbackWrapper::NewTemplate(
+            env, p->getter, p->data, &getter_tpl));
       }
       if (p->setter != nullptr) {
-        v8::Local<v8::Value> setter_data =
-            v8impl::CreateFunctionCallbackData(env, p->setter, p->data);
-
-        setter_tpl = v8::FunctionTemplate::New(
-            isolate, v8impl::FunctionCallbackWrapper::Invoke, setter_data);
+        STATUS_CALL(v8impl::FunctionCallbackWrapper::NewTemplate(
+            env, p->setter, p->data, &setter_tpl));
       }
 
       tpl->PrototypeTemplate()->SetAccessorProperty(
@@ -849,16 +779,9 @@ napi_status napi_define_class(napi_env env,
         attributes,
         v8::AccessControl::DEFAULT);
     } else if (p->method != nullptr) {
-      v8::Local<v8::Value> cbdata =
-          v8impl::CreateFunctionCallbackData(env, p->method, p->data);
-
-      RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
-
-      v8::Local<v8::FunctionTemplate> t =
-        v8::FunctionTemplate::New(isolate,
-          v8impl::FunctionCallbackWrapper::Invoke,
-          cbdata,
-          v8::Signature::New(isolate, tpl));
+      v8::Local<v8::FunctionTemplate> t;
+      STATUS_CALL(v8impl::FunctionCallbackWrapper::NewTemplate(
+          env, p->method, p->data, &t, v8::Signature::New(isolate, tpl)));
 
       tpl->PrototypeTemplate()->Set(property_name, t, attributes);
     } else {
@@ -1263,33 +1186,16 @@ napi_status napi_define_properties(napi_env env,
     STATUS_CALL(v8impl::V8NameFromPropertyDescriptor(env, p, &property_name));
 
     if (p->getter != nullptr || p->setter != nullptr) {
-      v8::Local<v8::Value> local_getter;
-      v8::Local<v8::Value> local_setter;
+      v8::Local<v8::Function> local_getter;
+      v8::Local<v8::Function> local_setter;
 
       if (p->getter != nullptr) {
-        v8::Local<v8::Value> getter_data =
-            v8impl::CreateFunctionCallbackData(env, p->getter, p->data);
-        CHECK_MAYBE_EMPTY(env, getter_data, napi_generic_failure);
-
-        v8::MaybeLocal<v8::Function> maybe_getter =
-            v8::Function::New(context,
-                              v8impl::FunctionCallbackWrapper::Invoke,
-                              getter_data);
-        CHECK_MAYBE_EMPTY(env, maybe_getter, napi_generic_failure);
-
-        local_getter = maybe_getter.ToLocalChecked();
+        STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction(
+            env, p->getter, p->data, &local_getter));
       }
       if (p->setter != nullptr) {
-        v8::Local<v8::Value> setter_data =
-            v8impl::CreateFunctionCallbackData(env, p->setter, p->data);
-        CHECK_MAYBE_EMPTY(env, setter_data, napi_generic_failure);
-
-        v8::MaybeLocal<v8::Function> maybe_setter =
-            v8::Function::New(context,
-                              v8impl::FunctionCallbackWrapper::Invoke,
-                              setter_data);
-        CHECK_MAYBE_EMPTY(env, maybe_setter, napi_generic_failure);
-        local_setter = maybe_setter.ToLocalChecked();
+        STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction(
+            env, p->setter, p->data, &local_setter));
       }
 
       v8::PropertyDescriptor descriptor(local_getter, local_setter);
@@ -1304,19 +1210,10 @@ napi_status napi_define_properties(napi_env env,
         return napi_set_last_error(env, napi_invalid_arg);
       }
     } else if (p->method != nullptr) {
-      v8::Local<v8::Value> cbdata =
-          v8impl::CreateFunctionCallbackData(env, p->method, p->data);
-
-      CHECK_MAYBE_EMPTY(env, cbdata, napi_generic_failure);
-
-      v8::MaybeLocal<v8::Function> maybe_fn =
-          v8::Function::New(context,
-                            v8impl::FunctionCallbackWrapper::Invoke,
-                            cbdata);
-
-      CHECK_MAYBE_EMPTY(env, maybe_fn, napi_generic_failure);
-
-      v8::PropertyDescriptor descriptor(maybe_fn.ToLocalChecked(),
+      v8::Local<v8::Function> method;
+      STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction(
+          env, p->method, p->data, &method));
+      v8::PropertyDescriptor descriptor(method,
                                         (p->attributes & napi_writable) != 0);
       descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
       descriptor.set_configurable((p->attributes & napi_configurable) != 0);