Skip to content

Commit 37a207e

Browse files
heimskrheimskr
authored
NAPI fixes (#21775)
### What does this PR do? Defers exceptions thrown by NAPI code until execution returns/flows to JS code. ### How did you verify your code works? Ran existing NAPI tests and added to napi.test.ts.
1 parent 3cf6da9 commit 37a207e

File tree

8 files changed

+386
-43
lines changed

8 files changed

+386
-43
lines changed

‎src/bun.js/bindings/BunProcess.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -618,6 +618,9 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, (JSC::JSGlobalObject * globalOb
618618
env->filename = filename_cstr;
619619

620620
auto encoded = reinterpret_cast<EncodedJSValue>(napi_register_module_v1(env, reinterpret_cast<napi_value>(exportsValue)));
621+
if (env->throwPendingException()) {
622+
return {};
623+
}
621624
RETURN_IF_EXCEPTION(scope, {});
622625
JSC::JSValue resultValue = encoded == 0 ? exports : JSValue::decode(encoded);
623626

‎src/bun.js/bindings/NapiClass.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ JSC_HOST_CALL_ATTRIBUTES JSC::EncodedJSValue NapiClass_ConstructorFunction(JSC::
6767

6868
JSValue ret = toJS(napi->constructor()(napi->env(), frame.toNapi()));
6969
napi_set_last_error(napi->env(), napi_ok);
70+
if (napi->env()->throwPendingException()) {
71+
return {};
72+
}
7073
RETURN_IF_EXCEPTION(scope, {});
7174
if (ret.isEmpty()) {
7275
ret = jsUndefined();

‎src/bun.js/bindings/napi.cpp

Lines changed: 56 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -90,20 +90,7 @@ using namespace Zig;
9090
/* You should not use this throw scope directly -- if you need */ \
9191
/* to throw or clear exceptions, make your own scope */ \
9292
auto napi_preamble_throw_scope__ = DECLARE_THROW_SCOPE(_env->vm()); \
93-
NAPI_RETURN_IF_EXCEPTION(_env)
94-
95-
// Every NAPI function should use this at the start. It does the following:
96-
// - if NAPI_VERBOSE is 1, log that the function was called
97-
// - if env is nullptr, return napi_invalid_arg
98-
// - if there is a pending exception, return napi_pending_exception
99-
// No do..while is used as this declares a variable that other macros need to use
100-
#define NAPI_PREAMBLE(_env) \
101-
NAPI_LOG_CURRENT_FUNCTION; \
102-
NAPI_CHECK_ARG(_env, _env); \
103-
/* You should not use this throw scope directly -- if you need */ \
104-
/* to throw or clear exceptions, make your own scope */ \
105-
auto napi_preamble_throw_scope__ = DECLARE_THROW_SCOPE(_env->vm()); \
106-
NAPI_RETURN_IF_EXCEPTION(_env)
93+
NAPI_RETURN_IF_VM_EXCEPTION(_env)
10794

10895
// Only use this for functions that need their own throw or catch scope. Functions that call into
10996
// JS code that might throw should use NAPI_RETURN_IF_EXCEPTION.
@@ -136,7 +123,17 @@ using namespace Zig;
136123
} while (0)
137124

138125
// Return an error code if an exception was thrown after NAPI_PREAMBLE
139-
#define NAPI_RETURN_IF_EXCEPTION(_env) RETURN_IF_EXCEPTION(napi_preamble_throw_scope__, napi_set_last_error(_env, napi_pending_exception))
126+
#define NAPI_RETURN_IF_VM_EXCEPTION(_env) RETURN_IF_EXCEPTION(napi_preamble_throw_scope__, napi_set_last_error((_env), napi_pending_exception))
127+
128+
#define NAPI_RETURN_IF_EXCEPTION_WITH_SCOPE(_env, _scope) \
129+
do { \
130+
RETURN_IF_EXCEPTION((_scope), napi_set_last_error((_env), napi_pending_exception)); \
131+
if ((_env)->hasPendingException()) { \
132+
return napi_set_last_error((_env), napi_pending_exception); \
133+
} \
134+
} while (0)
135+
136+
#define NAPI_RETURN_IF_EXCEPTION(_env) NAPI_RETURN_IF_EXCEPTION_WITH_SCOPE((_env), napi_preamble_throw_scope__)
140137

141138
// Return indicating that no error occurred in a NAPI function, and an exception is not expected
142139
#define NAPI_RETURN_SUCCESS(_env) \
@@ -918,6 +915,7 @@ extern "C" napi_status napi_create_function(napi_env env, const char* utf8name,
918915
void* data, napi_value* result)
919916
{
920917
NAPI_PREAMBLE(env);
918+
NAPI_RETURN_IF_EXCEPTION(env);
921919
NAPI_CHECK_ARG(env, result);
922920
NAPI_CHECK_ARG(env, cb);
923921

@@ -961,18 +959,17 @@ napi_define_properties(napi_env env, napi_value object, size_t property_count,
961959
const napi_property_descriptor* properties)
962960
{
963961
NAPI_PREAMBLE_NO_THROW_SCOPE(env);
964-
NAPI_CHECK_ARG(env, object);
965-
NAPI_RETURN_EARLY_IF_FALSE(env, properties || property_count == 0, napi_invalid_arg);
966-
967962
Zig::GlobalObject* globalObject = toJS(env);
968963
JSC::VM& vm = JSC::getVM(globalObject);
964+
auto throwScope = DECLARE_THROW_SCOPE(vm);
965+
NAPI_RETURN_IF_EXCEPTION_WITH_SCOPE(env, throwScope);
966+
NAPI_CHECK_ARG(env, object);
967+
NAPI_RETURN_EARLY_IF_FALSE(env, properties || property_count == 0, napi_invalid_arg);
969968

970969
JSValue objectValue = toJS(object);
971970
JSC::JSObject* objectObject = objectValue.getObject();
972971
NAPI_RETURN_EARLY_IF_FALSE(env, objectObject, napi_object_expected);
973972

974-
auto throwScope = DECLARE_THROW_SCOPE(vm);
975-
976973
for (size_t i = 0; i < property_count; i++) {
977974
Napi::defineProperty(env, objectObject, properties[i], true, throwScope);
978975

@@ -1003,7 +1000,6 @@ static napi_status throwErrorWithCStrings(napi_env env, const char* code_utf8, c
10031000
{
10041001
auto* globalObject = toJS(env);
10051002
auto& vm = JSC::getVM(globalObject);
1006-
auto scope = DECLARE_THROW_SCOPE(vm);
10071003

10081004
if (!msg_utf8) {
10091005
return napi_set_last_error(env, napi_invalid_arg);
@@ -1012,8 +1008,8 @@ static napi_status throwErrorWithCStrings(napi_env env, const char* code_utf8, c
10121008
WTF::String code = code_utf8 ? WTF::String::fromUTF8(code_utf8) : WTF::String();
10131009
WTF::String message = WTF::String::fromUTF8(msg_utf8);
10141010

1015-
auto* error = createErrorWithCode(vm, globalObject, code, message, type);
1016-
scope.throwException(globalObject, error);
1011+
JSC::ErrorInstance* error = createErrorWithCode(vm, globalObject, code, message, type);
1012+
env->scheduleException(error);
10171013
return napi_set_last_error(env, napi_ok);
10181014
}
10191015

@@ -1254,9 +1250,13 @@ extern "C" napi_status napi_is_exception_pending(napi_env env, bool* result)
12541250
NAPI_CHECK_ENV_NOT_IN_GC(env);
12551251
NAPI_CHECK_ARG(env, result);
12561252

1257-
auto globalObject = toJS(env);
1258-
auto scope = DECLARE_THROW_SCOPE(JSC::getVM(globalObject));
1259-
*result = scope.exception() != nullptr;
1253+
if (env->hasPendingException()) {
1254+
*result = true;
1255+
} else {
1256+
auto globalObject = toJS(env);
1257+
auto scope = DECLARE_THROW_SCOPE(JSC::getVM(globalObject));
1258+
*result = scope.exception() != nullptr;
1259+
}
12601260
// skip macros as they assume we made a throw scope in the preamble
12611261
return napi_set_last_error(env, napi_ok);
12621262
}
@@ -1275,6 +1275,9 @@ extern "C" napi_status napi_get_and_clear_last_exception(napi_env env,
12751275
auto scope = DECLARE_CATCH_SCOPE(JSC::getVM(globalObject));
12761276
if (scope.exception()) [[unlikely]] {
12771277
*result = toNapi(JSValue(scope.exception()->value()), globalObject);
1278+
} else if (std::optional<JSValue> pending = env->pendingException()) {
1279+
*result = toNapi(pending.value(), globalObject);
1280+
env->clearPendingException();
12781281
} else {
12791282
*result = toNapi(JSC::jsUndefined(), globalObject);
12801283
}
@@ -1300,16 +1303,15 @@ extern "C" napi_status napi_fatal_exception(napi_env env,
13001303

13011304
extern "C" napi_status napi_throw(napi_env env, napi_value error)
13021305
{
1303-
NAPI_PREAMBLE_NO_THROW_SCOPE(env);
1306+
NAPI_PREAMBLE(env);
1307+
NAPI_CHECK_ENV_NOT_IN_GC(env);
1308+
NAPI_RETURN_IF_EXCEPTION(env);
1309+
if (env->isFinishingFinalizers()) {
1310+
return napi_set_last_error(env, env->napiModule().nm_version >= 10 ? napi_cannot_run_js : napi_pending_exception);
1311+
}
13041312
NAPI_CHECK_ARG(env, error);
1305-
auto globalObject = toJS(env);
1306-
JSC::VM& vm = JSC::getVM(globalObject);
1307-
auto throwScope = DECLARE_THROW_SCOPE(vm);
1308-
1309-
JSValue value = toJS(error);
1310-
JSC::throwException(globalObject, throwScope, value);
1311-
1312-
return napi_set_last_error(env, napi_ok);
1313+
env->scheduleException(toJS(error));
1314+
NAPI_RETURN_SUCCESS(env);
13131315
}
13141316

13151317
extern "C" napi_status node_api_symbol_for(napi_env env,
@@ -1486,7 +1488,7 @@ extern "C" JS_EXPORT napi_status node_api_create_buffer_from_arraybuffer(napi_en
14861488
if (!impl || byte_offset + byte_length > impl->byteLength()) [[unlikely]] {
14871489
auto* error = createErrorWithCode(JSC::getVM(globalObject), globalObject, "ERR_OUT_OF_RANGE"_s, "The byte offset + length is out of range"_s, JSC::ErrorType::RangeError);
14881490
RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception));
1489-
scope.throwException(globalObject, error);
1491+
env->scheduleException(error);
14901492
return napi_set_last_error(env, napi_pending_exception);
14911493
}
14921494

@@ -2112,9 +2114,9 @@ napi_status napi_get_value_string_any_encoding(napi_env env, napi_value napiValu
21122114

21132115
Zig::GlobalObject* globalObject = toJS(env);
21142116
JSString* jsString = jsValue.toString(globalObject);
2115-
NAPI_RETURN_IF_EXCEPTION(env);
2117+
NAPI_RETURN_IF_VM_EXCEPTION(env);
21162118
const auto view = jsString->view(globalObject);
2117-
NAPI_RETURN_IF_EXCEPTION(env);
2119+
NAPI_RETURN_IF_VM_EXCEPTION(env);
21182120

21192121
if (buf == nullptr) {
21202122
// they just want to know the length
@@ -2530,7 +2532,7 @@ extern "C" napi_status napi_run_script(napi_env env, napi_value script,
25302532
JSValue value = JSC::evaluate(globalObject, sourceCode, globalObject->globalThis(), returnedException);
25312533

25322534
if (returnedException) {
2533-
throwScope.throwException(globalObject, returnedException);
2535+
env->scheduleException(returnedException.get());
25342536
return napi_set_last_error(env, napi_pending_exception);
25352537
}
25362538

@@ -2707,6 +2709,10 @@ extern "C" napi_status napi_call_function(napi_env env, napi_value recv,
27072709
const napi_value* argv,
27082710
napi_value* result)
27092711
{
2712+
if (env->throwPendingException()) {
2713+
return napi_set_last_error(env, napi_pending_exception);
2714+
}
2715+
27102716
NAPI_PREAMBLE(env);
27112717
NAPI_RETURN_EARLY_IF_FALSE(env, argc == 0 || argv, napi_invalid_arg);
27122718
NAPI_CHECK_ARG(env, func);
@@ -2863,4 +2869,15 @@ extern "C" JSGlobalObject* NapiEnv__globalObject(napi_env env)
28632869
return env->globalObject();
28642870
}
28652871

2872+
extern "C" bool NapiEnv__getAndClearPendingException(napi_env env, JSC::EncodedJSValue* exception)
2873+
{
2874+
if (std::optional<JSC::JSValue> pending = env->pendingException()) {
2875+
*exception = JSValue::encode(*pending);
2876+
env->clearPendingException();
2877+
return true;
2878+
}
2879+
2880+
return false;
2881+
}
2882+
28662883
}

‎src/bun.js/bindings/napi.h

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "napi_macros.h"
1818

1919
#include <list>
20+
#include <optional>
2021
#include <unordered_set>
2122

2223
extern "C" void napi_internal_register_cleanup_zig(napi_env env);
@@ -211,12 +212,51 @@ struct napi_env__ {
211212
napi_internal_enqueue_finalizer(this, finalize_cb, data, finalize_hint);
212213
} else {
213214
finalize_cb(this, data, finalize_hint);
215+
throwPendingException();
214216
}
215217
}
216218

219+
void scheduleException(JSC::JSValue exception)
220+
{
221+
if (exception.isEmpty()) {
222+
m_pendingException.clear();
223+
}
224+
225+
m_pendingException.set(m_vm, exception);
226+
}
227+
228+
bool throwPendingException()
229+
{
230+
if (!m_pendingException) {
231+
return false;
232+
}
233+
234+
auto scope = DECLARE_THROW_SCOPE(m_vm);
235+
JSC::throwException(globalObject(), scope, m_pendingException.get());
236+
m_pendingException.clear();
237+
return true;
238+
}
239+
240+
void clearPendingException()
241+
{
242+
m_pendingException.clear();
243+
}
244+
245+
bool hasPendingException() const
246+
{
247+
return static_cast<bool>(m_pendingException);
248+
}
249+
217250
inline Zig::GlobalObject* globalObject() const { return m_globalObject; }
218251
inline const napi_module& napiModule() const { return m_napiModule; }
219252
inline JSC::VM& vm() const { return m_vm; }
253+
inline std::optional<JSC::JSValue> pendingException() const
254+
{
255+
if (!m_pendingException) {
256+
return std::nullopt;
257+
}
258+
return m_pendingException.get();
259+
}
220260

221261
// Returns true if finalizers from this module need to be scheduled for the next tick after garbage collection, instead of running during garbage collection
222262
inline bool mustDeferFinalizers() const
@@ -309,6 +349,7 @@ struct napi_env__ {
309349
JSC::VM& m_vm;
310350
std::list<std::pair<void (*)(void*), void*>> m_cleanupHooks;
311351
std::list<Napi::AsyncCleanupHook> m_asyncCleanupHooks;
352+
JSC::Strong<JSC::Unknown> m_pendingException;
312353
};
313354

314355
extern "C" void napi_internal_cleanup_env_cpp(napi_env);

‎src/napi/js_native_api_types.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ typedef enum {
9090
napi_date_expected,
9191
napi_arraybuffer_expected,
9292
napi_detachable_arraybuffer_expected,
93-
napi_would_deadlock // unused
93+
napi_would_deadlock, // unused
94+
napi_no_external_buffers_allowed,
95+
napi_cannot_run_js,
9496
} napi_status;
9597
// Note: when adding a new enum value to `napi_status`, please also update
9698
// * `constexpr int last_status` in the definition of `napi_get_last_error_info()'

‎src/napi/napi.zig

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,17 @@ pub const NapiEnv = opaque {
4646
return napi_internal_get_version(self);
4747
}
4848

49+
pub fn getAndClearPendingException(self: *NapiEnv) ?JSValue {
50+
var exception: JSValue = undefined;
51+
if (NapiEnv__getAndClearPendingException(self, &exception)) {
52+
return exception;
53+
}
54+
55+
return null;
56+
}
57+
4958
extern fn NapiEnv__globalObject(*NapiEnv) *jsc.JSGlobalObject;
59+
extern fn NapiEnv__getAndClearPendingException(*NapiEnv, *JSValue) bool;
5060
extern fn napi_internal_get_version(*NapiEnv) u32;
5161
};
5262

@@ -1097,7 +1107,9 @@ pub const napi_async_work = struct {
10971107
this.data,
10981108
);
10991109

1100-
if (global.hasException()) {
1110+
if (env.getAndClearPendingException()) |exception| {
1111+
_ = vm.uncaughtException(global, exception, false);
1112+
} else if (global.hasException()) {
11011113
global.reportActiveExceptionAsUnhandled(error.JSError);
11021114
}
11031115
}
@@ -1348,13 +1360,20 @@ pub const Finalizer = struct {
13481360
const env = this.env.?;
13491361
const handle_scope = NapiHandleScope.open(env, false);
13501362
defer if (handle_scope) |scope| scope.close(env);
1363+
13511364
if (this.fun) |fun| {
13521365
fun(env, this.data, this.hint);
13531366
}
1367+
13541368
napi_internal_remove_finalizer(env, this.fun, this.hint, this.data);
1369+
13551370
if (env.toJS().tryTakeException()) |exception| {
13561371
_ = env.toJS().bunVM().uncaughtException(env.toJS(), exception, false);
13571372
}
1373+
1374+
if (env.getAndClearPendingException()) |exception| {
1375+
_ = env.toJS().bunVM().uncaughtException(env.toJS(), exception, false);
1376+
}
13581377
}
13591378

13601379
/// For Node-API modules not built with NAPI_EXPERIMENTAL, finalizers should be deferred to the

0 commit comments

Comments
 (0)