N-API: Implement stricter wrapping #13872

gabrielschulhof · 2017-06-22T11:52:11Z

Use a stronger criterion to identify objects in the prototype chain that store
pointers to native data that were added by previous calls to napi_wrap().

Whereas the old criterion for identifying napi_wrap()-injected prototype
chain objects was to consider an object with an internal field
count of 1 to be such an object, the new criterion is to consider an object
with an internal field count of 2 such that the second field holds a
v8::External which itself contains a pointer to a global static string unique
to N-API to be a napi_wrap()-injected prototype chain object.

This greatly reduces the possibility of returning a pointer that was not
previously added with napi_wrap(), and it allows us to recognize that an
object has already undergone napi_wrap() and we can thus replace the old
v8::External with the new one such that, when the old one will be garbage-
collected, any finalize callback for its native pointer will also be called.

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)

gabrielschulhof · 2017-06-22T11:59:33Z

src/node_api.cc

+// Search the object's prototype chain for the wrapper object. Usually the
+// wrapper would be the first in the chain, but it is OK for other objects to
+// be inserted in the prototype chain.
+bool FindWrapper(v8::Local<v8::Object> obj,


If we used a v8::FunctionTemplate instead of a simple string we could take advantage of v8::Object::FindInstanceInPrototypeChain(Local< FunctionTemplate> tmpl) instead of having to perform the descent ourselves.

The problem with that is that the FunctionTemplate it requires needs to be stored somewhere. Now, it can be stored on the napi_env but the problem with that is that the env is per-module. So, if one module does napi_wrap(), then the other module will not recognize that this has happened, because it will have its own copy of a FunctionTemplate serving the same purpose. Is this desirable or is this to be avoided?

@nodejs/addon-api

I'd say it we are better off depending on something that prevents things from working across modules.

I meant to say not depending on something that prevents things from working across modules. I think that's why you avoided the function template in the PR so all should be good.

So, if one module does napi_wrap(), then the other module will not recognize that this has happened, because it will have its own copy of a FunctionTemplate serving the same purpose.

It doesn't have to be. FunctionTemplates can be per-isolate (don't need to be per-context) so if you manage to associate some state with the isolate, you should be good.

As to how to do that: you could use v8::Isolate::SetData() but that's a scarce resource (only 4 slots and one is already taken.)

Perhaps a static std::map<v8::Isolate*, State*> guarded by a mutex? You only need to look it up once per napi_env because you can cache it inside the env. Lock contention shouldn't be an issue.

mhdawson · 2017-06-22T18:43:45Z

will wait to do complete review until updated to return an error on second wrap as discussed in the N-API meeting today.

gabrielschulhof · 2017-06-22T19:17:23Z

@mhdawson done now.

mhdawson · 2017-06-26T18:35:48Z

doc/api/n-api.md

+*Note*: Calling `napi_wrap()` a second time on an object that already has a
+native instance associated with it by virtue of a previous call to
+`napi_wrap()` will have the effect that the new instance will replace the old
+instance, and the old instance will eventually be freed by the JavaScript


I think this needs to be updated to reflect an error will be thrown.

mhdawson · 2017-06-26T18:38:29Z

src/node_api.cc

+  // Store the pointer as an external in the wrapper. If an external is already
+  // there, this will replace it and the old external will eventually be gc-ed.
+  // At that point the associated pointer will be finalized if a finalize_cb was
+  // specified at the time of wrapping.


This should be updated as well. Since we should error out above.

mhdawson · 2017-06-26T18:41:24Z

test/addons-napi/test_wrap/binding.gyp

@@ -0,0 +1,6 @@
+{


I think we should add this test to test_general instead of using a new addon. Each addon makes the compile just a little bit longer.

gabrielschulhof · 2017-06-26T20:26:41Z

@mhdawson I have addressed your comments.

mhdawson

LGTM

mhdawson · 2017-06-28T19:15:08Z

src/node_api.cc

+// Search the object's prototype chain for the wrapper object. Usually the
+// wrapper would be the first in the chain, but it is OK for other objects to
+// be inserted in the prototype chain.
+bool FindWrapper(v8::Local<v8::Object> obj,


I meant to say not depending on something that prevents things from working across modules. I think that's why you avoided the function template in the PR so all should be good.

mhdawson · 2017-06-28T19:16:59Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/8854/

mhdawson · 2017-07-06T16:01:01Z

@gabrielschulhof sorry this dropped off my radar, needs a rebase and a new CI run.

gabrielschulhof · 2017-07-06T16:18:30Z

I had an unrelated test failure when I ran make -j4 test:

Path: sequential/test-fs-readfile-tostring-fail
/boot/nix/node/node/test/sequential/test-fs-readfile-tostring-fail.js:56
  throw err;
  ^

AssertionError [ERR_ASSERTION]: false == true
    at /boot/nix/node/node/test/sequential/test-fs-readfile-tostring-fail.js:31:12
    at /boot/nix/node/node/test/common/index.js:518:15
    at tryToString (fs.js:512:3)
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:500:12)
Command: out/Release/node /boot/nix/node/node/test/sequential/test-fs-readfile-tostring-fail.js

gabrielschulhof · 2017-07-06T16:31:23Z

@mhdawson I've rebased, but I suspect a test run now will likely run into this unrelated error.

mhdawson · 2017-07-06T18:27:49Z

Needs one more rebase since I landed the other PR we talked about first.

gabrielschulhof · 2017-07-06T20:47:15Z

@mhdawson It's rebased now, and when I ran make -j4 test locally, this time it passed with flying colours :)

TimothyGu · 2017-07-07T01:13:51Z

src/node_api.cc

@@ -673,6 +673,38 @@ v8::Local<v8::Object> CreateAccessorCallbackData(napi_env env,
  return cbdata;
 }

+// Pointer used to identify items wrapped by N-API. Used by FindWrapper and
+// napi_wrap().
+char napi_wrap_name[] = "N-API Wrapper";


Nit: const char*?

I left out the const because converting from const char* to void* requires a pile of casting:

reinterpret_cast<void*>(const_cast<char*>(v8impl::napi_wrap_name))

But anyway, I'll add it.

TimothyGu · 2017-07-07T01:17:48Z

test/addons-napi/test_general/test_general.c

+napi_value wrap(napi_env env, napi_callback_info info) {
+  size_t argc = 2;
+  napi_value argv[2];
+  napi_ref payload, callback;


Is callback used anywhere?

mhdawson · 2017-07-07T17:17:00Z

Looks like @gabrielschulhof pushed a commit to address @TimothyGu's comments.

CI run - https://ci.nodejs.org/job/node-test-pull-request/9030/

gabrielschulhof · 2017-07-11T06:27:31Z

Is this OK to land?

TimothyGu · 2017-07-11T07:00:30Z

doc/api/n-api.md

@@ -2905,6 +2905,10 @@ required in order to enable correct proper of the reference.
 Afterward, additional manipulation of the wrapper's prototype chain may cause
 `napi_unwrap()` to fail.

+*Note*: Calling `napi_wrap()` a second time on an object that already has a
+native instance associated with it by virtue of a previous call to
+`napi_wrap()` will cause an error to be thrown.


error to be returned?

TimothyGu · 2017-07-11T07:06:44Z

src/node_api.cc

+
+  // Store the pointer as an external in the wrapper.
+  v8::Local<v8::Value> external = v8::External::New(isolate, native_object);
+  wrapper->SetInternalField(0, external);


Why are you overwriting the External created and stored just a few lines up?

I'm not. A few lines up I'm setting the other index (1) whereas here I'm setting (0).

Oh, dang! Merge error. You're right!

Use a stronger criterion to identify objects in the prototype chain that store pointers to native data that were added by previous calls to `napi_wrap()`. Whereas the old criterion for identifying `napi_wrap()`-injected prototype chain objects was to consider an object with an internal field count of 1 to be such an object, the new criterion is to consider an object with an internal field count of 2 such that the second field holds a `v8::External` which itself contains a pointer to a global static string unique to N-API to be a `napi_wrap()`-injected prototype chain object. This greatly reduces the possibility of returning a pointer that was not previously added with `napi_wrap()`, and it allows us to recognize that an object has already undergone `napi_wrap()` and we can thus prevent a chain of wrappers only the first of which is accessible from appearing in the prototype chain, as would be the result of multiple calls to `napi_wrap()` using the same object.

gabrielschulhof · 2017-07-11T08:01:24Z

@TimothyGu should be good now, but it needs another CI run.

TimothyGu · 2017-07-12T02:02:36Z

Running CI at https://ci.nodejs.org/job/node-test-pull-request/9078/. Might take a while though because of the backlog from the CI lockdown.

evanlucas · 2017-07-12T16:01:24Z

Can you lowercase the subsystem in the commit?

mhdawson · 2017-07-12T16:02:51Z

@evanlucas will do

mhdawson · 2017-07-12T16:08:46Z

Landed as d5b397c

Use a stronger criterion to identify objects in the prototype chain that store pointers to native data that were added by previous calls to `napi_wrap()`. Whereas the old criterion for identifying `napi_wrap()`-injected prototype chain objects was to consider an object with an internal field count of 1 to be such an object, the new criterion is to consider an object with an internal field count of 2 such that the second field holds a `v8::External` which itself contains a pointer to a global static string unique to N-API to be a `napi_wrap()`-injected prototype chain object. This greatly reduces the possibility of returning a pointer that was not previously added with `napi_wrap()`, and it allows us to recognize that an object has already undergone `napi_wrap()` and we can thus prevent a chain of wrappers only the first of which is accessible from appearing in the prototype chain, as would be the result of multiple calls to `napi_wrap()` using the same object. PR-URL: #13872 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

mhdawson · 2017-07-12T16:10:41Z

@bnoordhuis I just noticed your comment about not recognizing the wrap between 2 modules after landing. I had discussed this with Gabriel and the way the check works that is not a problem since we don't check against the FunctionTemplate itself.

Having said that @gabrielschulhof does make sense for us to look at the suggestion to see if that makes thing better.

Use a stronger criterion to identify objects in the prototype chain that store pointers to native data that were added by previous calls to `napi_wrap()`. Whereas the old criterion for identifying `napi_wrap()`-injected prototype chain objects was to consider an object with an internal field count of 1 to be such an object, the new criterion is to consider an object with an internal field count of 2 such that the second field holds a `v8::External` which itself contains a pointer to a global static string unique to N-API to be a `napi_wrap()`-injected prototype chain object. This greatly reduces the possibility of returning a pointer that was not previously added with `napi_wrap()`, and it allows us to recognize that an object has already undergone `napi_wrap()` and we can thus prevent a chain of wrappers only the first of which is accessible from appearing in the prototype chain, as would be the result of multiple calls to `napi_wrap()` using the same object. PR-URL: #13872 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Re: nodejs#13872 (comment)

Use a stronger criterion to identify objects in the prototype chain that store pointers to native data that were added by previous calls to `napi_wrap()`. Whereas the old criterion for identifying `napi_wrap()`-injected prototype chain objects was to consider an object with an internal field count of 1 to be such an object, the new criterion is to consider an object with an internal field count of 2 such that the second field holds a `v8::External` which itself contains a pointer to a global static string unique to N-API to be a `napi_wrap()`-injected prototype chain object. This greatly reduces the possibility of returning a pointer that was not previously added with `napi_wrap()`, and it allows us to recognize that an object has already undergone `napi_wrap()` and we can thus prevent a chain of wrappers only the first of which is accessible from appearing in the prototype chain, as would be the result of multiple calls to `napi_wrap()` using the same object. PR-URL: nodejs#13872 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Use a stronger criterion to identify objects in the prototype chain that store pointers to native data that were added by previous calls to `napi_wrap()`. Whereas the old criterion for identifying `napi_wrap()`-injected prototype chain objects was to consider an object with an internal field count of 1 to be such an object, the new criterion is to consider an object with an internal field count of 2 such that the second field holds a `v8::External` which itself contains a pointer to a global static string unique to N-API to be a `napi_wrap()`-injected prototype chain object. This greatly reduces the possibility of returning a pointer that was not previously added with `napi_wrap()`, and it allows us to recognize that an object has already undergone `napi_wrap()` and we can thus prevent a chain of wrappers only the first of which is accessible from appearing in the prototype chain, as would be the result of multiple calls to `napi_wrap()` using the same object. Backport-PR-URL: #19447 PR-URL: #13872 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

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 Jun 22, 2017

gabrielschulhof commented Jun 22, 2017

View reviewed changes

gabrielschulhof mentioned this pull request Jun 22, 2017

WIP: N-API: Implement Promise #13717

Closed

4 tasks

gabrielschulhof force-pushed the stricter-wrapping branch from 9d6d6f1 to 5fd44ce Compare June 22, 2017 17:24

gabrielschulhof force-pushed the stricter-wrapping branch from 5fd44ce to ad01722 Compare June 22, 2017 19:11

mhdawson reviewed Jun 26, 2017

View reviewed changes

gabrielschulhof force-pushed the stricter-wrapping branch from ad01722 to 19fb553 Compare June 26, 2017 20:24

jasnell approved these changes Jun 26, 2017

View reviewed changes

mhdawson approved these changes Jun 28, 2017

View reviewed changes

gabrielschulhof force-pushed the stricter-wrapping branch from 19fb553 to 023c1bf Compare July 6, 2017 16:17

gabrielschulhof force-pushed the stricter-wrapping branch from 023c1bf to ee07177 Compare July 6, 2017 20:46

TimothyGu reviewed Jul 7, 2017

View reviewed changes

gabrielschulhof force-pushed the stricter-wrapping branch from ee07177 to c8f4446 Compare July 7, 2017 08:35

TimothyGu reviewed Jul 11, 2017

View reviewed changes

gabrielschulhof force-pushed the stricter-wrapping branch from c8f4446 to 10e30e5 Compare July 11, 2017 07:57

mhdawson closed this Jul 12, 2017

gabrielschulhof mentioned this pull request Jul 13, 2017

n-api: Re-use napi_env between modules #14217

Closed

3 tasks

addaleax mentioned this pull request Jul 18, 2017

v8.2.0 proposal #13744

Merged

gabrielschulhof deleted the stricter-wrapping branch July 19, 2017 12:14

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Jul 22, 2017

n-api: re-use napi_env between modules

cdb7f13

Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Re: nodejs#13872 (comment)

MylesBorins added dont-land-on-v4.x labels Aug 16, 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: Implement stricter wrapping #13872

N-API: Implement stricter wrapping #13872

gabrielschulhof commented Jun 22, 2017 •

edited

Loading

gabrielschulhof Jun 22, 2017 •

edited

Loading

gabrielschulhof Jun 22, 2017

mhdawson Jun 22, 2017

mhdawson Jun 28, 2017

bnoordhuis Jul 12, 2017 •

edited

Loading

mhdawson commented Jun 22, 2017

gabrielschulhof commented Jun 22, 2017

mhdawson Jun 26, 2017

mhdawson Jun 26, 2017

mhdawson Jun 26, 2017

gabrielschulhof commented Jun 26, 2017

mhdawson left a comment

mhdawson Jun 28, 2017

mhdawson commented Jun 28, 2017

mhdawson commented Jul 6, 2017

gabrielschulhof commented Jul 6, 2017

gabrielschulhof commented Jul 6, 2017

mhdawson commented Jul 6, 2017

gabrielschulhof commented Jul 6, 2017

TimothyGu Jul 7, 2017

gabrielschulhof Jul 7, 2017

TimothyGu Jul 7, 2017

mhdawson commented Jul 7, 2017

gabrielschulhof commented Jul 11, 2017

TimothyGu Jul 11, 2017

TimothyGu Jul 11, 2017

gabrielschulhof Jul 11, 2017

gabrielschulhof Jul 11, 2017

gabrielschulhof commented Jul 11, 2017

TimothyGu commented Jul 12, 2017

evanlucas commented Jul 12, 2017

mhdawson commented Jul 12, 2017

mhdawson commented Jul 12, 2017

mhdawson commented Jul 12, 2017

N-API: Implement stricter wrapping #13872

N-API: Implement stricter wrapping #13872

Conversation

gabrielschulhof commented Jun 22, 2017 • edited Loading

Checklist

Affected core subsystem(s)

gabrielschulhof Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

mhdawson commented Jun 22, 2017

gabrielschulhof commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Jun 26, 2017

mhdawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jun 28, 2017

mhdawson commented Jul 6, 2017

gabrielschulhof commented Jul 6, 2017

gabrielschulhof commented Jul 6, 2017

mhdawson commented Jul 6, 2017

gabrielschulhof commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jul 7, 2017

gabrielschulhof commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Jul 11, 2017

TimothyGu commented Jul 12, 2017

evanlucas commented Jul 12, 2017

mhdawson commented Jul 12, 2017

mhdawson commented Jul 12, 2017

mhdawson commented Jul 12, 2017

gabrielschulhof commented Jun 22, 2017 •

edited

Loading

gabrielschulhof Jun 22, 2017 •

edited

Loading

bnoordhuis Jul 12, 2017 •

edited

Loading