Skip to content

Commit

Permalink
n-api: Fix test-addons-napi failures
Browse files Browse the repository at this point in the history 
 - Reimplement napi_make_callback to call node::MakeCallback using V8
   (chakrashim) types. We always knew the direct call to
   JsCallFunction() was incorrect there.
 - Refactor the reference test cases so each GC call happens after a
   setImmediate callback, because the GC often will not collect values
   that were just-allocated in the same event loop iteration. (One of
   the test cases already needed this for the newer V8 GC. For the
   ChakraCore GC, more of them do.)

Fixes: nodejs#246
  • Loading branch information
@jasongin
jasongin committed May 18, 2017
1 parent d07825c commit bb7eef7
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 83 deletions.
28 changes: 14 additions & 14 deletions src/node_api_jsrt.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1623,22 +1623,22 @@ napi_status napi_make_callback(napi_env env,
size_t argc,
const napi_value* argv,
napi_value* result) {
JsValueRef object = reinterpret_cast<JsValueRef>(recv);
JsValueRef function = reinterpret_cast<JsValueRef>(func);
std::vector<JsValueRef> args(argc + 1);
args[0] = object;
for (size_t i = 0; i < argc; i++) {
args[i + 1] = reinterpret_cast<JsValueRef>(argv[i]);
}
JsValueRef returnValue;
CHECK_JSRT(JsCallFunction(
function,
args.data(),
static_cast<uint16_t>(argc + 1),
&returnValue));
v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
v8::Local<v8::Object> v8recv =
v8impl::V8LocalValueFromJsValue(recv).As<v8::Object>();
v8::Local<v8::Function> v8func =
v8impl::V8LocalValueFromJsValue(func).As<v8::Function>();
v8::Local<v8::Value>* v8argv =
reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv));

// TODO: Expose JSRT or N-API version of node::MakeCallback?
v8::Local<v8::Value> v8result =
node::MakeCallback(isolate, v8recv, v8func, argc, v8argv);

if (result != nullptr) {
*result = reinterpret_cast<napi_value>(returnValue);
*result = v8impl::JsValueFromV8LocalValue(v8result);
}

return napi_ok;
}

Expand Down
169 changes: 100 additions & 69 deletions test/addons-napi/test_reference/test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,80 +17,111 @@ const test_reference = require(`./build/${common.buildType}/test_reference`);
// of a finalizer callback increments the finalizeCount property.
assert.strictEqual(test_reference.finalizeCount, 0);

{
// External value without a finalizer
let value = test_reference.createExternal();
assert.strictEqual(test_reference.finalizeCount, 0);
assert.strictEqual(typeof value, 'object');
test_reference.checkExternal(value);
value = null;
global.gc();
assert.strictEqual(test_reference.finalizeCount, 0);
// Run each test function in sequence,
// with an async delay and GC call between each.
function runTests(i, title, tests) {
if (!tests[i]) {
return;
} else if (typeof tests[i] === 'string') {
title = tests[i];
runTests(i + 1, title, tests);
} else {
try {
tests[i]();
} catch (e) {
console.error('Test failed: ' + title);
throw e;
}
setImmediate(() => {
global.gc();
runTests(i + 1, title, tests);
});
}
}
runTests(0, undefined, [

{
// External value with a finalizer
let value = test_reference.createExternalWithFinalize();
assert.strictEqual(test_reference.finalizeCount, 0);
assert.strictEqual(typeof value, 'object');
test_reference.checkExternal(value);
value = null;
global.gc();
assert.strictEqual(test_reference.finalizeCount, 1);
}

{
// Strong reference
let value = test_reference.createExternalWithFinalize();
assert.strictEqual(test_reference.finalizeCount, 0);
test_reference.createReference(value, 1);
assert.strictEqual(test_reference.referenceValue, value);
value = null;
global.gc(); // Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);
test_reference.deleteReference();
global.gc(); // Value should be GC'd because the strong ref was deleted
assert.strictEqual(test_reference.finalizeCount, 1);
}

{
// Strong reference, increment then decrement to weak reference
let value = test_reference.createExternalWithFinalize();
assert.strictEqual(test_reference.finalizeCount, 0);
test_reference.createReference(value, 1);
value = null;
global.gc(); // Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);
'External value without a finalizer',
() => {
let value = test_reference.createExternal();
assert.strictEqual(test_reference.finalizeCount, 0);
assert.strictEqual(typeof value, 'object');
test_reference.checkExternal(value);
},
() => {
assert.strictEqual(test_reference.finalizeCount, 0);
},

assert.strictEqual(test_reference.incrementRefcount(), 2);
global.gc(); // Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);

assert.strictEqual(test_reference.decrementRefcount(), 1);
global.gc(); // Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);
'External value with a finalizer',
() => {
let value = test_reference.createExternalWithFinalize();
assert.strictEqual(test_reference.finalizeCount, 0);
assert.strictEqual(typeof value, 'object');
test_reference.checkExternal(value);
},
() => {
assert.strictEqual(test_reference.finalizeCount, 1);
},

assert.strictEqual(test_reference.decrementRefcount(), 0);
global.gc(); // Value should be GC'd because the ref is now weak!
assert.strictEqual(test_reference.finalizeCount, 1);
'Weak reference',
() => {
let value = test_reference.createExternalWithFinalize();
assert.strictEqual(test_reference.finalizeCount, 0);
test_reference.createReference(value, 0);
assert.strictEqual(test_reference.referenceValue, value);
value = null;
},
() => {
// Value should be GC'd because there is only a weak ref
assert.strictEqual(test_reference.referenceValue, undefined);
assert.strictEqual(test_reference.finalizeCount, 1);
test_reference.deleteReference();
},

test_reference.deleteReference();
global.gc(); // Value was already GC'd
assert.strictEqual(test_reference.finalizeCount, 1);
}
'Strong reference',
() => {
let value = test_reference.createExternalWithFinalize();
assert.strictEqual(test_reference.finalizeCount, 0);
test_reference.createReference(value, 1);
assert.strictEqual(test_reference.referenceValue, value);
},
() => {
// Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);
test_reference.deleteReference();
},
() => {
// Value should be GC'd because the strong ref was deleted
assert.strictEqual(test_reference.finalizeCount, 1);
},

{
// Weak reference
let value = test_reference.createExternalWithFinalize();
assert.strictEqual(test_reference.finalizeCount, 0);
test_reference.createReference(value, 0);
assert.strictEqual(test_reference.referenceValue, value);
value = null;
setImmediate(common.mustCall(() => {
// This test only works if gc() is called from an immediate callback.
global.gc(); // Value should be GC'd because there is only a weak ref
assert.strictEqual(test_reference.referenceValue, undefined);
'Strong reference, increment then decrement to weak reference',
() => {
let value = test_reference.createExternalWithFinalize();
assert.strictEqual(test_reference.finalizeCount, 0);
test_reference.createReference(value, 1);
},
() => {
// Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);
assert.strictEqual(test_reference.incrementRefcount(), 2);
},
() => {
// Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);
assert.strictEqual(test_reference.decrementRefcount(), 1);
},
() => {
// Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);
assert.strictEqual(test_reference.decrementRefcount(), 0);
},
() => {
// Value should be GC'd because the ref is now weak!
assert.strictEqual(test_reference.finalizeCount, 1);
test_reference.deleteReference();
}));
}
},
() => {
// Value was already GC'd
assert.strictEqual(test_reference.finalizeCount, 1);
},
]);

0 comments on commit bb7eef7

Please sign in to comment.