From 0e8930103f3a72436ccb6e6c78209d1cf482573d Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <gabrielschulhof@gmail.com>
Date: Sat, 13 May 2023 01:36:13 -0700
Subject: [PATCH 1/9] node-api: add status napi_cannot_run_js

Add the new status in order to distinguish a state wherein an exception
is pending from one wherein the engine is unable to execute JS. We take
advantage of the new runtime add-on version reporting in order to remain
forward compatible with add-ons that do not expect the new status code.
---
 doc/api/n-api.md                              | 19 ++++++--
 src/js_native_api_types.h                     |  3 +-
 src/js_native_api_v8.cc                       |  3 +-
 src/js_native_api_v8.h                        |  9 ++--
 test/node-api/test_cannot_run_js/binding.gyp  | 14 ++++++
 test/node-api/test_cannot_run_js/test.js      | 23 ++++++++++
 .../test_cannot_run_js/test_cannot_run_js.c   | 46 +++++++++++++++++++
 7 files changed, 109 insertions(+), 8 deletions(-)
 create mode 100644 test/node-api/test_cannot_run_js/binding.gyp
 create mode 100644 test/node-api/test_cannot_run_js/test.js
 create mode 100644 test/node-api/test_cannot_run_js/test_cannot_run_js.c

diff --git a/doc/api/n-api.md b/doc/api/n-api.md
index 77cc49946c4231..e14bf18c5c235d 100644
--- a/doc/api/n-api.md
+++ b/doc/api/n-api.md
@@ -580,7 +580,8 @@ typedef enum {
   napi_arraybuffer_expected,
   napi_detachable_arraybuffer_expected,
   napi_would_deadlock,  /* unused */
-  napi_no_external_buffers_allowed
+  napi_no_external_buffers_allowed,
+  napi_cannot_run_js
 } napi_status;
 ```
 
@@ -814,6 +815,18 @@ typedef void (*napi_finalize)(napi_env env,
 Unless for reasons discussed in [Object Lifetime Management][], creating a
 handle and/or callback scope inside the function body is not necessary.
 
+Since these functions may be called while the JavaScript engine is in a state
+where it cannot execute JavaScript code, some Node-API calls may return
+`napi_pending_exception` even when there is no exception pending.
+
+Change History:
+
+* experimental:
+
+  Node-API calls made from a finalizer will return `napi_cannot_run_js` when
+  the JavaScript engine is unable to execute JavaScript, and will return
+  `napi_exception_pending` if there is a pending exception.
+
 #### `napi_async_execute_callback`
 
 <!-- YAML
@@ -1952,11 +1965,11 @@ from [`napi_add_async_cleanup_hook`][].
 The Node.js environment may be torn down at an arbitrary time as soon as
 possible with JavaScript execution disallowed, like on the request of
 [`worker.terminate()`][]. When the environment is being torn down, the
-registered `napi_finalize` callbacks of JavaScript objects, Thread-safe
+registered `napi_finalize` callbacks of JavaScript objects, thread-safe
 functions and environment instance data are invoked immediately and
 independently.
 
-The invocation of `napi_finalize` callbacks are scheduled after the manually
+The invocation of `napi_finalize` callbacks is scheduled after the manually
 registered cleanup hooks. In order to ensure a proper order of addon
 finalization during environment shutdown to avoid use-after-free in the
 `napi_finalize` callback, addons should register a cleanup hook with
diff --git a/src/js_native_api_types.h b/src/js_native_api_types.h
index 517614950a90f7..005382f173fee9 100644
--- a/src/js_native_api_types.h
+++ b/src/js_native_api_types.h
@@ -99,7 +99,8 @@ typedef enum {
   napi_arraybuffer_expected,
   napi_detachable_arraybuffer_expected,
   napi_would_deadlock,  // unused
-  napi_no_external_buffers_allowed
+  napi_no_external_buffers_allowed,
+  napi_cannot_run_js,
 } napi_status;
 // Note: when adding a new enum value to `napi_status`, please also update
 //   * `const int last_status` in the definition of `napi_get_last_error_info()'
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index 9cbcd2445d1980..3afe992498c059 100644
--- a/src/js_native_api_v8.cc
+++ b/src/js_native_api_v8.cc
@@ -682,6 +682,7 @@ static const char* error_messages[] = {
     "A detachable arraybuffer was expected",
     "Main thread would deadlock",
     "External buffers are not allowed",
+    "Cannot run JavaScript",
 };
 
 napi_status NAPI_CDECL napi_get_last_error_info(
@@ -693,7 +694,7 @@ napi_status NAPI_CDECL napi_get_last_error_info(
   // message in the `napi_status` enum each time a new error message is added.
   // We don't have a napi_status_last as this would result in an ABI
   // change each time a message was added.
-  const int last_status = napi_no_external_buffers_allowed;
+  const int last_status = napi_cannot_run_js;
 
   static_assert(NAPI_ARRAYSIZE(error_messages) == last_status + 1,
                 "Count of error messages must match count of error values");
diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
index 2f4705dce67713..f7646778311bfd 100644
--- a/src/js_native_api_v8.h
+++ b/src/js_native_api_v8.h
@@ -212,9 +212,12 @@ inline napi_status napi_set_last_error(napi_env env,
 #define NAPI_PREAMBLE(env)                                                     \
   CHECK_ENV((env));                                                            \
   RETURN_STATUS_IF_FALSE(                                                      \
-      (env),                                                                   \
-      (env)->last_exception.IsEmpty() && (env)->can_call_into_js(),            \
-      napi_pending_exception);                                                 \
+      (env), (env)->last_exception.IsEmpty(), napi_pending_exception);         \
+  RETURN_STATUS_IF_FALSE((env),                                                \
+                         (env)->can_call_into_js(),                            \
+                         (env->module_api_version == NAPI_VERSION_EXPERIMENTAL \
+                              ? napi_cannot_run_js                             \
+                              : napi_pending_exception));                      \
   napi_clear_last_error((env));                                                \
   v8impl::TryCatch try_catch((env))
 
diff --git a/test/node-api/test_cannot_run_js/binding.gyp b/test/node-api/test_cannot_run_js/binding.gyp
new file mode 100644
index 00000000000000..592b0fae22731e
--- /dev/null
+++ b/test/node-api/test_cannot_run_js/binding.gyp
@@ -0,0 +1,14 @@
+{
+  "targets": [
+    {
+      "target_name": "test_cannot_run_js",
+      "sources": [ "test_cannot_run_js.c" ],
+      "defines": [ "NAPI_EXPERIMENTAL" ],
+    },
+    {
+      "target_name": "test_pending_exception",
+      "sources": [ "test_cannot_run_js.c" ],
+      "defines": [ "NAPI_VERSION=8" ],
+    }
+  ]
+}
diff --git a/test/node-api/test_cannot_run_js/test.js b/test/node-api/test_cannot_run_js/test.js
new file mode 100644
index 00000000000000..aa5b5e1c222063
--- /dev/null
+++ b/test/node-api/test_cannot_run_js/test.js
@@ -0,0 +1,23 @@
+'use strict';
+
+// Test that `napi_call_function()` returns `napi_cannot_run_js` in experimental
+// mode and `napi_pending_exception` otherwise. This test calls the add-on's
+// `createRef()` method, which creates a strong reference to a JS function. When
+// the process exits, it calls all reference finalizers. The finalizer for the
+// strong reference created herein will attempt to call `napi_call_function()`
+// and will abort the process if the API doesn't return the correct status.
+
+const { buildType } = require('../../common');
+const addon_v8 = require(`./build/${buildType}/test_pending_exception`);
+const addon_new = require(`./build/${buildType}/test_cannot_run_js`);
+
+function runTests(addon, isVersion8) {
+  addon.createRef(runTests);
+}
+
+function runAllTests() {
+  runTests(addon_v8, /* isVersion8 */ true);
+  runTests(addon_new, /* isVersion8 */ false);
+}
+
+runAllTests();
diff --git a/test/node-api/test_cannot_run_js/test_cannot_run_js.c b/test/node-api/test_cannot_run_js/test_cannot_run_js.c
new file mode 100644
index 00000000000000..5aac465e9b7150
--- /dev/null
+++ b/test/node-api/test_cannot_run_js/test_cannot_run_js.c
@@ -0,0 +1,46 @@
+#include <node_api.h>
+#include "../../js-native-api/common.h"
+#include "stdlib.h"
+
+static void Finalize(napi_env env, void* data, void* hint) {
+  napi_value cb;
+  napi_ref* ref = data;
+#ifdef NAPI_EXPERIMENTAL
+  napi_status expected_status = napi_cannot_run_js;
+#else
+  napi_status expected_status = napi_pending_exception;
+#endif  // NAPI_EXPERIMENTAL
+
+  if (napi_get_reference_value(env, *ref, &cb) != napi_ok) abort();
+  if (napi_delete_reference(env, *ref) != napi_ok) abort();
+  if (napi_call_function(env, cb, cb, 0, NULL, NULL) != expected_status)
+    abort();
+  free(ref);
+}
+
+static napi_value CreateRef(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value cb;
+  napi_valuetype value_type;
+  napi_ref* ref = malloc(sizeof(*ref));
+  NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, &cb, NULL, NULL));
+  NODE_API_ASSERT(env, argc == 1, "Function takes only one argument");
+  NODE_API_CALL(env, napi_typeof(env, cb, &value_type));
+  NODE_API_ASSERT(
+      env, value_type == napi_function, "argument must be function");
+  NODE_API_CALL(env, napi_add_finalizer(env, cb, ref, Finalize, NULL, ref));
+  return cb;
+}
+
+NAPI_MODULE_INIT() {
+  napi_property_descriptor properties[] = {
+      DECLARE_NODE_API_PROPERTY("createRef", CreateRef),
+  };
+
+  NODE_API_CALL(
+      env,
+      napi_define_properties(
+          env, exports, sizeof(properties) / sizeof(*properties), properties));
+
+  return exports;
+}

From 86277c9a3ebdabd27dda105c476f8b29979095f4 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <gabrielschulhof@gmail.com>
Date: Mon, 15 May 2023 21:56:24 -0700
Subject: [PATCH 2/9] doc: update doc/api/n-api.md

---
 doc/api/n-api.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/api/n-api.md b/doc/api/n-api.md
index e14bf18c5c235d..ee1ad68c4cff84 100644
--- a/doc/api/n-api.md
+++ b/doc/api/n-api.md
@@ -821,7 +821,7 @@ where it cannot execute JavaScript code, some Node-API calls may return
 
 Change History:
 
-* experimental:
+* experimental (`NAPI_EXPERIMENTAL` is defined):
 
   Node-API calls made from a finalizer will return `napi_cannot_run_js` when
   the JavaScript engine is unable to execute JavaScript, and will return

From fa45e0a184d331d7541af100ffcdad8a0dd56435 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <gabrielschulhof@gmail.com>
Date: Thu, 18 May 2023 07:22:05 -0700
Subject: [PATCH 3/9] Update test/node-api/test_cannot_run_js/test.js

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
---
 test/node-api/test_cannot_run_js/test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/node-api/test_cannot_run_js/test.js b/test/node-api/test_cannot_run_js/test.js
index aa5b5e1c222063..2162d4fdd189bf 100644
--- a/test/node-api/test_cannot_run_js/test.js
+++ b/test/node-api/test_cannot_run_js/test.js
@@ -12,7 +12,7 @@ const addon_v8 = require(`./build/${buildType}/test_pending_exception`);
 const addon_new = require(`./build/${buildType}/test_cannot_run_js`);
 
 function runTests(addon, isVersion8) {
-  addon.createRef(runTests);
+  addon.createRef(common.mustNotCall());
 }
 
 function runAllTests() {

From 95137f5c8a774d90a85b17414ac70945062168c8 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <gabrielschulhof@gmail.com>
Date: Thu, 18 May 2023 07:42:51 -0700
Subject: [PATCH 4/9] test: access global property to illicit error status

---
 .../test_cannot_run_js/binding.gyp                |  5 +++++
 .../test_cannot_run_js/test.js                    |  4 ++--
 .../test_cannot_run_js/test_cannot_run_js.c       | 15 ++++++++-------
 3 files changed, 15 insertions(+), 9 deletions(-)
 rename test/{node-api => js-native-api}/test_cannot_run_js/binding.gyp (79%)
 rename test/{node-api => js-native-api}/test_cannot_run_js/test.js (89%)
 rename test/{node-api => js-native-api}/test_cannot_run_js/test_cannot_run_js.c (78%)

diff --git a/test/node-api/test_cannot_run_js/binding.gyp b/test/js-native-api/test_cannot_run_js/binding.gyp
similarity index 79%
rename from test/node-api/test_cannot_run_js/binding.gyp
rename to test/js-native-api/test_cannot_run_js/binding.gyp
index 592b0fae22731e..89a50a520a6946 100644
--- a/test/node-api/test_cannot_run_js/binding.gyp
+++ b/test/js-native-api/test_cannot_run_js/binding.gyp
@@ -1,4 +1,9 @@
 {
+  "target_defaults": {
+    "sources": [
+        "../entry_point.c",
+    ]
+  },
   "targets": [
     {
       "target_name": "test_cannot_run_js",
diff --git a/test/node-api/test_cannot_run_js/test.js b/test/js-native-api/test_cannot_run_js/test.js
similarity index 89%
rename from test/node-api/test_cannot_run_js/test.js
rename to test/js-native-api/test_cannot_run_js/test.js
index 2162d4fdd189bf..2df88933697838 100644
--- a/test/node-api/test_cannot_run_js/test.js
+++ b/test/js-native-api/test_cannot_run_js/test.js
@@ -7,12 +7,12 @@
 // strong reference created herein will attempt to call `napi_call_function()`
 // and will abort the process if the API doesn't return the correct status.
 
-const { buildType } = require('../../common');
+const { buildType, mustNotCall } = require('../../common');
 const addon_v8 = require(`./build/${buildType}/test_pending_exception`);
 const addon_new = require(`./build/${buildType}/test_cannot_run_js`);
 
 function runTests(addon, isVersion8) {
-  addon.createRef(common.mustNotCall());
+  addon.createRef(mustNotCall());
 }
 
 function runAllTests() {
diff --git a/test/node-api/test_cannot_run_js/test_cannot_run_js.c b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
similarity index 78%
rename from test/node-api/test_cannot_run_js/test_cannot_run_js.c
rename to test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
index 5aac465e9b7150..b618d31e336058 100644
--- a/test/node-api/test_cannot_run_js/test_cannot_run_js.c
+++ b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
@@ -1,9 +1,9 @@
-#include <node_api.h>
-#include "../../js-native-api/common.h"
+#include <js_native_api.h>
+#include "../common.h"
 #include "stdlib.h"
 
 static void Finalize(napi_env env, void* data, void* hint) {
-  napi_value cb;
+  napi_value global, set_timeout;
   napi_ref* ref = data;
 #ifdef NAPI_EXPERIMENTAL
   napi_status expected_status = napi_cannot_run_js;
@@ -11,10 +11,9 @@ static void Finalize(napi_env env, void* data, void* hint) {
   napi_status expected_status = napi_pending_exception;
 #endif  // NAPI_EXPERIMENTAL
 
-  if (napi_get_reference_value(env, *ref, &cb) != napi_ok) abort();
   if (napi_delete_reference(env, *ref) != napi_ok) abort();
-  if (napi_call_function(env, cb, cb, 0, NULL, NULL) != expected_status)
-    abort();
+  if (napi_get_global(env, &global) != napi_ok) abort();
+  if (napi_get_named_property(env, global, "setTimeout", &set_timeout) != expected_status) abort();
   free(ref);
 }
 
@@ -32,7 +31,8 @@ static napi_value CreateRef(napi_env env, napi_callback_info info) {
   return cb;
 }
 
-NAPI_MODULE_INIT() {
+EXTERN_C_START
+napi_value Init(napi_env env, napi_value exports) {
   napi_property_descriptor properties[] = {
       DECLARE_NODE_API_PROPERTY("createRef", CreateRef),
   };
@@ -44,3 +44,4 @@ NAPI_MODULE_INIT() {
 
   return exports;
 }
+EXTERN_C_END

From 72980e263e2bb05b5cc695b5e42b8a77269fbc72 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <gabrielschulhof@gmail.com>
Date: Thu, 18 May 2023 07:51:54 -0700
Subject: [PATCH 5/9] heed the linter

---
 test/js-native-api/test_cannot_run_js/test_cannot_run_js.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
index b618d31e336058..6f2a6e71b8dc22 100644
--- a/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
+++ b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
@@ -13,7 +13,9 @@ static void Finalize(napi_env env, void* data, void* hint) {
 
   if (napi_delete_reference(env, *ref) != napi_ok) abort();
   if (napi_get_global(env, &global) != napi_ok) abort();
-  if (napi_get_named_property(env, global, "setTimeout", &set_timeout) != expected_status) abort();
+  if (napi_get_named_property(env, global, "setTimeout", &set_timeout) !=
+      expected_status)
+    abort();
   free(ref);
 }
 

From d6f3232626d9c041a1ae8aebe8801ef0d6fcb468 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <gabrielschulhof@gmail.com>
Date: Fri, 19 May 2023 07:52:21 -0700
Subject: [PATCH 6/9] test: address nit

---
 test/js-native-api/test_cannot_run_js/test.js | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/js-native-api/test_cannot_run_js/test.js b/test/js-native-api/test_cannot_run_js/test.js
index 2df88933697838..da77d0e0e1cff3 100644
--- a/test/js-native-api/test_cannot_run_js/test.js
+++ b/test/js-native-api/test_cannot_run_js/test.js
@@ -4,8 +4,9 @@
 // mode and `napi_pending_exception` otherwise. This test calls the add-on's
 // `createRef()` method, which creates a strong reference to a JS function. When
 // the process exits, it calls all reference finalizers. The finalizer for the
-// strong reference created herein will attempt to call `napi_call_function()`
-// and will abort the process if the API doesn't return the correct status.
+// strong reference created herein will attempt to call `napi_get_property()` on
+// a property of the global object and will abort the process if the API doesn't
+// return the correct status.
 
 const { buildType, mustNotCall } = require('../../common');
 const addon_v8 = require(`./build/${buildType}/test_pending_exception`);

From d8bee9e820e55b6db6efe1d9ce180ef586f0ef03 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <Gschulhof@auction.com>
Date: Fri, 19 May 2023 08:29:09 -0700
Subject: [PATCH 7/9] test: use variables instead of target_defaults

Some platforms do not seem to support target_defaults.
---
 test/js-native-api/test_cannot_run_js/binding.gyp | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/test/js-native-api/test_cannot_run_js/binding.gyp b/test/js-native-api/test_cannot_run_js/binding.gyp
index 89a50a520a6946..3e997f4839a418 100644
--- a/test/js-native-api/test_cannot_run_js/binding.gyp
+++ b/test/js-native-api/test_cannot_run_js/binding.gyp
@@ -1,18 +1,24 @@
 {
-  "target_defaults": {
-    "sources": [
+  "variables": {
+    "common_sources": [
         "../entry_point.c",
     ]
   },
   "targets": [
     {
       "target_name": "test_cannot_run_js",
-      "sources": [ "test_cannot_run_js.c" ],
+      "sources": [
+        '<@(common_sources)',
+        "test_cannot_run_js.c"
+      ],
       "defines": [ "NAPI_EXPERIMENTAL" ],
     },
     {
       "target_name": "test_pending_exception",
-      "sources": [ "test_cannot_run_js.c" ],
+      "sources": [
+        '<@(common_sources)',
+        "test_cannot_run_js.c"
+      ],
       "defines": [ "NAPI_VERSION=8" ],
     }
   ]

From ab54044be3925d013915d391614eb202bb90fdb7 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <Gschulhof@auction.com>
Date: Fri, 19 May 2023 09:02:18 -0700
Subject: [PATCH 8/9] test: explicitly copy dependency

---
 test/js-native-api/test_cannot_run_js/binding.gyp | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/test/js-native-api/test_cannot_run_js/binding.gyp b/test/js-native-api/test_cannot_run_js/binding.gyp
index 3e997f4839a418..3d22a77be0efaf 100644
--- a/test/js-native-api/test_cannot_run_js/binding.gyp
+++ b/test/js-native-api/test_cannot_run_js/binding.gyp
@@ -1,14 +1,9 @@
 {
-  "variables": {
-    "common_sources": [
-        "../entry_point.c",
-    ]
-  },
   "targets": [
     {
       "target_name": "test_cannot_run_js",
       "sources": [
-        '<@(common_sources)',
+        "../entry_point.c",
         "test_cannot_run_js.c"
       ],
       "defines": [ "NAPI_EXPERIMENTAL" ],
@@ -16,7 +11,7 @@
     {
       "target_name": "test_pending_exception",
       "sources": [
-        '<@(common_sources)',
+        "../entry_point.c",
         "test_cannot_run_js.c"
       ],
       "defines": [ "NAPI_VERSION=8" ],

From 128a6f8509397b914d709f856781b2afd6b280f8 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <Gschulhof@auction.com>
Date: Fri, 19 May 2023 18:03:31 -0700
Subject: [PATCH 9/9] test: create job dependency to copy the common file

---
 .../js-native-api/test_cannot_run_js/binding.gyp | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/test/js-native-api/test_cannot_run_js/binding.gyp b/test/js-native-api/test_cannot_run_js/binding.gyp
index 3d22a77be0efaf..210417b47b17ec 100644
--- a/test/js-native-api/test_cannot_run_js/binding.gyp
+++ b/test/js-native-api/test_cannot_run_js/binding.gyp
@@ -1,20 +1,32 @@
 {
   "targets": [
+    {
+      "target_name": "copy_entry_point",
+      "type": "none",
+      "copies": [
+        {
+          "destination": ".",
+          "files": [ "../entry_point.c" ]
+        }
+      ]
+    },
     {
       "target_name": "test_cannot_run_js",
       "sources": [
-        "../entry_point.c",
+        "entry_point.c",
         "test_cannot_run_js.c"
       ],
       "defines": [ "NAPI_EXPERIMENTAL" ],
+      "dependencies": [ "copy_entry_point" ],
     },
     {
       "target_name": "test_pending_exception",
       "sources": [
-        "../entry_point.c",
+        "entry_point.c",
         "test_cannot_run_js.c"
       ],
       "defines": [ "NAPI_VERSION=8" ],
+      "dependencies": [ "copy_entry_point" ],
     }
   ]
 }