node-api: stop ref gc during environment teardown

gabrielschulhof · ruyadorno · commit d1a3e0efb65d · 2021-03-23T21:14:59.000-04:00
A gc may happen during environment teardown. Thus, during finalization initiated by environment teardown we must remove the V8 finalizer before calling the Node-API finalizer. Fixes: #37236 PR-URL: #37616 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
@@ -270,6 +270,20 @@ class RefBase : protected Finalizer, RefTracker {
 
  protected:
   inline void Finalize(bool is_env_teardown = false) override {
+    // In addition to being called during environment teardown, this method is
+    // also the entry point for the garbage collector. During environment
+    // teardown we have to remove the garbage collector's reference to this
+    // method so that, if, as part of the user's callback, JS gets executed,
+    // resulting in a garbage collection pass, this method is not re-entered as
+    // part of that pass, because that'll cause a double free (as seen in
+    // https://github.com/nodejs/node/issues/37236).
+    //
+    // Since this class does not have access to the V8 persistent reference,
+    // this method is overridden in the `Reference` class below. Therein the
+    // weak callback is removed, ensuring that the garbage collector does not
+    // re-enter this method, and the method chains up to continue the process of
+    // environment-teardown-induced finalization.
+
     // During environment teardown we have to convert a strong reference to
     // a weak reference to force the deferring behavior if the user's finalizer
     // happens to delete this reference so that the code in this function that
@@ -278,9 +292,10 @@ class RefBase : protected Finalizer, RefTracker {
     if (is_env_teardown && RefCount() > 0) _refcount = 0;
 
     if (_finalize_callback != nullptr) {
-      _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint);
       // This ensures that we never call the finalizer twice.
+      napi_finalize fini = _finalize_callback;
       _finalize_callback = nullptr;
+      _env->CallFinalizer(fini, _finalize_data, _finalize_hint);
     }
 
     // this is safe because if a request to delete the reference
@@ -355,6 +370,17 @@ class Reference : public RefBase {
     }
   }
 
+ protected:
+  inline void Finalize(bool is_env_teardown = false) override {
+    // During env teardown, `~napi_env()` alone is responsible for finalizing.
+    // Thus, we don't want any stray gc passes to trigger a second call to
+    // `Finalize()`, so let's reset the persistent here.
+    if (is_env_teardown) _persistent.ClearWeak();
+
+    // Chain up to perform the rest of the finalization.
+    RefBase::Finalize(is_env_teardown);
+  }
+
  private:
   // The N-API finalizer callback may make calls into the engine. V8's heap is
   // not in a consistent state during the weak callback, and therefore it does
diff --git a/test/node-api/test_env_teardown_gc/binding.c b/test/node-api/test_env_teardown_gc/binding.c
@@ -0,0 +1,37 @@
+#include <stdlib.h>
+#include <node_api.h>
+#include "../../js-native-api/common.h"
+
+static void MyObject_fini(napi_env env, void* data, void* hint) {
+  napi_ref* ref = data;
+  napi_value global;
+  napi_value cleanup;
+  NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &global));
+  NODE_API_CALL_RETURN_VOID(
+      env, napi_get_named_property(env, global, "cleanup", &cleanup));
+  napi_status status = napi_call_function(env, global, cleanup, 0, NULL, NULL);
+  // We may not be allowed to call into JS, in which case a pending exception
+  // will be returned.
+  NODE_API_ASSERT_RETURN_VOID(env,
+      status == napi_ok || status == napi_pending_exception,
+      "Unexpected status for napi_call_function");
+  NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref));
+  free(ref);
+}
+
+static napi_value MyObject(napi_env env, napi_callback_info info) {
+  napi_value js_this;
+  napi_ref* ref = malloc(sizeof(*ref));
+  NODE_API_CALL(env, napi_get_cb_info(env, info, NULL, NULL, &js_this, NULL));
+  NODE_API_CALL(env, napi_wrap(env, js_this, ref, MyObject_fini, NULL, ref));
+  return NULL;
+}
+
+NAPI_MODULE_INIT() {
+  napi_value ctor;
+  NODE_API_CALL(
+      env, napi_define_class(
+          env, "MyObject", NAPI_AUTO_LENGTH, MyObject, NULL, 0, NULL, &ctor));
+  NODE_API_CALL(env, napi_set_named_property(env, exports, "MyObject",  ctor));
+  return exports;
+}
diff --git a/test/node-api/test_env_teardown_gc/binding.gyp b/test/node-api/test_env_teardown_gc/binding.gyp
@@ -0,0 +1,8 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'binding.c' ]
+    }
+  ]
+}
diff --git a/test/node-api/test_env_teardown_gc/test.js b/test/node-api/test_env_teardown_gc/test.js
@@ -0,0 +1,14 @@
+'use strict';
+// Flags: --expose-gc
+
+process.env.NODE_TEST_KNOWN_GLOBALS = 0;
+
+const common = require('../../common');
+const binding = require(`./build/${common.buildType}/binding`);
+
+global.it = new binding.MyObject();
+
+global.cleanup = () => {
+  delete global.it;
+  global.gc();
+};
