napi: don't deliver terminations inside ungated functions

src/jsc/bindings/napi.cpp

@@ -17,6 +17,7 @@
 
 #include "helpers.h"
 #include <JavaScriptCore/FrameTracers.h>
+#include <JavaScriptCore/VMTrapsInlines.h>
 #include <JavaScriptCore/JSObjectInlines.h>
 #include <JavaScriptCore/JSCellInlines.h>
 #include <wtf/text/ExternalStringImpl.h>
@@ -106,15 +107,17 @@ using namespace Zig;
         NAPI_CHECK_ARG(_env, _env);        \
     } while (0)
 
-// Like NAPI_PREAMBLE but for pure value constructors/accessors, which Node lets an addon call while
-// an exception is pending (CHECK_ENV_NOT_IN_GC only) — node-addon-api relies on that to build the
-// Error it wraps a failed call in. Any exception already on the VM (a napi_throw*, or a termination
-// request that materialised in an earlier call while a worker is being stopped) is stashed for the
-// duration and restored on return; the throw scope still catches what the body itself raises.
+// NAPI_PREAMBLE for the value constructors/accessors Node gates with CHECK_ENV only: callable with an
+// exception pending (stashed for the call; only then, since the scope's restore is unconditional), and
+// never where a worker.terminate() / node:vm timeout is delivered (DeferTraps: the next exception check
+// after the call delivers it, as in Node). No JS or addon code may run under it.
 #define NAPI_PREAMBLE_NO_PENDING_CHECK(_env)                                       \
     NAPI_LOG_CURRENT_FUNCTION;                                                     \
     NAPI_CHECK_ARG(_env, _env);                                                    \
-    JSC::SuspendExceptionScope napi_preamble_suspended_exception__ { _env->vm() }; \
+    std::optional<JSC::SuspendExceptionScope> napi_preamble_suspended_exception__; \
+    if (_env->vm().exceptionForInspection()) [[unlikely]]                          \
+        napi_preamble_suspended_exception__.emplace(_env->vm());                   \
+    JSC::DeferTraps napi_preamble_defer_traps__ { _env->vm() };                    \
     auto napi_preamble_throw_scope__ = DECLARE_TOP_EXCEPTION_SCOPE(_env->vm());
 
 // Return an error code if arg is null. Only use for input validation.
@@ -1444,23 +1447,18 @@ extern "C" napi_status napi_create_type_error(napi_env env, napi_value code,
     return createErrorWithNapiValues(env, code, msg, JSC::ErrorType::TypeError, result);
 }
 
-extern "C" JS_EXPORT napi_status
-node_api_create_external_string_latin1(napi_env env,
-    char* str,
-    size_t length,
-    napi_finalize finalize_callback,
-    void* finalize_hint,
-    napi_value* result,
-    bool* copied)
+// node_api_create_external_string_{latin1,utf16}. On `disposeNow` the caller runs the addon's
+// finalizer itself, once this function's preamble scopes have closed.
+template<typename ExternalChar, typename Char>
+static napi_status createExternalString(napi_env env, Char* str, size_t length, napi_finalize finalize_callback, void* finalize_hint, napi_value* result, bool* copied, bool& disposeNow)
 {
-    // https://nodejs.org/api/n-api.html#node_api_create_external_string_latin1
     NAPI_PREAMBLE_NO_PENDING_CHECK(env);
     // Node's CHECK_NEW_STRING_ARGS: str may be null when length is 0.
     NAPI_RETURN_EARLY_IF_FALSE(env, length == 0 || str != nullptr, napi_invalid_arg);
     NAPI_CHECK_ARG(env, result);
     NAPI_RETURN_EARLY_IF_FALSE(env, length == NAPI_AUTO_LENGTH || length <= INT_MAX, napi_invalid_arg);
 
-    length = length == NAPI_AUTO_LENGTH ? strlen(str) : length;
+    length = length == NAPI_AUTO_LENGTH ? std::char_traits<Char>::length(str) : length;
     Zig::GlobalObject* globalObject = toJS(env);
 
     if (copied) {
@@ -1471,14 +1469,12 @@ node_api_create_external_string_latin1(napi_env env,
     // returning the empty string and disposing the caller's buffer immediately.
     if (length == 0) {
         *result = toNapi(JSC::jsEmptyString(JSC::getVM(globalObject)), globalObject);
-        env->doFinalizer(finalize_callback, str, finalize_hint);
-        // Ownership transferred; return ok even if doFinalizer promoted a
-        // pre-existing napi_throw to the VM, so the caller doesn't double-free.
-        return napi_set_last_error(env, napi_ok);
+        disposeNow = true;
+        NAPI_RETURN_SUCCESS(env);
     }
 
-    Ref<WTF::ExternalStringImpl> impl = WTF::ExternalStringImpl::create({ reinterpret_cast<const Latin1Character*>(str), static_cast<unsigned int>(length) }, finalize_hint, [finalize_callback, env](void* hint, void* str, unsigned length) {
-        NAPI_LOG("latin1 string finalizer");
+    Ref<WTF::ExternalStringImpl> impl = WTF::ExternalStringImpl::create({ reinterpret_cast<const ExternalChar*>(str), static_cast<unsigned int>(length) }, finalize_hint, [finalize_callback, env](void* hint, void* str, unsigned) {
+        NAPI_LOG("external string finalizer");
         env->doFinalizer(finalize_callback, str, hint);
     });
 
@@ -1490,6 +1486,26 @@ node_api_create_external_string_latin1(napi_env env,
     NAPI_RETURN_SUCCESS(env);
 }
 
+extern "C" JS_EXPORT napi_status
+node_api_create_external_string_latin1(napi_env env,
+    char* str,
+    size_t length,
+    napi_finalize finalize_callback,
+    void* finalize_hint,
+    napi_value* result,
+    bool* copied)
+{
+    // https://nodejs.org/api/n-api.html#node_api_create_external_string_latin1
+    bool disposeNow = false;
+    napi_status status = createExternalString<Latin1Character>(env, str, length, finalize_callback, finalize_hint, result, copied, disposeNow);
+    if (disposeNow) {
+        env->doFinalizer(finalize_callback, str, finalize_hint);
+        // napi_ok even if the finalizer threw: the buffer is consumed either way.
+        return napi_set_last_error(env, napi_ok);
+    }
+    return status;
+}
+
 extern "C" JS_EXPORT napi_status
 node_api_create_external_string_utf16(napi_env env,
     char16_t* str,
@@ -1500,40 +1516,13 @@ node_api_create_external_string_utf16(napi_env env,
     bool* copied)
 {
     // https://nodejs.org/api/n-api.html#node_api_create_external_string_utf16
-    NAPI_PREAMBLE_NO_PENDING_CHECK(env);
-    // Node's CHECK_NEW_STRING_ARGS: str may be null when length is 0.
-    NAPI_RETURN_EARLY_IF_FALSE(env, length == 0 || str != nullptr, napi_invalid_arg);
-    NAPI_CHECK_ARG(env, result);
-    NAPI_RETURN_EARLY_IF_FALSE(env, length == NAPI_AUTO_LENGTH || length <= INT_MAX, napi_invalid_arg);
-
-    length = length == NAPI_AUTO_LENGTH ? std::char_traits<char16_t>::length(str) : length;
-    Zig::GlobalObject* globalObject = toJS(env);
-
-    if (copied) {
-        *copied = false;
-    }
-
-    // WTF::ExternalStringImpl does not allow zero-length strings; match Node.js/V8 by
-    // returning the empty string and disposing the caller's buffer immediately.
-    if (length == 0) {
-        *result = toNapi(JSC::jsEmptyString(JSC::getVM(globalObject)), globalObject);
+    bool disposeNow = false;
+    napi_status status = createExternalString<char16_t>(env, str, length, finalize_callback, finalize_hint, result, copied, disposeNow);
+    if (disposeNow) {
         env->doFinalizer(finalize_callback, str, finalize_hint);
-        // Ownership transferred; return ok even if doFinalizer promoted a
-        // pre-existing napi_throw to the VM, so the caller doesn't double-free.
         return napi_set_last_error(env, napi_ok);
     }
-
-    Ref<WTF::ExternalStringImpl> impl = WTF::ExternalStringImpl::create({ reinterpret_cast<const char16_t*>(str), static_cast<unsigned int>(length) }, finalize_hint, [finalize_callback, env](void* hint, void* str, unsigned length) {
-        NAPI_LOG("utf16 string finalizer");
-        env->doFinalizer(finalize_callback, str, hint);
-    });
-
-    JSString* out = JSC::jsString(JSC::getVM(globalObject), WTF::String(WTF::move(impl)));
-    ensureStillAliveHere(out);
-    *result = toNapi(out, globalObject);
-    ensureStillAliveHere(out);
-
-    NAPI_RETURN_SUCCESS(env);
+    return status;
 }
 
 extern "C" JS_EXPORT napi_status node_api_create_property_key_latin1(napi_env env, const char* str, size_t length, napi_value* result)
@@ -2891,9 +2880,8 @@ extern "C" napi_status napi_get_value_bigint_int64(napi_env env, napi_value valu
     JSValue jsValue = toJS(value);
     NAPI_RETURN_EARLY_IF_FALSE(env, jsValue.isHeapBigInt(), napi_bigint_expected);
 
-    // toBigInt64 can throw if the value is not a bigint. we have already checked, so we shouldn't
-    // hit an exception here and it's okay to assert at the end
     *result = jsValue.toBigInt64(toJS(env));
+    NAPI_RETURN_IF_VM_EXCEPTION(env);
 
     JSBigInt* bigint = jsValue.asHeapBigInt();
     auto length = bigint->length();
@@ -2925,8 +2913,6 @@ extern "C" napi_status napi_get_value_bigint_uint64(napi_env env, napi_value val
     JSValue jsValue = toJS(value);
     NAPI_RETURN_EARLY_IF_FALSE(env, jsValue.isHeapBigInt(), napi_bigint_expected);
 
-    // toBigInt64 can throw if the value is not a bigint. we have already checked, so we shouldn't
-    // hit an exception here and it's okay to assert at the end
     *result = jsValue.toBigUInt64(toJS(env));
     NAPI_RETURN_IF_VM_EXCEPTION(env);
 

test/napi/napi-app/module.js

@@ -1452,4 +1452,46 @@ nativeTests.test_threadsafe_function_microtask_order = async () => {
   }
 };
 
+// A script that only ever calls the ungated napi functions (ungated-calls-
+// spin-worker.js has the worker version) still has to be stoppable when the
+// stop is requested while one of those calls is running. Several rounds, since
+// whether it lands inside a call or in the loop itself is down to timing.
+nativeTests.test_ungated_calls_vm_timeout = () => {
+  const vm = require("node:vm");
+  const spin = nativeTests.make_ungated_calls_spinner();
+  for (let i = 0; i < 5; i++) {
+    try {
+      vm.runInNewContext("for (;;) spin(bigint, string);", { spin, bigint: -7n, string: "ungated" }, { timeout: 20 });
+      console.log("returned");
+    } catch (e) {
+      console.log(e.code);
+    }
+  }
+};
+
+nativeTests.test_ungated_calls_worker_terminate = async () => {
+  const { Worker } = require("node:worker_threads");
+  const path = require("node:path");
+  for (let i = 0; i < 2; i++) {
+    const worker = new Worker(path.join(__dirname, "ungated-calls-spin-worker.js"));
+    await new Promise((resolve, reject) => {
+      worker.once("message", resolve);
+      worker.once("error", reject);
+    });
+    console.log("terminate() resolved with", await worker.terminate());
+  }
+};
+
+// See ungated_calls_through_timeout in standalone_tests.cpp: 200ms of ungated
+// calls under a 20ms timeout.
+nativeTests.test_ungated_calls_through_vm_timeout = () => {
+  const vm = require("node:vm");
+  try {
+    vm.runInNewContext("f(200)", { f: nativeTests.ungated_calls_through_timeout }, { timeout: 20 });
+    console.log("returned");
+  } catch (e) {
+    console.log(e.code);
+  }
+};
+
 module.exports = nativeTests;