Refcount for v8impl::Reference should be unsigned (nodejs#207)

* Refcount for v8impl::Reference should be unsigned

Add ```Reference::RefCount()``` and check the refcount value before trying to call ```Reference::Unref()``` to avoid underflowing the refcount. This allows us to continue returning an error from ```napi_reference_unref``` if it is called with a reference that already has refcount set to zero.

* Add check for _refcount == 0 in Reference::Unref to avoid an underflow

Author: boingoing

src/node_api.cc

@@ -144,7 +144,7 @@ class Reference : private Finalizer {
  private:
   Reference(v8::Isolate* isolate,
             v8::Local<v8::Value> value,
-            int initial_refcount,
+            uint32_t initial_refcount,
             bool delete_self,
             napi_finalize finalize_callback,
             void* finalize_data,
@@ -173,7 +173,7 @@ class Reference : private Finalizer {
  public:
   static Reference* New(v8::Isolate* isolate,
                         v8::Local<v8::Value> value,
-                        int initial_refcount,
+                        uint32_t initial_refcount,
                         bool delete_self,
                         napi_finalize finalize_callback = nullptr,
                         void* finalize_data = nullptr,
@@ -191,15 +191,18 @@ class Reference : private Finalizer {
     delete reference;
   }
 
-  int Ref() {
+  uint32_t Ref() {
     if (++_refcount == 1) {
       _persistent.ClearWeak();
     }
 
     return _refcount;
   }
 
-  int Unref() {
+  uint32_t Unref() {
+    if (_refcount == 0) {
+        return 0;
+    }
     if (--_refcount == 0) {
       _persistent.SetWeak(
           this, FinalizeCallback, v8::WeakCallbackType::kParameter);
@@ -209,6 +212,10 @@ class Reference : private Finalizer {
     return _refcount;
   }
 
+  uint32_t RefCount() {
+    return _refcount;
+  }
+
   v8::Local<v8::Value> Get() {
     if (_persistent.IsEmpty()) {
       return v8::Local<v8::Value>();
@@ -239,7 +246,7 @@ class Reference : private Finalizer {
   }
 
   v8::Persistent<v8::Value> _persistent;
-  int _refcount;
+  uint32_t _refcount;
   bool _delete_self;
 };
 
@@ -1901,11 +1908,10 @@ napi_status napi_get_value_external(napi_env env,
 // Set initial_refcount to 0 for a weak reference, >0 for a strong reference.
 napi_status napi_create_reference(napi_env env,
                                   napi_value value,
-                                  int initial_refcount,
+                                  uint32_t initial_refcount,
                                   napi_ref* result) {
   NAPI_PREAMBLE(env);
   CHECK_ARG(result);
-  RETURN_STATUS_IF_FALSE(initial_refcount >= 0, napi_invalid_arg);
 
   v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
 
@@ -1933,12 +1939,12 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) {
 // refcount is >0, and the referenced object is effectively "pinned".
 // Calling this when the refcount is 0 and the object is unavailable
 // results in an error.
-napi_status napi_reference_ref(napi_env env, napi_ref ref, int* result) {
+napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) {
   NAPI_PREAMBLE(env);
   CHECK_ARG(ref);
 
   v8impl::Reference* reference = reinterpret_cast<v8impl::Reference*>(ref);
-  int count = reference->Ref();
+  uint32_t count = reference->Ref();
 
   if (result != nullptr) {
     *result = count;
@@ -1951,16 +1957,18 @@ napi_status napi_reference_ref(napi_env env, napi_ref ref, int* result) {
 // the result is 0 the reference is now weak and the object may be GC'd at any
 // time if there are no other references. Calling this when the refcount is
 // already 0 results in an error.
-napi_status napi_reference_unref(napi_env env, napi_ref ref, int* result) {
+napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) {
   NAPI_PREAMBLE(env);
   CHECK_ARG(ref);
 
   v8impl::Reference* reference = reinterpret_cast<v8impl::Reference*>(ref);
-  int count = reference->Unref();
-  if (count < 0) {
+
+  if (reference->RefCount() == 0) {
     return napi_set_last_error(napi_generic_failure);
   }
 
+  uint32_t count = reference->Unref();
+
   if (result != nullptr) {
     *result = count;
   }

src/node_api.h

@@ -352,7 +352,7 @@ NAPI_EXTERN napi_status napi_get_value_external(napi_env env,
 // Set initial_refcount to 0 for a weak reference, >0 for a strong reference.
 NAPI_EXTERN napi_status napi_create_reference(napi_env env,
                                               napi_value value,
-                                              int initial_refcount,
+                                              uint32_t initial_refcount,
                                               napi_ref* result);
 
 // Deletes a reference. The referenced value is released, and may
@@ -366,15 +366,15 @@ NAPI_EXTERN napi_status napi_delete_reference(napi_env env, napi_ref ref);
 // results in an error.
 NAPI_EXTERN napi_status napi_reference_ref(napi_env env,
                                            napi_ref ref,
-                                           int* result);
+                                           uint32_t* result);
 
 // Decrements the reference count, optionally returning the resulting count.
 // If the result is 0 the reference is now weak and the object may be GC'd
 // at any time if there are no other references. Calling this when the
 // refcount is already 0 results in an error.
 NAPI_EXTERN napi_status napi_reference_unref(napi_env env,
                                              napi_ref ref,
-                                             int* result);
+                                             uint32_t* result);
 
 // Attempts to get a referenced value. If the reference is weak,
 // the value might no longer be available, in that case the call