Updates for review feedback (nodejs#203)

 - Add an env parameter to napi_finalize
 - Remove unnecessary reinterpret_cast<> calls
 - Remove redundant Maybe.IsNothing() checks
 - Make function call result parameters optional
 - Wrap internal field pointer values in External
 - Fix broken callback tests and add error-checking
 - Other misc test cleanup

Author: jasongin

src/node_api.cc

@@ -90,23 +90,69 @@ v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
   return local;
 }
 
+// Adapter for napi_finalize callbacks.
+class Finalizer {
+ protected:
+  Finalizer(v8::Isolate* isolate,
+             napi_finalize finalize_callback,
+             void* finalize_data,
+             void* finalize_hint)
+    : _isolate(isolate),
+      _finalize_callback(finalize_callback),
+      _finalize_data(finalize_data),
+      _finalize_hint(finalize_hint) {
+  }
+
+  ~Finalizer() {
+  }
+
+ public:
+  static Finalizer* New(v8::Isolate* isolate,
+                        napi_finalize finalize_callback = nullptr,
+                        void* finalize_data = nullptr,
+                        void* finalize_hint = nullptr) {
+    return new Finalizer(
+      isolate, finalize_callback, finalize_data, finalize_hint);
+  }
+
+  static void Delete(Finalizer* finalizer) {
+    delete finalizer;
+  }
+
+  // node::Buffer::FreeCallback
+  static void FinalizeBufferCallback(char* data, void* hint) {
+    Finalizer* finalizer = static_cast<Finalizer*>(hint);
+    if (finalizer->_finalize_callback != nullptr) {
+      finalizer->_finalize_callback(
+        v8impl::JsEnvFromV8Isolate(finalizer->_isolate),
+        data,
+        finalizer->_finalize_hint);
+    }
+
+    Delete(finalizer);
+  }
+
+ protected:
+  v8::Isolate* _isolate;
+  napi_finalize _finalize_callback;
+  void* _finalize_data;
+  void* _finalize_hint;
+};
+
 // Wrapper around v8::Persistent that implements reference counting.
-class Reference {
+class Reference : private Finalizer {
  private:
   Reference(v8::Isolate* isolate,
             v8::Local<v8::Value> value,
             int initial_refcount,
             bool delete_self,
-            napi_finalize finalize_callback = nullptr,
-            void* finalize_data = nullptr,
-            void* finalize_hint = nullptr)
-       : _isolate(isolate),
+            napi_finalize finalize_callback,
+            void* finalize_data,
+            void* finalize_hint)
+       : Finalizer(isolate, finalize_callback, finalize_data, finalize_hint),
         _persistent(isolate, value),
         _refcount(initial_refcount),
-        _delete_self(delete_self),
-        _finalize_callback(finalize_callback),
-        _finalize_data(finalize_data),
-        _finalize_hint(finalize_hint) {
+        _delete_self(delete_self) {
     if (initial_refcount == 0) {
       _persistent.SetWeak(
           this, FinalizeCallback, v8::WeakCallbackType::kParameter);
@@ -181,7 +227,9 @@ class Reference {
     bool delete_self = reference->_delete_self;
 
     if (reference->_finalize_callback != nullptr) {
-      reference->_finalize_callback(reference->_finalize_data,
+      reference->_finalize_callback(
+          v8impl::JsEnvFromV8Isolate(reference->_isolate),
+          reference->_finalize_data,
           reference->_finalize_hint);
     }
 
@@ -190,13 +238,9 @@ class Reference {
     }
   }
 
-  v8::Isolate* _isolate;
   v8::Persistent<v8::Value> _persistent;
   int _refcount;
   bool _delete_self;
-  napi_finalize _finalize_callback;
-  void* _finalize_data;
-  void* _finalize_hint;
 };
 
 class TryCatch : public v8::TryCatch {
@@ -419,7 +463,7 @@ v8::Local<v8::Object> CreateFunctionCallbackData(napi_env env,
       v8::External::New(isolate, reinterpret_cast<void*>(cb)));
   cbdata->SetInternalField(
       v8impl::kDataIndex,
-      v8::External::New(isolate, reinterpret_cast<void*>(data)));
+      v8::External::New(isolate, data));
   return cbdata;
 }
 
@@ -451,7 +495,7 @@ v8::Local<v8::Object> CreateAccessorCallbackData(napi_env env,
 
   cbdata->SetInternalField(
       v8impl::kDataIndex,
-      v8::External::New(isolate, reinterpret_cast<void*>(data)));
+      v8::External::New(isolate, data));
   return cbdata;
 }
 
@@ -1011,9 +1055,7 @@ napi_status napi_define_properties(napi_env env,
       auto define_maybe =
           obj->DefineOwnProperty(context, name, t->GetFunction(), attributes);
 
-      // IsNothing seems like a serious failure,
-      // should we return a different error code if the define failed?
-      if (define_maybe.IsNothing() || !define_maybe.FromMaybe(false)) {
+      if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(napi_generic_failure);
       }
     } else if (p->getter || p->setter) {
@@ -1422,7 +1464,6 @@ napi_status napi_call_function(napi_env env,
                                const napi_value* argv,
                                napi_value* result) {
   NAPI_PREAMBLE(env);
-  CHECK_ARG(result);
 
   v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
@@ -1439,8 +1480,10 @@ napi_status napi_call_function(napi_env env,
   if (try_catch.HasCaught()) {
     return napi_set_last_error(napi_pending_exception);
   } else {
-    CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
-    *result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
+    if (result != nullptr) {
+      CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
+      *result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
+    }
     return napi_ok;
   }
 }
@@ -1773,7 +1816,7 @@ napi_status napi_wrap(napi_env env,
   // via napi_define_class() can be (un)wrapped.
   RETURN_STATUS_IF_FALSE(obj->InternalFieldCount() > 0, napi_invalid_arg);
 
-  obj->SetAlignedPointerInInternalField(0, native_object);
+  obj->SetInternalField(0, v8::External::New(isolate, native_object));
 
   if (result != nullptr) {
     // The returned reference should be deleted via napi_delete_reference()
@@ -1807,7 +1850,7 @@ napi_status napi_unwrap(napi_env env, napi_value js_object, void** result) {
   // via napi_define_class() can be (un)wrapped.
   RETURN_STATUS_IF_FALSE(obj->InternalFieldCount() > 0, napi_invalid_arg);
 
-  *result = obj->GetAlignedPointerFromInternalField(0);
+  *result = v8::Local<v8::External>::Cast(obj->GetInternalField(0))->Value();
 
   return napi_ok;
 }
@@ -2124,17 +2167,20 @@ napi_status napi_make_callback(napi_env env,
                                const napi_value* argv,
                                napi_value* result) {
   NAPI_PREAMBLE(env);
-  CHECK_ARG(result);
 
   v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
   v8::Local<v8::Object> v8recv =
       v8impl::V8LocalValueFromJsValue(recv).As<v8::Object>();
   v8::Local<v8::Function> v8func =
       v8impl::V8LocalValueFromJsValue(func).As<v8::Function>();
 
-  *result = v8impl::JsValueFromV8LocalValue(
-    node::MakeCallback(isolate, v8recv, v8func, argc,
-      reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv))));
+  v8::Local<v8::Value> callback_result = node::MakeCallback(
+      isolate, v8recv, v8func, argc,
+      reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv)));
+
+  if (result != nullptr) {
+    *result = v8impl::JsValueFromV8LocalValue(callback_result);
+  }
 
   return GET_RETURN_STATUS();
 }
@@ -2199,11 +2245,17 @@ napi_status napi_create_external_buffer(napi_env env,
   NAPI_PREAMBLE(env);
   CHECK_ARG(result);
 
-  auto maybe = node::Buffer::New(v8impl::V8IsolateFromJsEnv(env),
+  v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
+
+  // The finalizer object will delete itself after invoking the callback.
+  v8impl::Finalizer* finalizer = v8impl::Finalizer::New(
+    isolate, finalize_cb, nullptr, finalize_hint);
+
+  auto maybe = node::Buffer::New(isolate,
                                  static_cast<char*>(data),
                                  length,
-                                 (node::Buffer::FreeCallback)finalize_cb,
-                                 finalize_hint);
+                                 v8impl::Finalizer::FinalizeBufferCallback,
+                                 finalizer);
 
   CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
 

src/node_api_types.h

@@ -17,8 +17,11 @@ typedef struct napi_handle_scope__ *napi_handle_scope;
 typedef struct napi_escapable_handle_scope__ *napi_escapable_handle_scope;
 typedef struct napi_callback_info__ *napi_callback_info;
 
-typedef void (*napi_callback)(napi_env, napi_callback_info);
-typedef void (*napi_finalize)(void* finalize_data, void* finalize_hint);
+typedef void (*napi_callback)(napi_env env,
+                              napi_callback_info info);
+typedef void (*napi_finalize)(napi_env env,
+                              void* finalize_data,
+                              void* finalize_hint);
 
 typedef enum {
   napi_default = 0,

test/addons-napi/3_callbacks/binding.c

@@ -1,38 +1,36 @@
 #include <node_api.h>
 
-void RunCallback(napi_env env, const napi_callback_info info) {
-  napi_status status;
-
+#define NAPI_CALL(theCall)                                                \
+  if (theCall != napi_ok) {                                               \
+    const char* errorMessage = napi_get_last_error_info()->error_message; \
+    errorMessage = errorMessage ? errorMessage : "empty error message";   \
+    napi_throw_error((env), errorMessage);                                \
+    return;                                                               \
+  }
+
+void RunCallback(napi_env env, napi_callback_info info) {
   napi_value args[1];
-  status = napi_get_cb_args(env, info, args, 1);
-  if (status != napi_ok) return;
+  NAPI_CALL(napi_get_cb_args(env, info, args, 1));
 
   napi_value cb = args[0];
 
   napi_value argv[1];
-  status = napi_create_string_utf8(env, "hello world", -1, argv);
-  if (status != napi_ok) return;
+  NAPI_CALL(napi_create_string_utf8(env, "hello world", -1, argv));
 
   napi_value global;
-  status = napi_get_global(env, &global);
-  if (status != napi_ok) return;
+  NAPI_CALL(napi_get_global(env, &global));
 
-  status = napi_call_function(env, global, cb, 1, argv, NULL);
-  if (status != napi_ok) return;
+  NAPI_CALL(napi_call_function(env, global, cb, 1, argv, NULL));
 }
 
-void RunCallbackWithRecv(napi_env env, const napi_callback_info info) {
-  napi_status status;
-
+void RunCallbackWithRecv(napi_env env, napi_callback_info info) {
   napi_value args[2];
-  status = napi_get_cb_args(env, info, args, 2);
-  if (status != napi_ok) return;
+  NAPI_CALL(napi_get_cb_args(env, info, args, 2));
 
   napi_value cb = args[0];
   napi_value recv = args[1];
 
-  status = napi_call_function(env, recv, cb, 0, NULL, NULL);
-  if (status != napi_ok) return;
+  NAPI_CALL(napi_call_function(env, recv, cb, 0, NULL, NULL));
 }
 
 #define DECLARE_NAPI_METHOD(name, func)                          \

test/addons-napi/3_callbacks/test.js

@@ -7,14 +7,9 @@ addon.RunCallback(function(msg) {
   assert.strictEqual(msg, 'hello world');
 });
 
-const global = function() { return this; }.apply();
-
 function testRecv(desiredRecv) {
   addon.RunCallbackWithRecv(function() {
-    assert.strictEqual(this,
-                       (desiredRecv === undefined ||
-                        desiredRecv === null) ?
-                       global : desiredRecv);
+    assert.strictEqual(this, desiredRecv);
   }, desiredRecv);
 }
 

test/addons-napi/4_object_factory/binding.c

@@ -1,6 +1,6 @@
 #include <node_api.h>
 
-void CreateObject(napi_env env, const napi_callback_info info) {
+void CreateObject(napi_env env, napi_callback_info info) {
   napi_status status;
 
   napi_value args[1];

test/addons-napi/4_object_factory/test.js

@@ -5,4 +5,4 @@ const addon = require(`./build/${common.buildType}/binding`);
 
 const obj1 = addon('hello');
 const obj2 = addon('world');
-assert.strictEqual(obj1.msg + ' ' + obj2.msg, 'hello world');
+assert.strictEqual(`${obj1.msg} ${obj2.msg}`, 'hello world');

test/addons-napi/6_object_wrap/myobject.cc

@@ -7,8 +7,10 @@ MyObject::MyObject(double value)
 
 MyObject::~MyObject() { napi_delete_reference(env_, wrapper_); }
 
-void MyObject::Destructor(void* nativeObject, void* /*finalize_hint*/) {
-  reinterpret_cast<MyObject*>(nativeObject)->~MyObject();
+void MyObject::Destructor(
+  napi_env env, void* nativeObject, void* /*finalize_hint*/) {
+  MyObject* obj = static_cast<MyObject*>(nativeObject);
+  delete obj;
 }
 
 #define DECLARE_NAPI_METHOD(name, func)                          \
@@ -67,7 +69,7 @@ void MyObject::New(napi_env env, napi_callback_info info) {
     obj->env_ = env;
     status = napi_wrap(env,
                        jsthis,
-                       reinterpret_cast<void*>(obj),
+                       obj,
                        MyObject::Destructor,
                        nullptr,  // finalize_hint
                        &obj->wrapper_);

test/addons-napi/6_object_wrap/myobject.h

@@ -6,7 +6,7 @@
 class MyObject {
  public:
   static void Init(napi_env env, napi_value exports);
-  static void Destructor(void* nativeObject, void* finalize_hint);
+  static void Destructor(napi_env env, void* nativeObject, void* finalize_hint);
 
  private:
   explicit MyObject(double value_ = 0);

test/addons-napi/7_factory_wrap/myobject.cc

@@ -4,8 +4,11 @@ MyObject::MyObject() : env_(nullptr), wrapper_(nullptr) {}
 
 MyObject::~MyObject() { napi_delete_reference(env_, wrapper_); }
 
-void MyObject::Destructor(void* nativeObject, void* /*finalize_hint*/) {
-  reinterpret_cast<MyObject*>(nativeObject)->~MyObject();
+void MyObject::Destructor(napi_env env,
+                          void* nativeObject,
+                          void* /*finalize_hint*/) {
+  MyObject* obj = static_cast<MyObject*>(nativeObject);
+  delete obj;
 }
 
 #define DECLARE_NAPI_METHOD(name, func)                          \
@@ -57,7 +60,7 @@ void MyObject::New(napi_env env, napi_callback_info info) {
   obj->env_ = env;
   status = napi_wrap(env,
                      jsthis,
-                     reinterpret_cast<void*>(obj),
+                     obj,
                      MyObject::Destructor,
                      nullptr, /* finalize_hint */
                      &obj->wrapper_);

test/addons-napi/7_factory_wrap/myobject.h

@@ -6,7 +6,7 @@
 class MyObject {
  public:
   static napi_status Init(napi_env env);
-  static void Destructor(void* nativeObject, void* /*finalize_hint*/);
+  static void Destructor(napi_env env, void* nativeObject, void* finalize_hint);
   static napi_status NewInstance(napi_env env,
                                  napi_value arg,
                                  napi_value* instance);