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 <[email protected]>
PR-URL: nodejs/node-addon-api#762
Reviewed-By: Michael Dawson <[email protected]>

test: add finalizer exception test

src: wrap finalizer callback

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 <typename Callable>
+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 <typename Callable, typename Return>
 struct CallbackData {
   static inline
@@ -120,17 +138,21 @@ struct CallbackData<Callable, void> {
 template <typename T, typename Finalizer, typename Hint = void>
 struct FinalizeData {
   static inline
-  void Wrapper(napi_env env, void* data, void* finalizeHint) {
-    FinalizeData* finalizeData = static_cast<FinalizeData*>(finalizeHint);
-    finalizeData->callback(Env(env), static_cast<T*>(data));
-    delete finalizeData;
+  void Wrapper(napi_env env, void* data, void* finalizeHint) noexcept {
+    WrapVoidCallback([&] {
+      FinalizeData* finalizeData = static_cast<FinalizeData*>(finalizeHint);
+      finalizeData->callback(Env(env), static_cast<T*>(data));
+      delete finalizeData;
+    });
   }
 
   static inline
-  void WrapperWithHint(napi_env env, void* data, void* finalizeHint) {
-    FinalizeData* finalizeData = static_cast<FinalizeData*>(finalizeHint);
-    finalizeData->callback(Env(env), static_cast<T*>(data), finalizeData->hint);
-    delete finalizeData;
+  void WrapperWithHint(napi_env env, void* data, void* finalizeHint) noexcept {
+    WrapVoidCallback([&] {
+      FinalizeData* finalizeData = static_cast<FinalizeData*>(finalizeHint);
+      finalizeData->callback(Env(env), static_cast<T*>(data), finalizeData->hint);
+      delete finalizeData;
+    });
   }
 
   Finalizer callback;

test/external.cc

@@ -51,13 +51,28 @@ Value GetFinalizeCount(const CallbackInfo& info) {
    return Number::New(info.Env(), finalizeCount);
 }
 
+Value CreateExternalWithFinalizeException(const CallbackInfo& info) {
+  return External<int>::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) {
   Object exports = Object::New(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);

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',
     () => {