n-api: enable napi_wrap() to work with any object #13250

jasongin · 2017-05-26T23:47:34Z

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. (ChakraCore doesn't have ObjectPrototypes or object internal fields.)

I encountered this problem when testing the N-API port of node-sass: it worked on V8, but failed on Node-ChakraCore, because it used napi_wrap() in a slightly different way from our existing unit tests and other examples. I also submitted a corresponding PR to Node-ChakraCore to update the napi_wrap() implementation there to similarly allow any object. (So then the test case added here will pass there.)

Regarding the updated napi_wrap() implementation: 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().

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

n-api

jasongin · 2017-05-26T23:47:49Z

@boingoing

mhdawson · 2017-05-29T15:37:21Z

doc/api/n-api.md

-invoked.) Therefore when obtaining a reference a finalize callback is also
+[`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


Maybe add a comma after 'Therefore'

mhdawson · 2017-05-29T15:58:31Z

doc/api/n-api.md

 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.


Is there some way we can search up the prototype chain so that unless they remove the wrapper object napi_unwrap can still work ? They could still mess it up but I think it would be less likely to happen.

Yes, I'll update the code to do that.

jasnell · 2017-05-30T01:52:28Z

doc/api/n-api.md

+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


s/Note/*Note*

jasnell · 2017-05-30T01:52:56Z

doc/api/n-api.md

 required in order to enable correct proper of the reference.

+Note: This API may modify the prototype chain of the wrapper object.


Same here... :-)

boingoing

Looks good to me 👍

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().

jasongin · 2017-05-31T00:12:45Z

@mhdawson @jasnell I pushed a commit to address your comments.

mhdawson

LGTM

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: #13250 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasongin · 2017-06-01T21:14:55Z

CI is good, except for one failure, a timeout in test-fs-watchfile on AIX that's obviously unrelated.
https://ci.nodejs.org/job/node-test-pull-request/8411/

Landed as 8ab8c33

jasnell · 2017-06-01T23:33:42Z

Looks like it was landed but not closed.

mscdex · 2017-06-02T02:10:31Z

@jasongin FWIW I'm now seeing a compiler warning since this PR:

Building addon /home/mscdex/git/node/test/addons-napi/test_object/
make[2]: Entering directory '/home/mscdex/git/node/test/addons-napi/test_object/build'
  CC(target) Release/obj.target/test_object/test_object.o
In file included from ../test_object.c:2:0:
../test_object.c: In function ‘Unwrap’:
../test_object.c:159:40: warning: passing argument 3 of ‘napi_unwrap’ from incompatible pointer type [-Wincompatible-pointer-types]
   NAPI_CALL(env, napi_unwrap(env, arg, &data));
                                        ^
../../common.h:38:8: note: in definition of macro ‘NAPI_CALL_BASE’
   if ((the_call) != napi_ok) {                                           \
        ^~~~~~~~
../test_object.c:159:3: note: in expansion of macro ‘NAPI_CALL’
   NAPI_CALL(env, napi_unwrap(env, arg, &data));
   ^~~~~~~~~
In file included from ../test_object.c:1:0:
/home/mscdex/git/node/src/node_api.h:329:25: note: expected ‘void **’ but argument is of type ‘int32_t ** {aka int **}’
 NAPI_EXTERN napi_status napi_unwrap(napi_env env,
                         ^~~~~~~~~~~

jasongin · 2017-06-02T03:54:55Z

@mscdex Thanks for pointing that out. I'll submit another PR to fix it tomorrow if nobody else gets to it earlier.

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: #13250 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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: nodejs#13250 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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(). Backport-PR-URL: #19447 PR-URL: #13250 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels May 26, 2017

jasongin requested a review from mhdawson May 26, 2017 23:48

jasongin mentioned this pull request May 27, 2017

n-api: enable napi_wrap() to work with any object nodejs/node-chakracore#269

Closed

4 tasks

mhdawson reviewed May 29, 2017

View reviewed changes

jasnell reviewed May 30, 2017

View reviewed changes

boingoing approved these changes May 30, 2017

View reviewed changes

jasongin added 2 commits May 30, 2017 14:55

Address review feedback

d6f0380

jasongin force-pushed the napi-wrap branch from be446ff to d6f0380 Compare May 31, 2017 00:12

mhdawson approved these changes May 31, 2017

View reviewed changes

jasnell approved these changes Jun 1, 2017

View reviewed changes

jasnell closed this Jun 1, 2017

jasongin deleted the napi-wrap branch June 2, 2017 03:52

jasongin mentioned this pull request Jun 2, 2017

test: fix build warning in addons-napi\test_object #13412

Closed

3 tasks

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added dont-land-on-v4.x labels Jun 18, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: enable napi_wrap() to work with any object #13250

n-api: enable napi_wrap() to work with any object #13250

jasongin commented May 26, 2017 •

edited

jasongin commented May 26, 2017

mhdawson May 29, 2017

mhdawson May 29, 2017

jasongin May 30, 2017

jasnell May 30, 2017

jasnell May 30, 2017

boingoing left a comment

jasongin commented May 31, 2017

mhdawson left a comment

jasongin commented Jun 1, 2017

jasnell commented Jun 1, 2017

mscdex commented Jun 2, 2017 •

edited

jasongin commented Jun 2, 2017

		required in order to enable correct proper of the reference.

		Note: This API may modify the prototype chain of the wrapper object.

n-api: enable napi_wrap() to work with any object #13250

n-api: enable napi_wrap() to work with any object #13250

Conversation

jasongin commented May 26, 2017 • edited

Checklist

Affected core subsystem(s)

jasongin commented May 26, 2017

mhdawson May 29, 2017

Choose a reason for hiding this comment

mhdawson May 29, 2017

Choose a reason for hiding this comment

jasongin May 30, 2017

Choose a reason for hiding this comment

jasnell May 30, 2017

Choose a reason for hiding this comment

jasnell May 30, 2017

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

jasongin commented May 31, 2017

mhdawson left a comment

Choose a reason for hiding this comment

jasongin commented Jun 1, 2017

jasnell commented Jun 1, 2017

mscdex commented Jun 2, 2017 • edited

jasongin commented Jun 2, 2017

jasongin commented May 26, 2017 •

edited

mscdex commented Jun 2, 2017 •

edited