From 8ab8c33985b273cd682842a26091da53ce3c3048 Mon Sep 17 00:00:00 2001
From: Jason Ginchereau <jasongin@microsoft.com>
Date: Thu, 25 May 2017 17:05:21 -0700
Subject: [PATCH] n-api: enable napi_wrap() to work with any object

Previously, napi_wrap() would only work with objects created from a
constructor returned by napi_define_class(). While the N-API team
was aware of this limitation, it was not clearly documented and is
likely to cause confusion anyway. It's much simpler if addons are
allowed to use any JS object. Also, the specific behavior of the
limitation is difficult to reimplement on other VMs that work
differently from V8.

V8 requires object internal fields to be declared on the object
prototype (which napi_define_class() used to do). Since it's too
late to modify the object prototype by the time napi_wrap() is
called, napi_wrap() now inserts a new object (with the internal
field) into the supplied object's prototype chain. Then it can be
retrieved from there later by napi_unwrap().

This change also includes improvements to the documentation for
napi_create_external(), partly to explain how it is different from
napi_wrap().

PR-URL: https://github.com/nodejs/node/pull/13250
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
---
 doc/api/n-api.md                           | 57 ++++++++-----
 src/node_api.cc                            | 42 ++++++----
 test/addons-napi/test_object/test.js       | 98 +++++++++++++++-------
 test/addons-napi/test_object/test_object.c | 27 ++++++
 4 files changed, 159 insertions(+), 65 deletions(-)

diff --git a/doc/api/n-api.md b/doc/api/n-api.md
index d1fb432d7d..fe3786aaaf 100644
--- a/doc/api/n-api.md
+++ b/doc/api/n-api.md
@@ -1059,20 +1059,24 @@ napi_status napi_create_external(napi_env env,
 ```
 
 - `[in] env`: The environment that the API is invoked under.
-- `[in] data`: Raw pointer to the external data being wrapped.
-- `[in] finalize_cb`: Optional callback to call when the wrapped object
+- `[in] data`: Raw pointer to the external data.
+- `[in] finalize_cb`: Optional callback to call when the external value
 is being collected.
 - `[in] finalize_hint`: Optional hint to pass to the finalize callback
 during collection.
-- `[out] result`: A `napi_value` representing an external object.
+- `[out] result`: A `napi_value` representing an external value.
 
 Returns `napi_ok` if the API succeeded.
 
-This API allocates a JavaScript object with external data attached to it.
-This is used to wrap native objects and project them into JavaScript.
-The API allows the caller to pass in a finalize callback, in case the
-underlying native resource needs to be cleaned up when the wrapper
-JavaScript object gets collected.
+This API allocates a JavaScript value with external data attached to it. This
+is used to pass external data through JavaScript code, so it can be retrieved
+later by native code. The API allows the caller to pass in a finalize callback,
+in case the underlying native resource needs to be cleaned up when the external
+JavaScript value gets collected.
+
+*Note*: The created value is not an object, and therefore does not support
+additional properties. It is considered a distinct value type: calling
+`napi_typeof()` with an external value yields `napi_external`.
 
 #### napi_create_external_arraybuffer
 <!-- YAML
@@ -1364,7 +1368,8 @@ Returns `napi_ok` if the API succeeded.
 
 This API is used to retrieve the underlying data buffer of an ArrayBuffer and
 its length.
-WARNING: Use caution while using this API. The lifetime of the underlying data
+
+*WARNING*: Use caution while using this API. The lifetime of the underlying data
 buffer is managed by the ArrayBuffer even after it's returned. A
 possible safe way to use this API is in conjunction with [`napi_create_reference`][],
 which can be used to guarantee control over the lifetime of the
@@ -1391,7 +1396,8 @@ Returns `napi_ok` if the API succeeded.
 
 This API is used to retrieve the underlying data buffer of a `node::Buffer`
 and it's length.
-Warning: Use caution while using this API since the underlying data buffer's
+
+*Warning*: Use caution while using this API since the underlying data buffer's
 lifetime is not guaranteed if it's managed by the VM.
 
 #### *napi_get_prototype*
@@ -1438,7 +1444,8 @@ to start projecting the TypedArray.
 Returns `napi_ok` if the API succeeded.
 
 This API returns various properties of a typed array.
-Warning: Use caution while using this API since the underlying data buffer
+
+*Warning*: Use caution while using this API since the underlying data buffer
 is managed by the VM
 
 #### *napi_get_value_bool*
@@ -1457,8 +1464,8 @@ Boolean.
 Returns `napi_ok` if the API succeeded. If a non-boolean `napi_value` is
 passed in it returns `napi_boolean_expected`.
 
-This API returns C boolean primitive equivalent of the given JavaScript
-Boolea
+This API returns the C boolean primitive equivalent of the given JavaScript
+Boolean.
 
 #### *napi_get_value_double*
 <!-- YAML
@@ -1493,14 +1500,14 @@ napi_status napi_get_value_external(napi_env env,
 ```
 
 - `[in] env`: The environment that the API is invoked under.
-- `[in] value`: `napi_value` representing JavaScript External value.
-- `[out] result`: Pointer to the data wrapped by the JavaScript External value.
+- `[in] value`: `napi_value` representing JavaScript external value.
+- `[out] result`: Pointer to the data wrapped by the JavaScript external value.
 
 Returns `napi_ok` if the API succeeded. If a non-external `napi_value` is
 passed in it returns `napi_invalid_arg`.
 
-This API returns the pointer to the data wrapped by the JavaScript
-External value
+This API retrieves the external data pointer that was previously passed to
+`napi_create_external()`.
 
 #### *napi_get_value_int32*
 <!-- YAML
@@ -2770,6 +2777,7 @@ napi_status napi_wrap(napi_env env,
 Returns `napi_ok` if the API succeeded.
 
 Wraps a native instance in JavaScript object of the corresponding type.
+The native instance can be retrieved later using `napi_unwrap()`.
 
 When JavaScript code invokes a constructor for a class that was defined using
 `napi_define_class()`, the `napi_callback` for the constructor is invoked.
@@ -2787,12 +2795,16 @@ The optional returned reference is initially a weak reference, meaning it
 has a reference count of 0. Typically this reference count would be incremented
 temporarily during async operations that require the instance to remain valid.
 
-Caution: The optional returned reference (if obtained) should be deleted via
-[`napi_delete_reference`][] ONLY in response to the finalize callback invocation.
-(If it is deleted before then, then the finalize callback may never be
-invoked.) Therefore when obtaining a reference a finalize callback is also
+*Caution*: The optional returned reference (if obtained) should be deleted via
+[`napi_delete_reference`][] ONLY in response to the finalize callback
+invocation. (If it is deleted before then, then the finalize callback may never
+be invoked.) Therefore, when obtaining a reference a finalize callback is also
 required in order to enable correct proper of the reference.
 
+*Note*: This API may modify the prototype chain of the wrapper object.
+Afterward, additional manipulation of the wrapper's prototype chain may cause
+`napi_unwrap()` to fail.
+
 ### *napi_unwrap*
 <!-- YAML
 added: v8.0.0
@@ -2809,6 +2821,9 @@ napi_status napi_unwrap(napi_env env,
 
 Returns `napi_ok` if the API succeeded.
 
+Retrieves a native instance that was previously wrapped in a JavaScript
+object using `napi_wrap()`.
+
 When JavaScript code invokes a method or property accessor on the class, the
 corresponding `napi_callback` is invoked. If the callback is for an instance
 method or accessor, then the `this` argument to the callback is the wrapper
diff --git a/src/node_api.cc b/src/node_api.cc
index 719ceacb4b..0c8abd0998 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -819,9 +819,6 @@ napi_status napi_define_class(napi_env env,
   v8::Local<v8::FunctionTemplate> tpl = v8::FunctionTemplate::New(
       isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
 
-  // we need an internal field to stash the wrapped object
-  tpl->InstanceTemplate()->SetInternalFieldCount(1);
-
   v8::Local<v8::String> name_string;
   CHECK_NEW_FROM_UTF8(env, name_string, utf8name);
   tpl->SetClassName(name_string);
@@ -1950,14 +1947,24 @@ napi_status napi_wrap(napi_env env,
   CHECK_ARG(env, js_object);
 
   v8::Isolate* isolate = env->isolate;
-  v8::Local<v8::Object> obj =
-      v8impl::V8LocalValueFromJsValue(js_object).As<v8::Object>();
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
 
-  // Only objects that were created from a NAPI constructor's prototype
-  // via napi_define_class() can be (un)wrapped.
-  RETURN_STATUS_IF_FALSE(env, obj->InternalFieldCount() > 0, napi_invalid_arg);
+  v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(js_object);
+  RETURN_STATUS_IF_FALSE(env, value->IsObject(), napi_invalid_arg);
+  v8::Local<v8::Object> obj = value.As<v8::Object>();
+
+  // Create a wrapper object with an internal field to hold the wrapped pointer.
+  v8::Local<v8::ObjectTemplate> wrapperTemplate =
+    v8::ObjectTemplate::New(isolate);
+  wrapperTemplate->SetInternalFieldCount(1);
+  v8::Local<v8::Object> wrapper =
+    wrapperTemplate->NewInstance(context).ToLocalChecked();
+  wrapper->SetInternalField(0, v8::External::New(isolate, native_object));
 
-  obj->SetInternalField(0, v8::External::New(isolate, native_object));
+  // Insert the wrapper into the object's prototype chain.
+  v8::Local<v8::Value> proto = obj->GetPrototype();
+  wrapper->SetPrototype(proto);
+  obj->SetPrototype(wrapper);
 
   if (result != nullptr) {
     // The returned reference should be deleted via napi_delete_reference()
@@ -1988,11 +1995,18 @@ napi_status napi_unwrap(napi_env env, napi_value js_object, void** result) {
   RETURN_STATUS_IF_FALSE(env, value->IsObject(), napi_invalid_arg);
   v8::Local<v8::Object> obj = value.As<v8::Object>();
 
-  // Only objects that were created from a NAPI constructor's prototype
-  // via napi_define_class() can be (un)wrapped.
-  RETURN_STATUS_IF_FALSE(env, obj->InternalFieldCount() > 0, napi_invalid_arg);
-
-  v8::Local<v8::Value> unwrappedValue = obj->GetInternalField(0);
+  // Search the object's prototype chain for the wrapper with an internal field.
+  // Usually the wrapper would be the first in the chain, but it is OK for
+  // other objects to be inserted in the prototype chain.
+  v8::Local<v8::Object> wrapper = obj;
+  do {
+    v8::Local<v8::Value> proto = wrapper->GetPrototype();
+    RETURN_STATUS_IF_FALSE(
+      env, !proto.IsEmpty() && proto->IsObject(), napi_invalid_arg);
+    wrapper = proto.As<v8::Object>();
+  } while (wrapper->InternalFieldCount() != 1);
+
+  v8::Local<v8::Value> unwrappedValue = wrapper->GetInternalField(0);
   RETURN_STATUS_IF_FALSE(env, unwrappedValue->IsExternal(), napi_invalid_arg);
 
   *result = unwrappedValue.As<v8::External>()->Value();
diff --git a/test/addons-napi/test_object/test.js b/test/addons-napi/test_object/test.js
index d14a15421f..4b64fa4b38 100644
--- a/test/addons-napi/test_object/test.js
+++ b/test/addons-napi/test_object/test.js
@@ -31,35 +31,73 @@ assert(test_object.Has(newObject, 'test_number'));
 assert.strictEqual(newObject.test_number, 987654321);
 assert.strictEqual(newObject.test_string, 'test string');
 
-// test_object.Inflate increases all properties by 1
-const cube = {
-  x: 10,
-  y: 10,
-  z: 10
-};
+{
+  // test_object.Inflate increases all properties by 1
+  const cube = {
+    x: 10,
+    y: 10,
+    z: 10
+  };
 
-assert.deepStrictEqual(test_object.Inflate(cube), {x: 11, y: 11, z: 11});
-assert.deepStrictEqual(test_object.Inflate(cube), {x: 12, y: 12, z: 12});
-assert.deepStrictEqual(test_object.Inflate(cube), {x: 13, y: 13, z: 13});
-cube.t = 13;
-assert.deepStrictEqual(test_object.Inflate(cube), {x: 14, y: 14, z: 14, t: 14});
-
-const sym1 = Symbol('1');
-const sym2 = Symbol('2');
-const sym3 = Symbol('3');
-const sym4 = Symbol('4');
-const object2 = {
-  [sym1]: '@@iterator',
-  [sym2]: sym3
-};
+  assert.deepStrictEqual(test_object.Inflate(cube), {x: 11, y: 11, z: 11});
+  assert.deepStrictEqual(test_object.Inflate(cube), {x: 12, y: 12, z: 12});
+  assert.deepStrictEqual(test_object.Inflate(cube), {x: 13, y: 13, z: 13});
+  cube.t = 13;
+  assert.deepStrictEqual(
+    test_object.Inflate(cube), {x: 14, y: 14, z: 14, t: 14});
+
+  const sym1 = Symbol('1');
+  const sym2 = Symbol('2');
+  const sym3 = Symbol('3');
+  const sym4 = Symbol('4');
+  const object2 = {
+    [sym1]: '@@iterator',
+    [sym2]: sym3
+  };
+
+  assert(test_object.Has(object2, sym1));
+  assert(test_object.Has(object2, sym2));
+  assert.strictEqual(test_object.Get(object2, sym1), '@@iterator');
+  assert.strictEqual(test_object.Get(object2, sym2), sym3);
+  assert(test_object.Set(object2, 'string', 'value'));
+  assert(test_object.Set(object2, sym4, 123));
+  assert(test_object.Has(object2, 'string'));
+  assert(test_object.Has(object2, sym4));
+  assert.strictEqual(test_object.Get(object2, 'string'), 'value');
+  assert.strictEqual(test_object.Get(object2, sym4), 123);
+}
+
+{
+  // Wrap a pointer in a JS object, then verify the pointer can be unwrapped.
+  const wrapper = {};
+  test_object.Wrap(wrapper);
+
+  assert(test_object.Unwrap(wrapper));
+}
+
+{
+  // Verify that wrapping doesn't break an object's prototype chain.
+  const wrapper = {};
+  const protoA = { protoA: true };
+  Object.setPrototypeOf(wrapper, protoA);
+  test_object.Wrap(wrapper);
+
+  assert(test_object.Unwrap(wrapper));
+  assert(wrapper.protoA);
+}
+
+{
+  // Verify the pointer can be unwrapped after inserting in the prototype chain.
+  const wrapper = {};
+  const protoA = { protoA: true };
+  Object.setPrototypeOf(wrapper, protoA);
+  test_object.Wrap(wrapper);
+
+  const protoB = { protoB: true };
+  Object.setPrototypeOf(protoB, Object.getPrototypeOf(wrapper));
+  Object.setPrototypeOf(wrapper, protoB);
 
-assert(test_object.Has(object2, sym1));
-assert(test_object.Has(object2, sym2));
-assert.strictEqual(test_object.Get(object2, sym1), '@@iterator');
-assert.strictEqual(test_object.Get(object2, sym2), sym3);
-assert(test_object.Set(object2, 'string', 'value'));
-assert(test_object.Set(object2, sym4, 123));
-assert(test_object.Has(object2, 'string'));
-assert(test_object.Has(object2, sym4));
-assert.strictEqual(test_object.Get(object2, 'string'), 'value');
-assert.strictEqual(test_object.Get(object2, sym4), 123);
+  assert(test_object.Unwrap(wrapper));
+  assert(wrapper.protoA, true);
+  assert(wrapper.protoB, true);
+}
diff --git a/test/addons-napi/test_object/test_object.c b/test/addons-napi/test_object/test_object.c
index 9e02c1d0bb..dd2db123f7 100644
--- a/test/addons-napi/test_object/test_object.c
+++ b/test/addons-napi/test_object/test_object.c
@@ -1,6 +1,7 @@
 #include <node_api.h>
 #include "../common.h"
 #include <string.h>
+#include <stdlib.h>
 
 napi_value Get(napi_env env, napi_callback_info info) {
   size_t argc = 2;
@@ -138,6 +139,30 @@ napi_value Inflate(napi_env env, napi_callback_info info) {
   return obj;
 }
 
+napi_value Wrap(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value arg;
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &arg, NULL, NULL));
+
+  int32_t* data = malloc(sizeof(int32_t));
+  *data = 3;
+  NAPI_CALL(env, napi_wrap(env, arg, data, NULL, NULL, NULL));
+  return NULL;
+}
+
+napi_value Unwrap(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value arg;
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &arg, NULL, NULL));
+
+  int32_t* data;
+  NAPI_CALL(env, napi_unwrap(env, arg, &data));
+
+  napi_value result;
+  NAPI_CALL(env, napi_get_boolean(env, data != NULL && *data == 3, &result));
+  return result;
+}
+
 void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
   napi_property_descriptor descriptors[] = {
     DECLARE_NAPI_PROPERTY("Get", Get),
@@ -145,6 +170,8 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
     DECLARE_NAPI_PROPERTY("Has", Has),
     DECLARE_NAPI_PROPERTY("New", New),
     DECLARE_NAPI_PROPERTY("Inflate", Inflate),
+    DECLARE_NAPI_PROPERTY("Wrap", Wrap),
+    DECLARE_NAPI_PROPERTY("Unwrap", Unwrap),
   };
 
   NAPI_CALL_RETURN_VOID(env, napi_define_properties(