Address review feedback

Author: jasongin

doc/api/n-api.md

@@ -1074,7 +1074,7 @@ 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
+*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`.
 
@@ -1368,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
@@ -1395,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*
@@ -1442,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*
@@ -2792,13 +2795,13 @@ 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
+*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
+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.
+*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.
 

src/node_api.cc

@@ -1995,13 +1995,16 @@ 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>();
 
-  // The object's prototype should be a wrapper with an internal field.
-  v8::Local<v8::Value> proto = obj->GetPrototype();
-  RETURN_STATUS_IF_FALSE(
-    env, !proto.IsEmpty() && proto->IsObject(), napi_invalid_arg);
-  v8::Local<v8::Object> wrapper = proto.As<v8::Object>();
-  RETURN_STATUS_IF_FALSE(
-    env, wrapper->InternalFieldCount() == 1, napi_invalid_arg);
+  // 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);

test/addons-napi/test_object/test.js

@@ -31,40 +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);
-
-// Wrap a pointer in a JS object, then verify that the pointer can be unwrapped.
-const wrapper = {};
-test_object.Wrap(wrapper);
-assert(test_object.Unwrap(wrapper));
+  assert(test_object.Unwrap(wrapper));
+  assert(wrapper.protoA, true);
+  assert(wrapper.protoB, true);
+}