Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

jasongin
Copy link
Member

@jasongin jasongin commented May 26, 2017

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

@nodejs-github-bot 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
Copy link
Member Author

@boingoing

doc/api/n-api.md Outdated
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comma after 'Therefore'

doc/api/n-api.md Outdated
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/n-api.md Outdated
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Note/*Note*

doc/api/n-api.md Outdated
required in order to enable correct proper of the reference.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here... :-)

Copy link
Contributor

@boingoing boingoing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

jasongin added 2 commits May 30, 2017 14:55
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
Copy link
Member Author

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jasongin added a commit that referenced this pull request Jun 1, 2017
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
Copy link
Member Author

jasongin commented Jun 1, 2017

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
Copy link
Member

jasnell commented Jun 1, 2017

Looks like it was landed but not closed.

@jasnell jasnell closed this Jun 1, 2017
@mscdex
Copy link
Contributor

mscdex commented Jun 2, 2017

@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 jasongin deleted the napi-wrap branch June 2, 2017 03:52
@jasongin
Copy link
Member Author

jasongin commented Jun 2, 2017

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

jasnell pushed a commit that referenced this pull request Jun 5, 2017
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>
@jasnell jasnell mentioned this pull request Jun 5, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
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>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants