diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index daad1907349d63..e99333a6a362d1 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -267,13 +267,7 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { if (_finalize_callback != nullptr) { - v8::HandleScope handle_scope(_env->isolate); - _env->CallIntoModule([&](napi_env env) { - _finalize_callback( - env, - _finalize_data, - _finalize_hint); - }); + _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint); } // this is safe because if a request to delete the reference diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 9c737f3c9cc9fc..83e6a0bd02e23c 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -101,6 +101,13 @@ struct napi_env__ { } } + virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) { + v8::HandleScope handle_scope(isolate); + CallIntoModule([&](napi_env env) { + cb(env, data, hint); + }); + } + v8impl::Persistent last_exception; // We store references in two different lists, depending on whether they have diff --git a/src/node_api.cc b/src/node_api.cc index 314e2c57a0493f..bb203fc03c310f 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -32,6 +32,17 @@ struct node_napi_env__ : public napi_env__ { node_env()->untransferable_object_private_symbol(), v8::True(isolate)); } + + void CallFinalizer(napi_finalize cb, void* data, void* hint) override { + napi_env env = static_cast(this); + node_env()->SetImmediate([=](node::Environment* node_env) { + v8::HandleScope handle_scope(env->isolate); + v8::Context::Scope context_scope(env->context()); + env->CallIntoModule([&](napi_env env) { + cb(env, data, hint); + }); + }); + } }; typedef node_napi_env__* node_napi_env; diff --git a/test/common/index.js b/test/common/index.js index a027500254eaac..1bdf85a349e346 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -664,6 +664,30 @@ function skipIfDumbTerminal() { } } +function gcUntil(name, condition) { + if (typeof name === 'function') { + condition = name; + name = undefined; + } + return new Promise((resolve, reject) => { + let count = 0; + function gcAndCheck() { + setImmediate(() => { + count++; + global.gc(); + if (condition()) { + resolve(); + } else if (count < 10) { + gcAndCheck(); + } else { + reject(name === undefined ? undefined : 'Test ' + name + ' failed'); + } + }); + } + gcAndCheck(); + }); +} + const common = { allowGlobals, buildType, @@ -673,6 +697,7 @@ const common = { disableCrashOnUnhandledRejection, expectsError, expectWarning, + gcUntil, getArrayBufferViews, getBufferSources, getCallSite, diff --git a/test/js-native-api/7_factory_wrap/test.js b/test/js-native-api/7_factory_wrap/test.js index 8aaf1b0ba91ee7..ff1516eaa5a092 100644 --- a/test/js-native-api/7_factory_wrap/test.js +++ b/test/js-native-api/7_factory_wrap/test.js @@ -6,20 +6,21 @@ const assert = require('assert'); const test = require(`./build/${common.buildType}/binding`); assert.strictEqual(test.finalizeCount, 0); -(() => { - const obj = test.createObject(10); - assert.strictEqual(obj.plusOne(), 11); - assert.strictEqual(obj.plusOne(), 12); - assert.strictEqual(obj.plusOne(), 13); -})(); -global.gc(); -assert.strictEqual(test.finalizeCount, 1); +async function runGCTests() { + (() => { + const obj = test.createObject(10); + assert.strictEqual(obj.plusOne(), 11); + assert.strictEqual(obj.plusOne(), 12); + assert.strictEqual(obj.plusOne(), 13); + })(); + await common.gcUntil('test 1', () => (test.finalizeCount === 1)); -(() => { - const obj2 = test.createObject(20); - assert.strictEqual(obj2.plusOne(), 21); - assert.strictEqual(obj2.plusOne(), 22); - assert.strictEqual(obj2.plusOne(), 23); -})(); -global.gc(); -assert.strictEqual(test.finalizeCount, 2); + (() => { + const obj2 = test.createObject(20); + assert.strictEqual(obj2.plusOne(), 21); + assert.strictEqual(obj2.plusOne(), 22); + assert.strictEqual(obj2.plusOne(), 23); + })(); + await common.gcUntil('test 2', () => (test.finalizeCount === 2)); +} +runGCTests(); diff --git a/test/js-native-api/8_passing_wrapped/test.js b/test/js-native-api/8_passing_wrapped/test.js index 525993c96d3bad..54c829d9c77dff 100644 --- a/test/js-native-api/8_passing_wrapped/test.js +++ b/test/js-native-api/8_passing_wrapped/test.js @@ -5,13 +5,16 @@ const common = require('../../common'); const assert = require('assert'); const addon = require(`./build/${common.buildType}/binding`); -let obj1 = addon.createObject(10); -let obj2 = addon.createObject(20); -const result = addon.add(obj1, obj2); -assert.strictEqual(result, 30); +async function runTest() { + let obj1 = addon.createObject(10); + let obj2 = addon.createObject(20); + const result = addon.add(obj1, obj2); + assert.strictEqual(result, 30); -// Make sure the native destructor gets called. -obj1 = null; -obj2 = null; -global.gc(); -assert.strictEqual(addon.finalizeCount(), 2); + // Make sure the native destructor gets called. + obj1 = null; + obj2 = null; + await common.gcUntil('8_passing_wrapped', + () => (addon.finalizeCount() === 2)); +} +runTest(); diff --git a/test/js-native-api/test_exception/test.js b/test/js-native-api/test_exception/test.js index 6ec878453f0c22..ff11b702198b88 100644 --- a/test/js-native-api/test_exception/test.js +++ b/test/js-native-api/test_exception/test.js @@ -66,14 +66,3 @@ const test_exception = (function() { 'Exception state did not remain clear as expected,' + ` .wasPending() returned ${exception_pending}`); } - -// Make sure that exceptions that occur during finalization are propagated. -function testFinalize(binding) { - let x = test_exception[binding](); - x = null; - assert.throws(() => { global.gc(); }, /Error during Finalize/); - - // To assuage the linter's concerns. - (function() {})(x); -} -testFinalize('createExternal'); diff --git a/test/js-native-api/test_exception/testFinalizerException.js b/test/js-native-api/test_exception/testFinalizerException.js new file mode 100644 index 00000000000000..1c0b58477519e0 --- /dev/null +++ b/test/js-native-api/test_exception/testFinalizerException.js @@ -0,0 +1,22 @@ +'use strict'; +if (process.argv[2] === 'child') { + const common = require('../../common'); + // Trying, catching the exception, and finding the bindings at the `Error`'s + // `binding` property is done intentionally, because we're also testing what + // happens when the add-on entry point throws. See test.js. + try { + require(`./build/${common.buildType}/test_exception`); + } catch (anException) { + anException.binding.createExternal(); + } + const interval = setInterval(() => { + clearInterval(interval); + }, 100); + return; +} + +const assert = require('assert'); +const { spawnSync } = require('child_process'); +const child = spawnSync(process.execPath, [ __filename, 'child' ]); +assert.strictEqual(child.signal, null); +assert(!!child.stderr.toString().match(/Error during Finalize/m)); diff --git a/test/js-native-api/test_general/test.js b/test/js-native-api/test_general/test.js index aa3a4eedc56634..de06aecb590529 100644 --- a/test/js-native-api/test_general/test.js +++ b/test/js-native-api/test_general/test.js @@ -51,24 +51,13 @@ assert.strictEqual(test_general.testGetVersion(), 6); // for null 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); -w = null; -global.gc(); -const derefItemWasCalled = test_general.derefItemWasCalled(); -assert.strictEqual(derefItemWasCalled, true, - 'deref_item() was called upon garbage collecting a ' + - 'wrapped object. test_general.derefItemWasCalled() ' + - `returned ${derefItemWasCalled}`); - - // Assert that wrapping twice fails. const x = {}; test_general.wrap(x); assert.throws(() => test_general.wrap(x), { name: 'Error', message: 'Invalid argument' }); +// Clean up here, otherwise derefItemWasCalled() will be polluted. +test_general.removeWrap(x); // Ensure that wrapping, removing the wrap, and then wrapping again works. const y = {}; @@ -76,21 +65,32 @@ test_general.wrap(y); test_general.removeWrap(y); // Wrapping twice succeeds if a remove_wrap() separates the instances test_general.wrap(y); - -// Ensure that removing a wrap and garbage collecting does not fire the -// finalize callback. -let z = {}; -test_general.testFinalizeWrap(z); -test_general.removeWrap(z); -z = null; -global.gc(); -const finalizeWasCalled = test_general.finalizeWasCalled(); -assert.strictEqual(finalizeWasCalled, false, - 'finalize callback was not called upon garbage collection.' + - ' test_general.finalizeWasCalled() ' + - `returned ${finalizeWasCalled}`); +// Clean up here, otherwise derefItemWasCalled() will be polluted. +test_general.removeWrap(y); // Test napi_adjust_external_memory const adjustedValue = test_general.testAdjustExternalMemory(); assert.strictEqual(typeof adjustedValue, 'number'); assert(adjustedValue > 0); + +async function runGCTests() { + // Ensure that garbage collecting an object with a wrapped native item results + // in the finalize callback being called. + assert.strictEqual(test_general.derefItemWasCalled(), false); + + (() => test_general.wrap({}))(); + await common.gcUntil('deref_item() was called upon garbage collecting a ' + + 'wrapped object.', + () => test_general.derefItemWasCalled()); + + // Ensure that removing a wrap and garbage collecting does not fire the + // finalize callback. + let z = {}; + test_general.testFinalizeWrap(z); + test_general.removeWrap(z); + z = null; + await common.gcUntil( + 'finalize callback was not called upon garbage collection.', + () => (!test_general.finalizeWasCalled())); +} +runGCTests(); diff --git a/test/js-native-api/test_general/testFinalizer.js b/test/js-native-api/test_general/testFinalizer.js index d72a4a44a304d8..54265d61bc37ff 100644 --- a/test/js-native-api/test_general/testFinalizer.js +++ b/test/js-native-api/test_general/testFinalizer.js @@ -24,9 +24,13 @@ global.gc(); // Add an item to an object that is already wrapped, and ensure that its // finalizer as well as the wrap finalizer gets called. -let finalizeAndWrap = {}; -test_general.wrap(finalizeAndWrap); -test_general.addFinalizerOnly(finalizeAndWrap, common.mustCall()); -finalizeAndWrap = null; -global.gc(); -assert.strictEqual(test_general.derefItemWasCalled(), true); +async function testFinalizeAndWrap() { + assert.strictEqual(test_general.derefItemWasCalled(), false); + let finalizeAndWrap = {}; + test_general.wrap(finalizeAndWrap); + test_general.addFinalizerOnly(finalizeAndWrap, common.mustCall()); + finalizeAndWrap = null; + await common.gcUntil('test finalize and wrap', + () => test_general.derefItemWasCalled()); +} +testFinalizeAndWrap(); diff --git a/test/js-native-api/test_reference/test.js b/test/js-native-api/test_reference/test.js index 389ee11d7e5f5b..0c9d13075f2aa0 100644 --- a/test/js-native-api/test_reference/test.js +++ b/test/js-native-api/test_reference/test.js @@ -1,10 +1,10 @@ 'use strict'; // Flags: --expose-gc -const common = require('../../common'); +const { gcUntil, buildType } = require('../../common'); const assert = require('assert'); -const test_reference = require(`./build/${common.buildType}/test_reference`); +const test_reference = require(`./build/${buildType}/test_reference`); // This test script uses external values with finalizer callbacks // in order to track when values get garbage-collected. Each invocation @@ -13,111 +13,80 @@ 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]) { - 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 without a finalizer', - () => { +async function runTests() { + (() => { const 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); - }, + })(); + await gcUntil('External value without a finalizer', + () => (test_reference.finalizeCount === 0)); - 'External value with a finalizer', - () => { + (() => { const 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); - }, + })(); + await gcUntil('External value with a finalizer', + () => (test_reference.finalizeCount === 1)); - 'Weak reference', - () => { + (() => { const value = test_reference.createExternalWithFinalize(); assert.strictEqual(test_reference.finalizeCount, 0); test_reference.createReference(value, 0); assert.strictEqual(test_reference.referenceValue, value); - }, - () => { - // 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(); - }, + })(); + // Value should be GC'd because there is only a weak ref + await gcUntil('Weak reference', + () => (test_reference.referenceValue === undefined && + test_reference.finalizeCount === 1)); + test_reference.deleteReference(); - 'Strong reference', - () => { + (() => { const 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); - }, + })(); + // Value should NOT be GC'd because there is a strong ref + await gcUntil('Strong reference', + () => (test_reference.finalizeCount === 0)); + test_reference.deleteReference(); + await gcUntil('Strong reference (cont.d)', + () => (test_reference.finalizeCount === 1)); - 'Strong reference, increment then decrement to weak reference', - () => { + (() => { const 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); - }, -]); + })(); + // Value should NOT be GC'd because there is a strong ref + await gcUntil('Strong reference, increment then decrement to weak reference', + () => (test_reference.finalizeCount === 0)); + assert.strictEqual(test_reference.incrementRefcount(), 2); + // Value should NOT be GC'd because there is a strong ref + await gcUntil( + 'Strong reference, increment then decrement to weak reference (cont.d-1)', + () => (test_reference.finalizeCount === 0)); + assert.strictEqual(test_reference.decrementRefcount(), 1); + // Value should NOT be GC'd because there is a strong ref + await gcUntil( + 'Strong reference, increment then decrement to weak reference (cont.d-2)', + () => (test_reference.finalizeCount === 0)); + assert.strictEqual(test_reference.decrementRefcount(), 0); + // Value should be GC'd because the ref is now weak! + await gcUntil( + 'Strong reference, increment then decrement to weak reference (cont.d-3)', + () => (test_reference.finalizeCount === 1)); + test_reference.deleteReference(); + // Value was already GC'd + await gcUntil( + 'Strong reference, increment then decrement to weak reference (cont.d-4)', + () => (test_reference.finalizeCount === 1)); +} +runTests(); // This test creates a napi_ref on an object that has // been wrapped by napi_wrap and for which the finalizer