Address PR feedback

Call_while_GC.txt

@@ -0,0 +1,18 @@
+safe functions:
+napi_get_last_error_info
+napi_get_cb_info
+napi_get_new_target
+napi_get_version
+node_api_post_finalizer
+napi_adjust_external_memory
+napi_set_instance_data
+napi_get_instance_data
+napi_add_env_cleanup_hook
+napi_remove_env_cleanup_hook
+napi_add_async_cleanup_hook
+napi_remove_async_cleanup_hook
+napi_fatal_error
+napi_open_callback_scope
+napi_close_callback_scope
+napi_get_node_version
+node_api_get_module_file_name

doc/api/n-api.md

@@ -5456,13 +5456,13 @@ 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).
+Normally, finalizers are called while the GC (garbage collector) collects
+objects. At that point calling any Node-API that may cause changes in the GC
+state will be disabled and will crash Node.js.
 
-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.
+`node_api_post_finalizer` helps to work around this limitation by allowing the
+add-on to defer calls to such Node-APIs to a point in time outside of the GC
+finalization.
 
 ## Simple asynchronous operations
 

src/js_native_api_v8.cc

@@ -661,13 +661,11 @@ void TrackedFinalizer::FinalizeCore(bool deleteMe) {
   // Either the RefBase is going to be deleted in the finalize_callback or not,
   // it should be removed from the tracked list.
   Unlink();
-  // 1. If the finalize_callback is present, it should either delete the
-  //    derived RefBase, or set ownership with Ownership::kRuntime.
-  // 2. If the finalizer is not present, the derived RefBase can be deleted
-  //    after the call.
+  // If the finalize_callback is present, it should either delete the
+  // derived RefBase, or the RefBase ownership was set to Ownership::kRuntime
+  // and the deleteMe parameter is true.
   if (finalize_callback != nullptr) {
     env_->CallFinalizer(finalize_callback, finalize_data, finalize_hint);
-    // No access to `this` after finalize_callback is called.
   }
 
   if (deleteMe) {
@@ -2026,6 +2024,7 @@ napi_status NAPI_CDECL napi_get_new_target(napi_env env,
   CHECK_ENV(env);
   CHECK_ARG(env, cbinfo);
   CHECK_ARG(env, result);
+  env->CheckGCAccess();
 
   v8impl::CallbackWrapper* info =
       reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);

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

@@ -4,7 +4,6 @@
       "target_name": "test_finalizer",
       "defines": [ "NAPI_EXPERIMENTAL" ],
       "sources": [
-        "../entry_point.c",
         "test_finalizer.c"
       ]
     }

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

@@ -5,6 +5,11 @@ const common = require('../../common');
 const test_finalizer = require(`./build/${common.buildType}/test_finalizer`);
 const assert = require('assert');
 
+// The goal of this test is to show that we can run "pure" finalizers in the
+// current JS loop tick. Thus, we do not use common.gcUntil function works
+// asynchronously using micro tasks.
+// We use IIFE for the obj scope instead of {} to be compatible with
+// non-V8 JS engines that do not support scoped variables.
 (() => {
   const obj = {};
   test_finalizer.addFinalizer(obj);
@@ -19,8 +24,14 @@ for (let i = 0; i < 10; ++i) {
 
 assert.strictEqual(test_finalizer.getFinalizerCallCount(), 1);
 
+// The finalizer that access JS cannot run synchronously. They are run in the
+// next JS loop tick. Thus, we must use common.gcUntil.
 async function runAsyncTests() {
+  // We do not use common.mustCall() because we want to see the finalizer
+  // called in response to GC and not as a part of env destruction.
   let js_is_called = false;
+  // We use IIFE for the obj scope instead of {} to be compatible with
+  // non-V8 JS engines that do not support scoped variables.
   (() => {
     const obj = {};
     test_finalizer.addFinalizerWithJS(obj, () => { js_is_called = true; });

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

@@ -4,6 +4,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include "../common.h"
+#include "../entry_point.h"
 
 typedef struct {
   int32_t finalize_count;
@@ -18,8 +19,9 @@ static void finalizerOnlyCallback(napi_env env,
 
   // It is safe to access instance data
   NODE_API_CALL_RETURN_VOID(env, napi_get_instance_data(env, (void**)&data));
-  NODE_API_ASSERT_RETURN_VOID(
-      env, count = data->finalize_count, "Expected to the same FinalizerData");
+  NODE_API_ASSERT_RETURN_VOID(env,
+                              count = data->finalize_count,
+                              "Expected to be the same FinalizerData");
 }
 
 static void finalizerCallingJSCallback(napi_env env,