From 19fa9f1bc47b0666be0747583bea8cb3d8ad5eb1 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <gabrielschulhof@gmail.com>
Date: Sat, 13 May 2023 01:36:13 -0700
Subject: [PATCH] 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.

PR-URL: https://github.com/nodejs/node/pull/47986
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
---
 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_cannot_run_js/binding.gyp            | 32 ++++++++++++
 test/js-native-api/test_cannot_run_js/test.js | 24 +++++++++
 .../test_cannot_run_js/test_cannot_run_js.c   | 49 +++++++++++++++++++
 7 files changed, 131 insertions(+), 8 deletions(-)
 create mode 100644 test/js-native-api/test_cannot_run_js/binding.gyp
 create mode 100644 test/js-native-api/test_cannot_run_js/test.js
 create mode 100644 test/js-native-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 54c53efcef87f9..ec8df553562be7 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 (`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
+  `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 827f3603efc5d8..032a01053f386b 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/js-native-api/test_cannot_run_js/binding.gyp b/test/js-native-api/test_cannot_run_js/binding.gyp
new file mode 100644
index 00000000000000..210417b47b17ec
--- /dev/null
+++ b/test/js-native-api/test_cannot_run_js/binding.gyp
@@ -0,0 +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",
+        "test_cannot_run_js.c"
+      ],
+      "defines": [ "NAPI_EXPERIMENTAL" ],
+      "dependencies": [ "copy_entry_point" ],
+    },
+    {
+      "target_name": "test_pending_exception",
+      "sources": [
+        "entry_point.c",
+        "test_cannot_run_js.c"
+      ],
+      "defines": [ "NAPI_VERSION=8" ],
+      "dependencies": [ "copy_entry_point" ],
+    }
+  ]
+}
diff --git a/test/js-native-api/test_cannot_run_js/test.js b/test/js-native-api/test_cannot_run_js/test.js
new file mode 100644
index 00000000000000..da77d0e0e1cff3
--- /dev/null
+++ b/test/js-native-api/test_cannot_run_js/test.js
@@ -0,0 +1,24 @@
+'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_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`);
+const addon_new = require(`./build/${buildType}/test_cannot_run_js`);
+
+function runTests(addon, isVersion8) {
+  addon.createRef(mustNotCall());
+}
+
+function runAllTests() {
+  runTests(addon_v8, /* isVersion8 */ true);
+  runTests(addon_new, /* isVersion8 */ false);
+}
+
+runAllTests();
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
new file mode 100644
index 00000000000000..6f2a6e71b8dc22
--- /dev/null
+++ b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
@@ -0,0 +1,49 @@
+#include <js_native_api.h>
+#include "../common.h"
+#include "stdlib.h"
+
+static void Finalize(napi_env env, void* data, void* hint) {
+  napi_value global, set_timeout;
+  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_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();
+  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;
+}
+
+EXTERN_C_START
+napi_value Init(napi_env env, napi_value exports) {
+  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;
+}
+EXTERN_C_END