add test to crash on GC state access and update docs

Author: vmoroz

doc/api/n-api.md

@@ -5468,6 +5468,42 @@ invocation. If it is deleted before then, then the finalize callback may never
 be invoked. Therefore, when obtaining a reference a finalize callback is also
 required in order to enable correct disposal of the reference.
 
+#### `node_api_post_finalizer`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+```c
+napi_status node_api_post_finalizer(napi_env env,
+                                    napi_finalize finalize_cb,
+                                    void* finalize_data,
+                                    void* finalize_hint);
+```
+
+* `[in] env`: The environment that the API is invoked under.
+* `[in] finalize_cb`: Native callback that will be used to free the
+  native data when the JavaScript object has been garbage-collected.
+  [`napi_finalize`][] provides more details.
+* `[in] finalize_data`: Optional data to be passed to `finalize_cb`.
+* `[in] finalize_hint`: Optional contextual hint that is passed to the
+  finalize callback.
+
+Returns `napi_ok` if the API succeeded.
+
+Schedules `napi_finalize` callback to be called asynchronously in the
+event loop.
+
+This API must be called inside of a finalizer if it must call any code
+that may affect the state of GC (garbage collector).
+
+The finalizers are called while GC collects objects. At that point of time
+calling any API that may cause changes in GC state will cause unpredictable
+behavior and crashes. The `node_api_post_finalizer` helps to work around
+this limitation by running code outside of the GC finalization.
+
 ## Simple asynchronous operations
 
 Addon modules often need to leverage async helpers from libuv as part of their

src/js_native_api_v8.cc

@@ -62,14 +62,14 @@ void napi_env__::InvokeFinalizerFromGC(v8impl::RefTracker* finalizer) {
     EnqueueFinalizer(finalizer);
   } else {
     // The experimental code calls finalizers immediately to release native
-    // objects as soon as possible, but it suspends use of JS from finalizer.
-    // If JS calls are needed, then the finalizer code must call
-    // node_api_post_finalizer.
+    // objects as soon as possible. In that state any code that may affect GC
+    // state causes a fatal error. To work around this issue the finalizer code
+    // must call node_api_post_finalizer.
     if (last_error.error_code == napi_ok && last_exception.IsEmpty()) {
-      bool saved_suspend_call_into_js = suspend_call_into_js;
-      suspend_call_into_js = true;
+      auto restore_state = node::OnScopeLeave(
+          [this, saved = in_gc_finalizer] { in_gc_finalizer = saved; });
+      in_gc_finalizer = true;
       finalizer->Finalize();
-      suspend_call_into_js = saved_suspend_call_into_js;
     } else {
       // The finalizers can be run in the middle of JS or C++ code.
       // That code may be in an error state. In that case use the asynchronous
@@ -93,6 +93,7 @@ napi_status NewString(napi_env env,
   CHECK_ARG(env, result);
   RETURN_STATUS_IF_FALSE(
       env, (length == NAPI_AUTO_LENGTH) || length <= INT_MAX, napi_invalid_arg);
+  env->CheckGCAccess();
 
   auto isolate = env->isolate;
   auto str_maybe = string_maker(isolate);
@@ -1539,6 +1540,7 @@ napi_status NAPI_CDECL napi_get_prototype(napi_env env,
 napi_status NAPI_CDECL napi_create_object(napi_env env, napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result = v8impl::JsValueFromV8LocalValue(v8::Object::New(env->isolate));
 
@@ -1548,6 +1550,7 @@ napi_status NAPI_CDECL napi_create_object(napi_env env, napi_value* result) {
 napi_status NAPI_CDECL napi_create_array(napi_env env, napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result = v8impl::JsValueFromV8LocalValue(v8::Array::New(env->isolate));
 
@@ -1559,6 +1562,7 @@ napi_status NAPI_CDECL napi_create_array_with_length(napi_env env,
                                                      napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result =
       v8impl::JsValueFromV8LocalValue(v8::Array::New(env->isolate, length));
@@ -1659,6 +1663,7 @@ napi_status NAPI_CDECL napi_create_double(napi_env env,
                                           napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result =
       v8impl::JsValueFromV8LocalValue(v8::Number::New(env->isolate, value));
@@ -1671,6 +1676,7 @@ napi_status NAPI_CDECL napi_create_int32(napi_env env,
                                          napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result =
       v8impl::JsValueFromV8LocalValue(v8::Integer::New(env->isolate, value));
@@ -1683,6 +1689,7 @@ napi_status NAPI_CDECL napi_create_uint32(napi_env env,
                                           napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result = v8impl::JsValueFromV8LocalValue(
       v8::Integer::NewFromUnsigned(env->isolate, value));
@@ -1695,6 +1702,7 @@ napi_status NAPI_CDECL napi_create_int64(napi_env env,
                                          napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result = v8impl::JsValueFromV8LocalValue(
       v8::Number::New(env->isolate, static_cast<double>(value)));
@@ -1707,6 +1715,7 @@ napi_status NAPI_CDECL napi_create_bigint_int64(napi_env env,
                                                 napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result =
       v8impl::JsValueFromV8LocalValue(v8::BigInt::New(env->isolate, value));
@@ -1719,6 +1728,7 @@ napi_status NAPI_CDECL napi_create_bigint_uint64(napi_env env,
                                                  napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   *result = v8impl::JsValueFromV8LocalValue(
       v8::BigInt::NewFromUnsigned(env->isolate, value));
@@ -1734,6 +1744,7 @@ napi_status NAPI_CDECL napi_create_bigint_words(napi_env env,
   NAPI_PREAMBLE(env);
   CHECK_ARG(env, words);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   v8::Local<v8::Context> context = env->context();
 
@@ -1753,6 +1764,7 @@ napi_status NAPI_CDECL napi_get_boolean(napi_env env,
                                         napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   v8::Isolate* isolate = env->isolate;
 
@@ -1770,6 +1782,7 @@ napi_status NAPI_CDECL napi_create_symbol(napi_env env,
                                           napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   v8::Isolate* isolate = env->isolate;
 
@@ -1792,6 +1805,7 @@ napi_status NAPI_CDECL node_api_symbol_for(napi_env env,
                                            napi_value* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   napi_value js_description_string;
   STATUS_CALL(napi_create_string_utf8(
@@ -1838,6 +1852,7 @@ napi_status NAPI_CDECL napi_create_error(napi_env env,
   CHECK_ENV(env);
   CHECK_ARG(env, msg);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   v8::Local<v8::Value> message_value = v8impl::V8LocalValueFromJsValue(msg);
   RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected);
@@ -1858,6 +1873,7 @@ napi_status NAPI_CDECL napi_create_type_error(napi_env env,
   CHECK_ENV(env);
   CHECK_ARG(env, msg);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   v8::Local<v8::Value> message_value = v8impl::V8LocalValueFromJsValue(msg);
   RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected);
@@ -1878,6 +1894,7 @@ napi_status NAPI_CDECL napi_create_range_error(napi_env env,
   CHECK_ENV(env);
   CHECK_ARG(env, msg);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   v8::Local<v8::Value> message_value = v8impl::V8LocalValueFromJsValue(msg);
   RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected);
@@ -1898,6 +1915,7 @@ napi_status NAPI_CDECL node_api_create_syntax_error(napi_env env,
   CHECK_ENV(env);
   CHECK_ARG(env, msg);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   v8::Local<v8::Value> message_value = v8impl::V8LocalValueFromJsValue(msg);
   RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected);

src/js_native_api_v8.h

@@ -68,7 +68,7 @@ struct napi_env__ {
     if (--refs == 0) DeleteMe();
   }
 
-  virtual bool can_call_into_js() const { return !suspend_call_into_js; }
+  virtual bool can_call_into_js() const { return true; }
 
   static inline void HandleThrow(napi_env env, v8::Local<v8::Value> value) {
     if (env->terminatedOrTerminating()) {
@@ -102,7 +102,6 @@ struct napi_env__ {
   // Call finalizer immediately.
   virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
     v8::HandleScope handle_scope(isolate);
-    v8::Context::Scope context_scope(context());
     CallIntoModule([&](napi_env env) { cb(env, data, hint); });
   }
 
@@ -134,6 +133,19 @@ struct napi_env__ {
     delete this;
   }
 
+  void CheckGCAccess() {
+    if (module_api_version == NAPI_VERSION_EXPERIMENTAL && in_gc_finalizer) {
+      v8impl::OnFatalError(
+          nullptr,
+          "Finalizer is calling a function that may affect GC state.\n"
+          "The finalizers are run directly from GC and must not affect GC "
+          "state.\n"
+          "Use `node_api_post_finalizer` from inside of the finalizer to work "
+          "around this issue.\n"
+          "It schedules the call as a new task in the event loop.");
+    }
+  }
+
   v8::Isolate* const isolate;  // Shortcut for context()->GetIsolate()
   v8impl::Persistent<v8::Context> context_persistent;
 
@@ -152,7 +164,7 @@ struct napi_env__ {
   int refs = 1;
   void* instance_data = nullptr;
   int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION;
-  bool suspend_call_into_js = false;
+  bool in_gc_finalizer = false;
 
  protected:
   // Should not be deleted directly. Delete with `napi_env__::DeleteMe()`
@@ -218,6 +230,7 @@ inline napi_status napi_set_last_error(napi_env env,
   CHECK_ENV((env));                                                            \
   RETURN_STATUS_IF_FALSE(                                                      \
       (env), (env)->last_exception.IsEmpty(), napi_pending_exception);         \
+  (env)->CheckGCAccess();                                                      \
   RETURN_STATUS_IF_FALSE((env),                                                \
                          (env)->can_call_into_js(),                            \
                          (env->module_api_version == NAPI_VERSION_EXPERIMENTAL \

src/js_native_api_v8_internals.h

@@ -17,6 +17,7 @@
 
 #include "env.h"
 #include "gtest/gtest_prod.h"
+#include "node_errors.h"
 #include "node_internals.h"
 
 #define NAPI_ARRAYSIZE(array) node::arraysize((array))
@@ -34,6 +35,11 @@ using Persistent = v8::Global<T>;
 
 using PersistentToLocal = node::PersistentToLocal;
 
+[[noreturn]] inline void OnFatalError(const char* location,
+                                      const char* message) {
+  node::OnFatalError(location, message);
+}
+
 }  // end of namespace v8impl
 
 #endif  // SRC_JS_NATIVE_API_V8_INTERNALS_H_

src/node_api.cc

@@ -33,7 +33,7 @@ void node_napi_env__::DeleteMe() {
 }
 
 bool node_napi_env__::can_call_into_js() const {
-  return Super::can_call_into_js() && node_env()->can_call_into_js();
+  return node_env()->can_call_into_js();
 }
 
 void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) {

src/node_api_internals.h

@@ -9,8 +9,6 @@
 #include "util-inl.h"
 
 struct node_napi_env__ : public napi_env__ {
-  using Super = napi_env__;
-
   node_napi_env__(v8::Local<v8::Context> context,
                   const std::string& module_filename,
                   int32_t module_api_version);

test/js-native-api/test_finalizer/test.js

@@ -5,15 +5,12 @@ const common = require('../../common');
 const test_finalizer = require(`./build/${common.buildType}/test_finalizer`);
 const assert = require('assert');
 
-function addFinalizer() {
-  // Define obj in a function context to let it GC-collected.
+(() => {
   const obj = {};
   test_finalizer.addFinalizer(obj);
-}
-
-addFinalizer();
+})();
 
-for (let i = 0; i < 1000; ++i) {
+for (let i = 0; i < 10; ++i) {
   global.gc();
   if (test_finalizer.getFinalizerCallCount() === 1) {
     break;
@@ -28,7 +25,7 @@ async function runAsyncTests() {
     const obj = {};
     test_finalizer.addFinalizerWithJS(obj, () => { js_is_called = true; });
   })();
-  await common.gcUntil('test JS finalizer',
+  await common.gcUntil('ensure JS finalizer called',
                        () => (test_finalizer.getFinalizerCallCount() === 2));
   assert(js_is_called);
 }

test/js-native-api/test_finalizer/test_fatal_finalize.js

@@ -0,0 +1,31 @@
+'use strict';
+
+if (process.argv[2] === 'child') {
+  const common = require('../../common');
+  const test_finalizer = require(`./build/${common.buildType}/test_finalizer`);
+
+  (() => {
+    const obj = {};
+    test_finalizer.addFinalizerFailOnJS(obj);
+  })();
+
+  // Collect garbage 10 times. At least one of those should throw the exception
+  // and cause the whole process to bail with it, its text printed to stderr and
+  // asserted by the parent process to match expectations.
+  let gcCount = 10;
+  (function gcLoop() {
+    global.gc();
+    if (--gcCount > 0) {
+      setImmediate(() => gcLoop());
+    }
+  })();
+  return;
+}
+
+const assert = require('assert');
+const { spawnSync } = require('child_process');
+const child = spawnSync(process.execPath, [
+  '--expose-gc', __filename, 'child',
+]);
+assert.strictEqual(child.signal, null);
+assert.match(child.stderr.toString(), /Finalizer is calling a function that may affect GC state/);