Fix the isWeakRef() C++ check (TooTallNate#88)

* Add currently-failing test case for issue TooTallNate#83.

* Add a test to make sure that `isWeakRef()` works as expected.

This also effectively checks the low-level native code for same, which is used
extensively.

* Make the use of internal fields more "symbolic."

* Add a second internal field to weak ref proxies.

This is used to hold a pointer which is unique to the module, and is checked
as part of the process of determining whether a value is a weak ref, in
`isWeakRef()`.

This additional check will work assuming the lack of ill intent of other
native code, but it is susceptible to "gaming" by malicious native code. That
said, it is nonetheless an improvement in correctness over the old version of
the check, and all bets are off with native code anyway.

* Better guard.

src/weakref.cc

@@ -34,11 +34,24 @@ Nan::Persistent<ObjectTemplate> proxyClass;
 
 Nan::Callback *globalCallback;
 
+// Field index used to store the container object.
+#define FIELD_INDEX_CONTAINER (0)
+
+// Field index used to store the unique pointer value used to identify objects
+// as instances created by this module).
+#define FIELD_INDEX_IDENT (1)
+
+// Count of internal fields used by instances created by this module.
+#define FIELD_COUNT (2)
+
+// The value stored at `FIELD_INDEX_IDENT`, which is designed to be unique to
+// this module, barring intentional mischief from other native code.
+#define IDENT_VALUE ((void *) &proxyClass)
 
 bool IsDead(Local<Object> proxy) {
-  assert(proxy->InternalFieldCount() == 1);
+  assert(proxy->InternalFieldCount() == FIELD_COUNT);
   proxy_container *cont = reinterpret_cast<proxy_container*>(
-    Nan::GetInternalFieldPointer(proxy, 0)
+    Nan::GetInternalFieldPointer(proxy, FIELD_INDEX_CONTAINER)
   );
   return cont == NULL || cont->target.IsEmpty();
 }
@@ -47,15 +60,15 @@ bool IsDead(Local<Object> proxy) {
 Local<Object> Unwrap(Local<Object> proxy) {
   assert(!IsDead(proxy));
   proxy_container *cont = reinterpret_cast<proxy_container*>(
-    Nan::GetInternalFieldPointer(proxy, 0)
+    Nan::GetInternalFieldPointer(proxy, FIELD_INDEX_CONTAINER)
   );
   Local<Object> _target = Nan::New<Object>(cont->target);
   return _target;
 }
 
 Local<Object> GetEmitter(Local<Object> proxy) {
   proxy_container *cont = reinterpret_cast<proxy_container*>(
-    Nan::GetInternalFieldPointer(proxy, 0)
+    Nan::GetInternalFieldPointer(proxy, FIELD_INDEX_CONTAINER)
   );
   assert(cont != NULL);
   Local<Object> _emitter = Nan::New<Object>(cont->emitter);
@@ -68,7 +81,6 @@ Local<Object> GetEmitter(Local<Object> proxy) {
   const bool dead = IsDead(info.This());  \
   if (!dead) obj = Unwrap(info.This());   \
 
-
 NAN_PROPERTY_GETTER(WeakNamedPropertyGetter) {
   UNWRAP
   info.GetReturnValue().Set(dead ? Local<Value>() : Nan::Get(obj, property).ToLocalChecked());
@@ -148,7 +160,7 @@ static void TargetCallback(const Nan::WeakCallbackInfo<proxy_container> &info) {
 
   // clean everything up
   Local<Object> proxy = Nan::New<Object>(cont->proxy);
-  Nan::SetInternalFieldPointer(proxy, 0, NULL);
+  Nan::SetInternalFieldPointer(proxy, FIELD_INDEX_CONTAINER, NULL);
   cont->proxy.Reset();
   cont->emitter.Reset();
   delete cont;
@@ -169,19 +181,23 @@ NAN_METHOD(Create) {
   cont->proxy.Reset(proxy);
   cont->emitter.Reset(_emitter);
   cont->target.Reset(_target);
-  Nan::SetInternalFieldPointer(proxy, 0, cont);
+  Nan::SetInternalFieldPointer(proxy, FIELD_INDEX_CONTAINER, cont);
+  Nan::SetInternalFieldPointer(proxy, FIELD_INDEX_IDENT, IDENT_VALUE);
 
   cont->target.SetWeak(cont, TargetCallback, Nan::WeakCallbackType::kParameter);
 
   info.GetReturnValue().Set(proxy);
 }
 
-/**
- * TODO: Make this better.
- */
-
 bool isWeakRef (Local<Value> val) {
-  return val->IsObject() && val.As<Object>()->InternalFieldCount() == 1;
+  if (!val->IsObject()) {
+    return false;
+  }
+
+  Local<Object> obj = val.As<Object>();
+
+  return obj->InternalFieldCount() == FIELD_COUNT
+    && Nan::GetInternalFieldPointer(obj, FIELD_INDEX_IDENT) == IDENT_VALUE;
 }
 
 /**
@@ -216,7 +232,7 @@ NAN_METHOD(IsNearDeath) {
   WEAKREF_FIRST_ARG
 
   proxy_container *cont = reinterpret_cast<proxy_container*>(
-    Nan::GetInternalFieldPointer(proxy, 0)
+    Nan::GetInternalFieldPointer(proxy, FIELD_INDEX_CONTAINER)
   );
   assert(cont != NULL);
 
@@ -273,7 +289,7 @@ NAN_MODULE_INIT(Initialize) {
                                  WeakIndexedPropertyQuery,
                                  WeakIndexedPropertyDeleter,
                                  WeakPropertyEnumerator);
-  p->SetInternalFieldCount(1);
+  p->SetInternalFieldCount(FIELD_COUNT);
 
   Nan::SetMethod(target, "get", Get);
   Nan::SetMethod(target, "isWeakRef", IsWeakRef);

test/create.js

@@ -20,4 +20,9 @@ describe('create()', function () {
     })
   })
 
+  it('should acknowledge the weakness of created values', function () {
+    var ref = weak.create([]);
+    assert(weak.isWeakRef(ref));
+  })
+
 })

test/promise.js

@@ -0,0 +1,14 @@
+var assert = require('assert');
+var weak = require('../')
+
+describe('promise', function () {
+  if (!global.Promise) {
+    // Version of JS without `Promise`; nothing to do.
+    return;
+  }
+
+  it('should not mistake a promise for a weak reference', function () {
+    var p = new Promise(function () {});
+    assert(!weak.isWeakRef(p));
+  });
+})