n-api: run all finalizers via SetImmediate()

Throwing an exception from a finalizer can cause the following fatal error: Error: async hook stack has become corrupted (actual: 2, expected: 0) 1: 0x970b5a node::InternalCallbackScope::~InternalCallbackScope() [./node] 2: 0x99dda0 node::Environment::RunTimers(uv_timer_s*) [./node] 3: 0x13d8b22 [./node] 4: 0x13dbe42 uv_run [./node] 5: 0xa57974 node::NodeMainInstance::Run() [./node] 6: 0x9dbc17 node::Start(int, char**) [./node] 7: 0x7f4965417f43 __libc_start_main [/lib64/libc.so.6] 8: 0x96f4ae _start [./node] By #34341 (comment), calling into JS from a finalizer and/or throwing exceptions from there is not advised, because the stack may or may not be set up for JS execution. The best solution is to run the user's finalizer from a `SetImmediate()` callback. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Fixes: #34341
gabrielschulhof · Jul 15, 2020 · Jul 23, 2020 · Jul 23, 2020 · f04eaa7447803ac419bc794cba42ce466dec01b4 · gabrielschulhof
commit f04eaa7447803ac419bc794cba42ce466dec01b4
diff --git a/benchmark/napi/ref/index.js b/benchmark/napi/ref/index.js
@@ -10,8 +10,10 @@ function callNewWeak() {
 function main({ n }) {
   addon.count = 0;
   bench.start();
-  while (addon.count < n) {
-    callNewWeak();
-  }
-  bench.end(n);
+  new Promise((resolve) => {
+    (function oneIteration() {
+      callNewWeak();
+      setImmediate(() => ((addon.count < n) ? oneIteration() : resolve()));
+    })();
+  }).then(() => bench.end(n));
 }
diff --git 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
@@ -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<v8::Value> 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
@@ -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<napi_env>(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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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.match(child.stderr.toString(), /Error during Finalize/m);
diff --git a/test/js-native-api/test_general/test.js b/test/js-native-api/test_general/test.js
@@ -51,46 +51,46 @@ 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 = {};
 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
@@ -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();