From 25c00516588cbc72ea294f0ebfd994dae1815fa8 Mon Sep 17 00:00:00 2001 From: Marly Fleitas Date: Thu, 9 Jul 2020 11:00:27 -0700 Subject: [PATCH] src: wrap finalizer callback Make sure C++ exceptions thrown from a finalizer are converted into JS exceptions just as they are in regular callbacks. Signed-off-by: Gabriel Schulhof PR-URL: https://github.com/nodejs/node-addon-api/pull/762 Reviewed-By: Michael Dawson test: add finalizer exception test src: wrap finalizer callback --- napi-inl.h | 38 +++++++++++++++++++++++++++-------- test/external.cc | 15 ++++++++++++++ test/external.js | 52 +++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 94 insertions(+), 11 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index a18ffcd..4f0636a 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -82,6 +82,24 @@ inline napi_value WrapCallback(Callable callback) { #endif // NAPI_CPP_EXCEPTIONS } +// For use in JS to C++ void callback wrappers to catch any Napi::Error +// exceptions and rethrow them as JavaScript exceptions before returning from the +// callback. +template +inline void WrapVoidCallback(Callable callback) { +#ifdef NAPI_CPP_EXCEPTIONS + try { + callback(); + } catch (const Error& e) { + e.ThrowAsJavaScriptException(); + } +#else // NAPI_CPP_EXCEPTIONS + // When C++ exceptions are disabled, errors are immediately thrown as JS + // exceptions, so there is no need to catch and rethrow them here. + callback(); +#endif // NAPI_CPP_EXCEPTIONS +} + template struct CallbackData { static inline @@ -120,17 +138,21 @@ struct CallbackData { template struct FinalizeData { static inline - void Wrapper(napi_env env, void* data, void* finalizeHint) { - FinalizeData* finalizeData = static_cast(finalizeHint); - finalizeData->callback(Env(env), static_cast(data)); - delete finalizeData; + void Wrapper(napi_env env, void* data, void* finalizeHint) noexcept { + WrapVoidCallback([&] { + FinalizeData* finalizeData = static_cast(finalizeHint); + finalizeData->callback(Env(env), static_cast(data)); + delete finalizeData; + }); } static inline - void WrapperWithHint(napi_env env, void* data, void* finalizeHint) { - FinalizeData* finalizeData = static_cast(finalizeHint); - finalizeData->callback(Env(env), static_cast(data), finalizeData->hint); - delete finalizeData; + void WrapperWithHint(napi_env env, void* data, void* finalizeHint) noexcept { + WrapVoidCallback([&] { + FinalizeData* finalizeData = static_cast(finalizeHint); + finalizeData->callback(Env(env), static_cast(data), finalizeData->hint); + delete finalizeData; + }); } Finalizer callback; diff --git a/test/external.cc b/test/external.cc index 25dacc4..9c22dcb 100644 --- a/test/external.cc +++ b/test/external.cc @@ -51,6 +51,19 @@ Value GetFinalizeCount(const CallbackInfo& info) { return Number::New(info.Env(), finalizeCount); } +Value CreateExternalWithFinalizeException(const CallbackInfo& info) { + return External::New(info.Env(), new int(1), + [](Env env, int* data) { + Error error = Error::New(env, "Finalizer exception"); + delete data; +#ifdef NAPI_CPP_EXCEPTIONS + throw error; +#else + error.ThrowAsJavaScriptException(); +#endif + }); +} + } // end anonymous namespace Object InitExternal(Env env) { @@ -58,6 +71,8 @@ Object InitExternal(Env env) { exports["createExternal"] = Function::New(env, CreateExternal); exports["createExternalWithFinalize"] = Function::New(env, CreateExternalWithFinalize); + exports["createExternalWithFinalizeException"] = + Function::New(env, CreateExternalWithFinalizeException); exports["createExternalWithFinalizeHint"] = Function::New(env, CreateExternalWithFinalizeHint); exports["checkExternal"] = Function::New(env, CheckExternal); exports["getFinalizeCount"] = Function::New(env, GetFinalizeCount); diff --git a/test/external.js b/test/external.js index 85dbe70..0443e3f 100644 --- a/test/external.js +++ b/test/external.js @@ -1,12 +1,58 @@ 'use strict'; const buildType = process.config.target_defaults.default_configuration; const assert = require('assert'); +const { spawnSync } = require('child_process'); const testUtil = require('./testUtil'); -module.exports = test(require(`./build/${buildType}/binding.node`)) - .then(() => test(require(`./build/${buildType}/binding_noexcept.node`))); +if (process.argv.length === 3) { + let interval; + + // Running as the child process, hook up an `uncaughtException` handler to + // examine the error thrown by the finalizer. + process.on('uncaughtException', (error) => { + // TODO (gabrielschulhof): Use assert.matches() when we drop support for + // Node.js v10.x. + assert(!!error.message.match(/Finalizer exception/)); + if (interval) { + clearInterval(interval); + } + process.exit(0); + }); + + // Create an external whose finalizer throws. + (() => + require(process.argv[2]).external.createExternalWithFinalizeException())(); + + // gc until the external's finalizer throws or until we give up. Since the + // exception is thrown from a native `SetImmediate()` we cannot catch it + // anywhere except in the process' `uncaughtException` handler. + let maxGCTries = 10; + (function gcInterval() { + global.gc(); + if (!interval) { + interval = setInterval(gcInterval, 100); + } else if (--maxGCTries === 0) { + throw new Error('Timed out waiting for the gc to throw'); + process.exit(1); + } + })(); + + return; +} + +module.exports = test(require.resolve(`./build/${buildType}/binding.node`)) + .then(() => + test(require.resolve(`./build/${buildType}/binding_noexcept.node`))); + +function test(bindingPath) { + const binding = require(bindingPath); + + const child = spawnSync(process.execPath, [ + '--expose-gc', __filename, bindingPath + ], { stdio: 'inherit' }); + assert.strictEqual(child.status, 0); + assert.strictEqual(child.signal, null); -function test(binding) { return testUtil.runGCTests([ 'External without finalizer', () => {