From 2b78ff955bf5d6aa29dea13b28e625af3045a8a7 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Sat, 9 Sep 2017 12:15:14 +0300 Subject: [PATCH] n-api: stop creating references to primitives The binding testing napi_wrap() creates references to primitives passed into the binding in its second parameter. This is unnecessary and not at all the point of the test. Additionally, creating persistent references to primitive values may not be supported by all VMs, since primitives are best persisted in their native form. Instead, the point of the test is to make sure that the finalize callback gets called when it should get called, that it gets called with the correct pointer, and that it does not get called when it should not get called. Creating persistent references is not necessary for verifying this. Re: https://github.com/nodejs/node-chakracore/issues/380 --- doc/api/n-api.md | 2 +- src/node_api.cc | 10 ++++++++-- test/addons-napi/test_general/test.js | 10 +++++----- test/addons-napi/test_general/test_general.c | 17 +++++++---------- 4 files changed, 21 insertions(+), 18 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 3a3cce03646821..9652e650851a3c 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -786,7 +786,7 @@ NODE_EXTERN napi_status napi_create_reference(napi_env env, - `[in] env`: The environment that the API is invoked under. - `[in] value`: `napi_value` representing the Object to which we want -a reference to. +a reference. - `[in] initial_refcount`: Initial reference count for the new reference. - `[out] result`: `napi_ref` pointing to the new reference. diff --git a/src/node_api.cc b/src/node_api.cc index 16549120b20737..d59c74d6d33caa 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -2458,8 +2458,14 @@ napi_status napi_create_reference(napi_env env, CHECK_ARG(env, value); CHECK_ARG(env, result); - v8impl::Reference* reference = v8impl::Reference::New( - env, v8impl::V8LocalValueFromJsValue(value), initial_refcount, false); + v8::Local v8_value = v8impl::V8LocalValueFromJsValue(value); + + if (!(v8_value->IsObject() || v8_value->IsFunction())) { + return napi_set_last_error(env, napi_object_expected); + } + + v8impl::Reference* reference = + v8impl::Reference::New(env, v8_value, initial_refcount, false); *result = reinterpret_cast(reference); return napi_clear_last_error(env); diff --git a/test/addons-napi/test_general/test.js b/test/addons-napi/test_general/test.js index e9e2b9614ca34c..3ff57922c5372b 100644 --- a/test/addons-napi/test_general/test.js +++ b/test/addons-napi/test_general/test.js @@ -60,7 +60,7 @@ assert.strictEqual(test_general.testNapiTypeof(null), 'null'); // Ensure that garbage collecting an object with a wrapped native item results // in the finalize callback being called. let w = {}; -test_general.wrap(w, []); +test_general.wrap(w); w = null; global.gc(); assert.strictEqual(test_general.derefItemWasCalled(), true, @@ -69,17 +69,17 @@ assert.strictEqual(test_general.derefItemWasCalled(), true, // Assert that wrapping twice fails. const x = {}; -test_general.wrap(x, 25); +test_general.wrap(x); assert.throws(function() { - test_general.wrap(x, 'Blah'); + test_general.wrap(x); }, Error); // Ensure that wrapping, removing the wrap, and then wrapping again works. const y = {}; -test_general.wrap(y, -12); +test_general.wrap(y); test_general.removeWrap(y); assert.doesNotThrow(function() { - test_general.wrap(y, 're-wrap!'); + test_general.wrap(y); }, Error, 'Wrapping twice succeeds if a remove_wrap() separates the instances'); // Ensure that removing a wrap and garbage collecting does not fire the diff --git a/test/addons-napi/test_general/test_general.c b/test/addons-napi/test_general/test_general.c index ea1f2ece0a5d30..43ffa2da5cb155 100644 --- a/test/addons-napi/test_general/test_general.c +++ b/test/addons-napi/test_general/test_general.c @@ -142,8 +142,10 @@ static bool deref_item_called = false; static void deref_item(napi_env env, void* data, void* hint) { (void) hint; + NAPI_ASSERT_RETURN_VOID(env, data == &deref_item_called, + "Finalize callback was called with the correct pointer"); + deref_item_called = true; - NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, (napi_ref)data)); } napi_value deref_item_was_called(napi_env env, napi_callback_info info) { @@ -155,15 +157,13 @@ napi_value deref_item_was_called(napi_env env, napi_callback_info info) { } napi_value wrap(napi_env env, napi_callback_info info) { - size_t argc = 2; - napi_value argv[2]; - napi_ref payload; + size_t argc = 1; + napi_value to_wrap; deref_item_called = false; - NAPI_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); - NAPI_CALL(env, napi_create_reference(env, argv[1], 1, &payload)); - NAPI_CALL(env, napi_wrap(env, argv[0], payload, deref_item, NULL, NULL)); + NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &to_wrap, NULL, NULL)); + NAPI_CALL(env, napi_wrap(env, to_wrap, &deref_item_called, deref_item, NULL, NULL)); return NULL; } @@ -175,9 +175,6 @@ napi_value remove_wrap(napi_env env, napi_callback_info info) { NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &wrapped, NULL, NULL)); NAPI_CALL(env, napi_remove_wrap(env, wrapped, &data)); - if (data != NULL) { - NAPI_CALL(env, napi_delete_reference(env, (napi_ref)data)); - } return NULL; }