Skip to content

Commit

Permalink
n-api: stop creating references to primitives
Browse files Browse the repository at this point in the history
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.

PR-URL: #15289
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Re: nodejs/node-chakracore#380
  • Loading branch information
Gabriel Schulhof authored and jasnell committed Sep 20, 2017
1 parent 38422d5 commit d5b3002
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 18 deletions.
2 changes: 1 addition & 1 deletion doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -787,7 +787,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.

Expand Down
10 changes: 8 additions & 2 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2467,8 +2467,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> 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<napi_ref>(reference);
return napi_clear_last_error(env);
Expand Down
10 changes: 5 additions & 5 deletions test/addons-napi/test_general/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down
17 changes: 7 additions & 10 deletions test/addons-napi/test_general/test_general.c
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,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) {
Expand All @@ -156,15 +158,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;
}
Expand All @@ -176,9 +176,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;
}
Expand Down

0 comments on commit d5b3002

Please sign in to comment.