From 7daf62ac3d01bb831e62c6c8dcc6092c7df7b19a Mon Sep 17 00:00:00 2001 From: Ben Grant Date: Tue, 8 Oct 2024 18:45:51 -0700 Subject: [PATCH 1/4] Test napi_throw_error thoroughly --- src/bun.js/bindings/napi.cpp | 2 +- test/napi/napi-app/main.cpp | 31 ++++++++++++++++++++++++++++ test/napi/napi-app/main.js | 2 +- test/napi/napi.test.ts | 39 ++++++++++++++++++++++++++++++++++-- 4 files changed, 70 insertions(+), 4 deletions(-) diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index 3a70970619c27..c9351cf8480aa 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -1306,7 +1306,7 @@ static void throwErrorWithCode(JSC::JSGlobalObject* globalObject, const char* ms auto scope = DECLARE_THROW_SCOPE(vm); auto message = msg_utf8 ? WTF::String::fromUTF8(msg_utf8) : String(); - auto code = msg_utf8 ? WTF::String::fromUTF8(code_utf8) : String(); + auto code = code_utf8 ? WTF::String::fromUTF8(code_utf8) : String(); auto* error = createError(globalObject, message); if (!code.isEmpty()) { diff --git a/test/napi/napi-app/main.cpp b/test/napi/napi-app/main.cpp index e07d8b773d1f0..b5d153b3200a8 100644 --- a/test/napi/napi-app/main.cpp +++ b/test/napi/napi-app/main.cpp @@ -639,6 +639,36 @@ napi_value call_and_get_exception(const Napi::CallbackInfo &info) { return exception; } +// takes an integer "mode" parameter which should be bitwise OR of one constant +// for message and one for code +// clang-format off +static constexpr uint32_t THROW_CODE_NULLPTR = 1 << 0; +static constexpr uint32_t THROW_CODE_EMPTY_STRING = 1 << 1; +static constexpr uint32_t THROW_CODE_NORMAL = 1 << 2; +static constexpr uint32_t THROW_MESSAGE_NULLPTR = 1 << 3; +static constexpr uint32_t THROW_MESSAGE_EMPTY_STRING = 1 << 4; +static constexpr uint32_t THROW_MESSAGE_NORMAL = 1 << 5; +// clang-format on + +napi_value throw_weird_error(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + // info[0] is GC callback + napi_value napi_mode = info[1]; + uint32_t mode; + assert(napi_get_value_uint32(env, napi_mode, &mode) == napi_ok); + + const char *code = mode & THROW_CODE_NULLPTR + ? nullptr + : (mode & THROW_CODE_EMPTY_STRING ? "" : "code"); + const char *message = + mode & THROW_MESSAGE_NULLPTR + ? nullptr + : (mode & THROW_MESSAGE_EMPTY_STRING ? "" : "message"); + + napi_throw_error(env, code, message); + return nullptr; +} + napi_value eval_wrapper(const Napi::CallbackInfo &info) { napi_value ret = nullptr; // info[0] is the GC callback @@ -740,6 +770,7 @@ Napi::Object InitAll(Napi::Env env, Napi::Object exports1) { Napi::Function::New(env, call_and_get_exception)); exports.Set("eval_wrapper", Napi::Function::New(env, eval_wrapper)); exports.Set("perform_get", Napi::Function::New(env, perform_get)); + exports.Set("throw_weird_error", Napi::Function::New(env, throw_weird_error)); return exports; } diff --git a/test/napi/napi-app/main.js b/test/napi/napi-app/main.js index bf7d66d9a4c11..d37ba09171f01 100644 --- a/test/napi/napi-app/main.js +++ b/test/napi/napi-app/main.js @@ -47,5 +47,5 @@ try { throw new Error(result); } } catch (e) { - console.log("synchronously threw:", e.name); + console.log(`synchronously threw ${e.name}: message ${JSON.stringify(e.message)}, code ${JSON.stringify(e.code)}`); } diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index b9e0e23da2671..ac2122ea62267 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -271,8 +271,14 @@ describe("napi", () => { checkSameOutput("eval_wrapper", ["(()=>{ throw new TypeError('oops'); })()"]); }); it("cannot see locals from around its invocation", () => { - // variable is declared on main.js:18, but it should not be in scope for the eval'd code - checkSameOutput("eval_wrapper", ["shouldNotExist"]); + // variable should_not_exist is declared on main.js:18, but it should not be in scope for the eval'd code + // this doesn't use checkSameOutput because V8 and JSC use different error messages for a missing variable + let bunResult = runOn(bunExe(), "eval_wrapper", ["shouldNotExist"]); + // remove all debug logs + bunResult = bunResult.replaceAll(/^\[\w+\].+$/gm, "").trim(); + expect(bunResult).toBe( + `synchronously threw ReferenceError: message "Can't find variable: shouldNotExist", code undefined`, + ); }); }); @@ -281,6 +287,35 @@ describe("napi", () => { checkSameOutput("test_get_property", []); }); }); + + describe("napi_throw_error", () => { + const THROW_CODE_NULLPTR = 1 << 0; + const THROW_CODE_EMPTY_STRING = 1 << 1; + const THROW_CODE_NORMAL = 1 << 2; + const THROW_MESSAGE_NULLPTR = 1 << 3; + const THROW_MESSAGE_EMPTY_STRING = 1 << 4; + const THROW_MESSAGE_NORMAL = 1 << 5; + + const errorCodeModes = { + nullptr: 1 << 0, + emptyString: 1 << 1, + normal: 1 << 2, + }; + const errorMessageModes = { + nullptr: 1 << 3, + emptyString: 1 << 4, + normal: 1 << 5, + }; + const modes = Object.keys(errorCodeModes) as Array; + + describe.each(modes)("when code is %s", codeMode => { + describe.each(modes)("when message is %s", messageMode => { + it("has the right code and message", () => { + checkSameOutput("throw_weird_error", [errorCodeModes[codeMode] | errorMessageModes[messageMode]]); + }); + }); + }); + }); }); function checkSameOutput(test: string, args: any[] | string) { From afdccc2e3ca07728ac32583fa8763696cabe1a94 Mon Sep 17 00:00:00 2001 From: Ben Grant Date: Wed, 9 Oct 2024 14:27:34 -0700 Subject: [PATCH 2/4] Fix and test napi_throw_*error functions --- src/bun.js/bindings/napi.cpp | 45 +++++-------- test/napi/napi-app/main.cpp | 127 +++++++++++++++++++++++++---------- test/napi/napi-app/module.js | 25 +++++++ test/napi/napi.test.ts | 34 +++------- 4 files changed, 138 insertions(+), 93 deletions(-) diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index c9351cf8480aa..5e2e23f0a776f 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -1300,20 +1300,26 @@ napi_define_properties(napi_env env, napi_value object, size_t property_count, return napi_ok; } -static void throwErrorWithCode(JSC::JSGlobalObject* globalObject, const char* msg_utf8, const char* code_utf8, const WTF::Function& createError) +// used to implement napi_throw_*_error +static napi_status throwErrorWithCStrings(JSC::JSGlobalObject* globalObject, const char* code_utf8, const char* msg_utf8, JSC::ErrorType type) { + if (msg_utf8 == nullptr) { + return napi_invalid_arg; + } + auto& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - auto message = msg_utf8 ? WTF::String::fromUTF8(msg_utf8) : String(); + auto message = WTF::String::fromUTF8(msg_utf8); auto code = code_utf8 ? WTF::String::fromUTF8(code_utf8) : String(); - auto* error = createError(globalObject, message); - if (!code.isEmpty()) { + auto* error = JSC::ErrorInstance::create(globalObject->vm(), globalObject->errorStructure(type), message, JSValue(), nullptr, RuntimeType::TypeNothing, type); + if (!code.isNull()) { error->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), JSC::jsString(vm, code), 0); } scope.throwException(globalObject, Exception::create(vm, error)); + return napi_ok; } static JSValue createErrorForNapi(napi_env env, napi_value code, napi_value msg, const WTF::Function& constructor) @@ -1354,12 +1360,7 @@ extern "C" napi_status napi_throw_error(napi_env env, { NAPI_PREMABLE Zig::GlobalObject* globalObject = toJS(env); - - throwErrorWithCode(globalObject, msg, code, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) { - return JSC::createError(globalObject, message); - }); - - return napi_ok; + return throwErrorWithCStrings(globalObject, code, msg, JSC::ErrorType::Error); } extern "C" napi_status napi_create_reference(napi_env env, napi_value value, @@ -1671,14 +1672,8 @@ extern "C" napi_status node_api_throw_syntax_error(napi_env env, const char* msg) { NAPI_PREMABLE - - auto globalObject = toJS(env); - - throwErrorWithCode(globalObject, msg, code, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) { - return JSC::createSyntaxError(globalObject, message); - }); - - return napi_ok; + Zig::GlobalObject* globalObject = toJS(env); + return throwErrorWithCStrings(globalObject, code, msg, JSC::ErrorType::SyntaxError); } extern "C" napi_status napi_throw_type_error(napi_env env, const char* code, @@ -1686,12 +1681,7 @@ extern "C" napi_status napi_throw_type_error(napi_env env, const char* code, { NAPI_PREMABLE Zig::GlobalObject* globalObject = toJS(env); - - throwErrorWithCode(globalObject, msg, code, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) { - return JSC::createTypeError(globalObject, message); - }); - - return napi_ok; + return throwErrorWithCStrings(globalObject, code, msg, JSC::ErrorType::TypeError); } extern "C" napi_status napi_create_type_error(napi_env env, napi_value code, @@ -1748,12 +1738,7 @@ extern "C" napi_status napi_throw_range_error(napi_env env, const char* code, { NAPI_PREMABLE Zig::GlobalObject* globalObject = toJS(env); - - throwErrorWithCode(globalObject, msg, code, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) { - return JSC::createRangeError(globalObject, message); - }); - - return napi_ok; + return throwErrorWithCStrings(globalObject, code, msg, JSC::ErrorType::RangeError); } extern "C" napi_status napi_object_freeze(napi_env env, napi_value object_value) diff --git a/test/napi/napi-app/main.cpp b/test/napi/napi-app/main.cpp index b5d153b3200a8..927f782454799 100644 --- a/test/napi/napi-app/main.cpp +++ b/test/napi/napi-app/main.cpp @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include napi_value fail(napi_env env, const char *msg) { @@ -35,6 +37,13 @@ static void run_gc(const Napi::CallbackInfo &info) { info[0].As().Call(0, nullptr); } +// calls napi_typeof and asserts it returns napi_ok +static napi_valuetype get_typeof(napi_env env, napi_value value) { + napi_valuetype result; + assert(napi_typeof(env, value, &result) == napi_ok); + return result; +} + napi_value test_issue_7685(const Napi::CallbackInfo &info) { Napi::Env env(info.Env()); Napi::HandleScope scope(env); @@ -229,8 +238,7 @@ napi_value test_napi_delete_property(const Napi::CallbackInfo &info) { // info[0] is a function to run the GC napi_value object = info[1]; - napi_valuetype type; - assert(napi_typeof(env, object, &type) == napi_ok); + napi_valuetype type = get_typeof(env, object); assert(type == napi_object); napi_value key; @@ -540,8 +548,7 @@ napi_value test_napi_ref(const Napi::CallbackInfo &info) { napi_value from_ref; assert(napi_get_reference_value(env, ref, &from_ref) == napi_ok); assert(from_ref != nullptr); - napi_valuetype typeof_result; - assert(napi_typeof(env, from_ref, &typeof_result) == napi_ok); + napi_valuetype typeof_result = get_typeof(env, from_ref); assert(typeof_result == napi_object); return ok(env); } @@ -629,8 +636,7 @@ napi_value call_and_get_exception(const Napi::CallbackInfo &info) { napi_value exception; assert(napi_get_and_clear_last_exception(env, &exception) == napi_ok); - napi_valuetype type; - assert(napi_typeof(env, exception, &type) == napi_ok); + napi_valuetype type = get_typeof(env, exception); printf("typeof thrown exception = %s\n", napi_valuetype_to_string(type)); assert(napi_is_exception_pending(env, &is_pending) == napi_ok); @@ -639,34 +645,82 @@ napi_value call_and_get_exception(const Napi::CallbackInfo &info) { return exception; } -// takes an integer "mode" parameter which should be bitwise OR of one constant -// for message and one for code -// clang-format off -static constexpr uint32_t THROW_CODE_NULLPTR = 1 << 0; -static constexpr uint32_t THROW_CODE_EMPTY_STRING = 1 << 1; -static constexpr uint32_t THROW_CODE_NORMAL = 1 << 2; -static constexpr uint32_t THROW_MESSAGE_NULLPTR = 1 << 3; -static constexpr uint32_t THROW_MESSAGE_EMPTY_STRING = 1 << 4; -static constexpr uint32_t THROW_MESSAGE_NORMAL = 1 << 5; -// clang-format on +// throw_error(code: string|undefined, msg: string|undefined, +// kind: 'error'|'type_error'|'range_error'|'syntax_error') +napi_value throw_error(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + + napi_value js_code = info[0]; + napi_value js_msg = info[1]; + napi_value js_error_kind = info[2]; + const char *code = nullptr; + const char *msg = nullptr; + char code_buf[256] = {0}, msg_buf[256] = {0}, error_kind_buf[256] = {0}; + + if (get_typeof(env, js_code) == napi_string) { + assert(napi_get_value_string_utf8(env, js_code, code_buf, sizeof code_buf, + nullptr) == napi_ok); + code = code_buf; + } + if (get_typeof(env, js_msg) == napi_string) { + assert(napi_get_value_string_utf8(env, js_msg, msg_buf, sizeof msg_buf, + nullptr) == napi_ok); + msg = msg_buf; + } + assert(napi_get_value_string_utf8(env, js_error_kind, error_kind_buf, + sizeof error_kind_buf, nullptr) == napi_ok); + + std::map + functions{{"error", napi_throw_error}, + {"type_error", napi_throw_type_error}, + {"range_error", napi_throw_range_error}, + {"syntax_error", node_api_throw_syntax_error}}; + + auto throw_function = functions[error_kind_buf]; + + if (msg == nullptr) { + assert(throw_function(env, code, msg) == napi_invalid_arg); + return ok(env); + } else { + assert(throw_function(env, code, msg) == napi_ok); + return nullptr; + } +} -napi_value throw_weird_error(const Napi::CallbackInfo &info) { +// create_and_throw_error(code: string|undefined, msg: string|undefined, +// kind: 'error'|'type_error'|'range_error'|'syntax_error') +napi_value create_and_throw_error(const Napi::CallbackInfo &info) { napi_env env = info.Env(); - // info[0] is GC callback - napi_value napi_mode = info[1]; - uint32_t mode; - assert(napi_get_value_uint32(env, napi_mode, &mode) == napi_ok); - const char *code = mode & THROW_CODE_NULLPTR - ? nullptr - : (mode & THROW_CODE_EMPTY_STRING ? "" : "code"); - const char *message = - mode & THROW_MESSAGE_NULLPTR - ? nullptr - : (mode & THROW_MESSAGE_EMPTY_STRING ? "" : "message"); + napi_value js_code = info[0]; + napi_value js_msg = info[1]; + napi_value js_error_kind = info[2]; + char error_kind_buf[256] = {0}; + + assert(napi_get_value_string_utf8(env, js_error_kind, error_kind_buf, + sizeof error_kind_buf, nullptr) == napi_ok); - napi_throw_error(env, code, message); - return nullptr; + std::map + functions{{"error", napi_create_error}, + {"type_error", napi_create_type_error}, + {"range_error", napi_create_range_error}, + {"syntax_error", node_api_create_syntax_error}}; + + auto create_error_function = functions[error_kind_buf]; + + napi_value err; + napi_status create_status = create_error_function(env, js_code, js_msg, &err); + if (get_typeof(env, js_msg) != napi_string || + get_typeof(env, js_code) != napi_string) { + assert(create_status == napi_string_expected); + return ok(env); + } else { + assert(create_status == napi_ok); + assert(napi_throw(env, err) == napi_ok); + return nullptr; + } } napi_value eval_wrapper(const Napi::CallbackInfo &info) { @@ -685,8 +739,7 @@ napi_value perform_get(const Napi::CallbackInfo &info) { napi_value value; // if key is a string, try napi_get_named_property - napi_valuetype type; - assert(napi_typeof(env, key, &type) == napi_ok); + napi_valuetype type = get_typeof(env, key); if (type == napi_string) { char buf[1024]; assert(napi_get_value_string_utf8(env, key, buf, 1024, nullptr) == napi_ok); @@ -696,8 +749,7 @@ napi_value perform_get(const Napi::CallbackInfo &info) { status == napi_pending_exception || status == napi_generic_failure); if (status == napi_ok) { assert(value != nullptr); - assert(napi_typeof(env, value, &type) == napi_ok); - printf("value type = %d\n", type); + printf("value type = %d\n", get_typeof(env, value)); } else { return ok(env); } @@ -708,8 +760,7 @@ napi_value perform_get(const Napi::CallbackInfo &info) { status == napi_pending_exception || status == napi_generic_failure); if (status == napi_ok) { assert(value != nullptr); - assert(napi_typeof(env, value, &type) == napi_ok); - printf("value type = %d\n", type); + printf("value type = %d\n", get_typeof(env, value)); return value; } else { return ok(env); @@ -770,7 +821,9 @@ Napi::Object InitAll(Napi::Env env, Napi::Object exports1) { Napi::Function::New(env, call_and_get_exception)); exports.Set("eval_wrapper", Napi::Function::New(env, eval_wrapper)); exports.Set("perform_get", Napi::Function::New(env, perform_get)); - exports.Set("throw_weird_error", Napi::Function::New(env, throw_weird_error)); + exports.Set("throw_error", Napi::Function::New(env, throw_error)); + exports.Set("create_and_throw_error", + Napi::Function::New(env, create_and_throw_error)); return exports; } diff --git a/test/napi/napi-app/module.js b/test/napi/napi-app/module.js index 79903ed5c7d0c..48f8a4e87c59c 100644 --- a/test/napi/napi-app/module.js +++ b/test/napi/napi-app/module.js @@ -91,4 +91,29 @@ nativeTests.test_get_property = () => { } }; +function loopErrorParams(callback) { + for (const errorKind of ["error", "type_error", "range_error", "syntax_error"]) { + for (const code of [undefined, "", "error code"]) { + for (const msg of [undefined, "", "error message"]) { + try { + callback(code, msg, errorKind); + console.log(`${callback.name}(${code ?? "nullptr"}, ${msg ?? "nullptr"}) did not throw`); + } catch (e) { + console.log( + `${callback.name} threw ${e.name}: message ${JSON.stringify(e.message)}, code ${JSON.stringify(e.code)}`, + ); + } + } + } + } +} + +nativeTests.test_throw_functions_exhaustive = () => { + loopErrorParams(nativeTests.throw_error); +}; + +nativeTests.test_create_error_functions_exhaustive = () => { + loopErrorParams(nativeTests.create_and_throw_error); +}; + module.exports = nativeTests; diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index ac2122ea62267..bbe2836ea632a 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -288,32 +288,14 @@ describe("napi", () => { }); }); - describe("napi_throw_error", () => { - const THROW_CODE_NULLPTR = 1 << 0; - const THROW_CODE_EMPTY_STRING = 1 << 1; - const THROW_CODE_NORMAL = 1 << 2; - const THROW_MESSAGE_NULLPTR = 1 << 3; - const THROW_MESSAGE_EMPTY_STRING = 1 << 4; - const THROW_MESSAGE_NORMAL = 1 << 5; - - const errorCodeModes = { - nullptr: 1 << 0, - emptyString: 1 << 1, - normal: 1 << 2, - }; - const errorMessageModes = { - nullptr: 1 << 3, - emptyString: 1 << 4, - normal: 1 << 5, - }; - const modes = Object.keys(errorCodeModes) as Array; - - describe.each(modes)("when code is %s", codeMode => { - describe.each(modes)("when message is %s", messageMode => { - it("has the right code and message", () => { - checkSameOutput("throw_weird_error", [errorCodeModes[codeMode] | errorMessageModes[messageMode]]); - }); - }); + describe("napi_throw functions", () => { + it("has the right code and message", () => { + checkSameOutput("test_throw_functions_exhaustive", []); + }); + }); + describe("napi_create_error functions", () => { + it("has the right code and message", () => { + checkSameOutput("test_create_error_functions_exhaustive", []); }); }); }); From e4edc5022e082abcd77e96c8b97e0764abfa0ad1 Mon Sep 17 00:00:00 2001 From: Ben Grant Date: Wed, 9 Oct 2024 16:59:38 -0700 Subject: [PATCH 3/4] Fix napi_create_*_error functions --- src/bun.js/bindings/napi.cpp | 157 +++++++++++------------------------ test/napi/napi-app/main.cpp | 14 +++- test/napi/napi-app/module.js | 30 +++++-- 3 files changed, 80 insertions(+), 121 deletions(-) diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index 5e2e23f0a776f..8f60db62928d1 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -1300,58 +1300,65 @@ napi_define_properties(napi_env env, napi_value object, size_t property_count, return napi_ok; } -// used to implement napi_throw_*_error -static napi_status throwErrorWithCStrings(JSC::JSGlobalObject* globalObject, const char* code_utf8, const char* msg_utf8, JSC::ErrorType type) +static JSC::ErrorInstance* createErrorWithCode(JSC::JSGlobalObject* globalObject, const WTF::String& code, const WTF::String& message, JSC::ErrorType type) { - if (msg_utf8 == nullptr) { - return napi_invalid_arg; - } + // no napi functions permit a null message, they must check before calling this function and + // return the right error code + ASSERT(!message.isNull()); auto& vm = globalObject->vm(); - auto scope = DECLARE_THROW_SCOPE(vm); - - auto message = WTF::String::fromUTF8(msg_utf8); - auto code = code_utf8 ? WTF::String::fromUTF8(code_utf8) : String(); + // we don't call JSC::createError() as it asserts the message is not an empty string "" auto* error = JSC::ErrorInstance::create(globalObject->vm(), globalObject->errorStructure(type), message, JSValue(), nullptr, RuntimeType::TypeNothing, type); if (!code.isNull()) { error->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), JSC::jsString(vm, code), 0); } - scope.throwException(globalObject, Exception::create(vm, error)); - return napi_ok; + return error; } -static JSValue createErrorForNapi(napi_env env, napi_value code, napi_value msg, const WTF::Function& constructor) +// used to implement napi_throw_*_error +static napi_status throwErrorWithCStrings(napi_env env, const char* code_utf8, const char* msg_utf8, JSC::ErrorType type) { auto* globalObject = toJS(env); - JSC::VM& vm = globalObject->vm(); - auto catchScope = DECLARE_CATCH_SCOPE(vm); - - JSValue codeValue = toJS(code); - WTF::String message; + auto& vm = globalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); - if (msg) { - JSValue messageValue = toJS(msg); - message = messageValue.toWTFString(globalObject); - if (catchScope.exception()) { - catchScope.clearException(); - return {}; - } + if (!msg_utf8) { + return napi_invalid_arg; } - auto* error = constructor(globalObject, message); + WTF::String code = code_utf8 ? WTF::String::fromUTF8(code_utf8) : WTF::String(); + WTF::String message = WTF::String::fromUTF8(msg_utf8); - if (codeValue && error) { - error->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), codeValue, 0); - } + auto* error = createErrorWithCode(globalObject, code, message, type); + scope.throwException(globalObject, error); + return napi_ok; +} - if (catchScope.exception()) { - catchScope.clearException(); - return {}; +// code must be a string or nullptr (no code) +// msg must be a string +// never calls toString, never throws +static napi_status createErrorWithNapiValues(napi_env env, napi_value code, napi_value message, JSC::ErrorType type, napi_value* result) +{ + if (!result || !message) { + return napi_invalid_arg; + } + JSValue js_code = toJS(code); + JSValue js_message = toJS(message); + if (!js_message.isString() || !(js_code.isEmpty() || js_code.isString())) { + return napi_string_expected; } - return error; + auto* globalObject = toJS(env); + + auto wtf_code = js_code.isEmpty() ? WTF::String() : js_code.getString(globalObject); + auto wtf_message = js_message.getString(globalObject); + + *result = toNapi( + createErrorWithCode(globalObject, wtf_code, wtf_message, type), + globalObject); + return napi_ok; } extern "C" napi_status napi_throw_error(napi_env env, @@ -1359,8 +1366,7 @@ extern "C" napi_status napi_throw_error(napi_env env, const char* msg) { NAPI_PREMABLE - Zig::GlobalObject* globalObject = toJS(env); - return throwErrorWithCStrings(globalObject, code, msg, JSC::ErrorType::Error); + return throwErrorWithCStrings(env, code, msg, JSC::ErrorType::Error); } extern "C" napi_status napi_create_reference(napi_env env, napi_value value, @@ -1651,20 +1657,7 @@ extern "C" napi_status node_api_create_syntax_error(napi_env env, napi_value* result) { NAPI_PREMABLE - if (UNLIKELY(!result)) { - return napi_invalid_arg; - } - - auto err = createErrorForNapi(env, code, msg, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) { - return JSC::createSyntaxError(globalObject, message); - }); - - if (UNLIKELY(!err)) { - return napi_generic_failure; - } - - *result = toNapi(err, toJS(env)); - return napi_ok; + return createErrorWithNapiValues(env, code, msg, JSC::ErrorType::SyntaxError, result); } extern "C" napi_status node_api_throw_syntax_error(napi_env env, @@ -1672,40 +1665,22 @@ extern "C" napi_status node_api_throw_syntax_error(napi_env env, const char* msg) { NAPI_PREMABLE - Zig::GlobalObject* globalObject = toJS(env); - return throwErrorWithCStrings(globalObject, code, msg, JSC::ErrorType::SyntaxError); + return throwErrorWithCStrings(env, code, msg, JSC::ErrorType::SyntaxError); } extern "C" napi_status napi_throw_type_error(napi_env env, const char* code, const char* msg) { NAPI_PREMABLE - Zig::GlobalObject* globalObject = toJS(env); - return throwErrorWithCStrings(globalObject, code, msg, JSC::ErrorType::TypeError); + return throwErrorWithCStrings(env, code, msg, JSC::ErrorType::TypeError); } extern "C" napi_status napi_create_type_error(napi_env env, napi_value code, napi_value msg, napi_value* result) { - if (UNLIKELY(!result || !env)) { - return napi_invalid_arg; - } - - auto err = createErrorForNapi(env, code, msg, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) { - if (message.isEmpty()) { - return JSC::createTypeError(globalObject); - } - - return JSC::createTypeError(globalObject, message); - }); - - if (UNLIKELY(!err)) { - return napi_generic_failure; - } - - *result = toNapi(err, toJS(env)); - return napi_ok; + NAPI_PREMABLE + return createErrorWithNapiValues(env, code, msg, JSC::ErrorType::TypeError, result); } extern "C" napi_status napi_create_error(napi_env env, napi_value code, @@ -1713,32 +1688,13 @@ extern "C" napi_status napi_create_error(napi_env env, napi_value code, napi_value* result) { NAPI_PREMABLE - - if (UNLIKELY(!result)) { - return napi_invalid_arg; - } - - auto err = createErrorForNapi(env, code, msg, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) { - if (message.isEmpty()) { - return JSC::createError(globalObject, String("Error"_s)); - } - - return JSC::createError(globalObject, message); - }); - - if (UNLIKELY(!err)) { - return napi_generic_failure; - } - - *result = toNapi(err, toJS(env)); - return napi_ok; + return createErrorWithNapiValues(env, code, msg, JSC::ErrorType::Error, result); } extern "C" napi_status napi_throw_range_error(napi_env env, const char* code, const char* msg) { NAPI_PREMABLE - Zig::GlobalObject* globalObject = toJS(env); - return throwErrorWithCStrings(globalObject, code, msg, JSC::ErrorType::RangeError); + return throwErrorWithCStrings(env, code, msg, JSC::ErrorType::RangeError); } extern "C" napi_status napi_object_freeze(napi_env env, napi_value object_value) @@ -1803,24 +1759,7 @@ extern "C" napi_status napi_create_range_error(napi_env env, napi_value code, napi_value* result) { NAPI_PREMABLE - - if (UNLIKELY(!result)) { - return napi_invalid_arg; - } - - auto err = createErrorForNapi(env, code, msg, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) { - if (message.isEmpty()) { - return JSC::createRangeError(globalObject, String("Range error"_s)); - } - - return JSC::createRangeError(globalObject, message); - }); - - if (UNLIKELY(!err)) { - return napi_generic_failure; - } - *result = toNapi(err, toJS(env)); - return napi_ok; + return createErrorWithNapiValues(env, code, msg, JSC::ErrorType::RangeError, result); } extern "C" napi_status napi_get_new_target(napi_env env, diff --git a/test/napi/napi-app/main.cpp b/test/napi/napi-app/main.cpp index 927f782454799..126933e33be4f 100644 --- a/test/napi/napi-app/main.cpp +++ b/test/napi/napi-app/main.cpp @@ -698,6 +698,13 @@ napi_value create_and_throw_error(const Napi::CallbackInfo &info) { napi_value js_error_kind = info[2]; char error_kind_buf[256] = {0}; + if (get_typeof(env, js_code) == napi_null) { + js_code = nullptr; + } + if (get_typeof(env, js_msg) == napi_null) { + js_msg = nullptr; + } + assert(napi_get_value_string_utf8(env, js_error_kind, error_kind_buf, sizeof error_kind_buf, nullptr) == napi_ok); @@ -712,9 +719,10 @@ napi_value create_and_throw_error(const Napi::CallbackInfo &info) { napi_value err; napi_status create_status = create_error_function(env, js_code, js_msg, &err); - if (get_typeof(env, js_msg) != napi_string || - get_typeof(env, js_code) != napi_string) { - assert(create_status == napi_string_expected); + if (!js_msg || get_typeof(env, js_msg) != napi_string || + (js_code && get_typeof(env, js_code) != napi_string)) { + assert(create_status == napi_string_expected || + create_status == napi_invalid_arg); return ok(env); } else { assert(create_status == napi_ok); diff --git a/test/napi/napi-app/module.js b/test/napi/napi-app/module.js index 48f8a4e87c59c..60ce6c4aac683 100644 --- a/test/napi/napi-app/module.js +++ b/test/napi/napi-app/module.js @@ -91,29 +91,41 @@ nativeTests.test_get_property = () => { } }; -function loopErrorParams(callback) { +nativeTests.test_throw_functions_exhaustive = () => { for (const errorKind of ["error", "type_error", "range_error", "syntax_error"]) { for (const code of [undefined, "", "error code"]) { for (const msg of [undefined, "", "error message"]) { try { - callback(code, msg, errorKind); - console.log(`${callback.name}(${code ?? "nullptr"}, ${msg ?? "nullptr"}) did not throw`); + nativeTests.throw_error(code, msg, errorKind); + console.log(`napi_throw_${errorKind}(${code ?? "nullptr"}, ${msg ?? "nullptr"}) did not throw`); } catch (e) { console.log( - `${callback.name} threw ${e.name}: message ${JSON.stringify(e.message)}, code ${JSON.stringify(e.code)}`, + `napi_throw_${errorKind} threw ${e.name}: message ${JSON.stringify(e.message)}, code ${JSON.stringify(e.code)}`, ); } } } } -} - -nativeTests.test_throw_functions_exhaustive = () => { - loopErrorParams(nativeTests.throw_error); }; nativeTests.test_create_error_functions_exhaustive = () => { - loopErrorParams(nativeTests.create_and_throw_error); + for (const errorKind of ["error", "type_error", "range_error", "syntax_error"]) { + // null (JavaScript null) is changed to nullptr by the native function + for (const code of [undefined, null, "", 42, "error code"]) { + for (const msg of [undefined, null, "", 42, "error message"]) { + try { + nativeTests.create_and_throw_error(code, msg, errorKind); + console.log( + `napi_create_${errorKind}(${code === null ? "nullptr" : code}, ${msg === null ? "nullptr" : msg}) did not make an error`, + ); + } catch (e) { + console.log( + `create_and_throw_error(${errorKind}) threw ${e.name}: message ${JSON.stringify(e.message)}, code ${JSON.stringify(e.code)}`, + ); + } + } + } + } }; module.exports = nativeTests; From 6cc947df1fa4492404074477f892db6fcb08651a Mon Sep 17 00:00:00 2001 From: Ben Grant Date: Wed, 9 Oct 2024 17:10:54 -0700 Subject: [PATCH 4/4] Update comments in napi tests --- test/napi/napi-app/main.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/test/napi/napi-app/main.cpp b/test/napi/napi-app/main.cpp index 126933e33be4f..1e91e2ba9c90f 100644 --- a/test/napi/napi-app/main.cpp +++ b/test/napi/napi-app/main.cpp @@ -646,7 +646,8 @@ napi_value call_and_get_exception(const Napi::CallbackInfo &info) { } // throw_error(code: string|undefined, msg: string|undefined, -// kind: 'error'|'type_error'|'range_error'|'syntax_error') +// error_kind: 'error'|'type_error'|'range_error'|'syntax_error') +// if code and msg are JS undefined then change them to nullptr napi_value throw_error(const Napi::CallbackInfo &info) { napi_env env = info.Env(); @@ -688,8 +689,9 @@ napi_value throw_error(const Napi::CallbackInfo &info) { } } -// create_and_throw_error(code: string|undefined, msg: string|undefined, -// kind: 'error'|'type_error'|'range_error'|'syntax_error') +// create_and_throw_error(code: any, msg: any, +// error_kind: 'error'|'type_error'|'range_error'|'syntax_error') +// if code and msg are JS null then change them to nullptr napi_value create_and_throw_error(const Napi::CallbackInfo &info) { napi_env env = info.Env(); @@ -719,8 +721,17 @@ napi_value create_and_throw_error(const Napi::CallbackInfo &info) { napi_value err; napi_status create_status = create_error_function(env, js_code, js_msg, &err); + // cases that should fail: + // - js_msg is nullptr + // - js_msg is not a string + // - js_code is not nullptr and not a string + // also we need to make sure not to call get_typeof with nullptr, since it + // asserts that napi_typeof succeeded if (!js_msg || get_typeof(env, js_msg) != napi_string || (js_code && get_typeof(env, js_code) != napi_string)) { + // bun and node may return different errors here depending on in what order + // the parameters are checked, but what's important is that there is an + // error assert(create_status == napi_string_expected || create_status == napi_invalid_arg); return ok(env);