diff --git a/Core/Node-API-JSI/Include/napi/napi-inl.h b/Core/Node-API-JSI/Include/napi/napi-inl.h index 47120224..8cd76f58 100644 --- a/Core/Node-API-JSI/Include/napi/napi-inl.h +++ b/Core/Node-API-JSI/Include/napi/napi-inl.h @@ -1324,11 +1324,30 @@ inline const T* TypedArrayOf::Data() const { namespace details { + // Native callbacks report JavaScript errors either by throwing Napi::Error or by leaving an + // exception pending after ThrowAsJavaScriptException(). Convert both forms at the host-function + // boundary so JSI observes the original JavaScript value, matching the other Node-API backends. + template + inline jsi::Value CallHost(napi_env env, jsi::Runtime& rt, Call&& call) { + try { + auto result = call(); + if (!env->last_exception.isUndefined()) { + auto error = std::move(env->last_exception); + env->last_exception = jsi::Value::undefined(); + throw jsi::JSError{rt, jsi::Value{rt, error}}; + } + return result; + } catch (const Napi::Error& error) { + env->last_exception = jsi::Value::undefined(); + throw jsi::JSError{rt, jsi::Value{rt, static_cast(error.Value())}}; + } + } + template struct Function { static inline jsi::Value Callback(napi_env env, const jsi::Value& thisVal, const jsi::Value* args, size_t count, void* data, Callable cb) { CallbackInfo callbackInfo{env, thisVal, args, count, {}, data}; - return {env->rt, cb(callbackInfo)}; + return CallHost(env, env->rt, [&]() -> jsi::Value { return {env->rt, cb(callbackInfo)}; }); } }; @@ -1336,8 +1355,7 @@ namespace details struct Function { static inline jsi::Value Callback(napi_env env, const jsi::Value& thisVal, const jsi::Value* args, size_t count, void* data, Callable cb) { CallbackInfo callbackInfo{env, thisVal, args, count, {}, data}; - cb(callbackInfo); - return {}; + return CallHost(env, env->rt, [&]() -> jsi::Value { cb(callbackInfo); return {}; }); } }; } @@ -2298,8 +2316,10 @@ ObjectWrap::DefineClass(napi_env env, [env, newTarget, data](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { CallbackInfo callbackInfo{env, thisVal, args, count, *newTarget, data}; // TODO: use prototype to wrap object? - thisVal.getObject(rt).setProperty(rt, env->native_name, jsi::Object::createFromHostObject(rt, std::make_shared(callbackInfo))); - return {}; + return details::CallHost(env, rt, [&]() -> jsi::Value { + thisVal.getObject(rt).setProperty(rt, env->native_name, jsi::Object::createFromHostObject(rt, std::make_shared(callbackInfo))); + return {}; + }); })).getObject(rt).getFunction(rt)}; *newTarget = {rt, static_cast(constructor)}; @@ -2319,28 +2339,26 @@ ObjectWrap::DefineClass(napi_env env, if (p.staticVoidMethod != nullptr) { descriptor.setProperty(rt, "value", jsi::Function::createFromHostFunction(rt, name, 0, - [env, method{p.staticVoidMethod}, data{p.data}](jsi::Runtime& /*rt*/, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { - (*method)({env, thisVal, args, count, nullptr, data}); - return {}; + [env, method{p.staticVoidMethod}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { + return details::CallHost(env, rt, [&]() -> jsi::Value { (*method)({env, thisVal, args, count, nullptr, data}); return {}; }); })); } else if (p.staticMethod != nullptr) { descriptor.setProperty(rt, "value", jsi::Function::createFromHostFunction(rt, name, 0, [env, method{p.staticMethod}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { - return {rt, (*method)({env, thisVal, args, count, nullptr, data})}; + return details::CallHost(env, rt, [&]() -> jsi::Value { return {rt, (*method)({env, thisVal, args, count, nullptr, data})}; }); })); } else if (p.staticGetter != nullptr || p.staticSetter != nullptr) { if (p.staticGetter != nullptr) { descriptor.setProperty(rt, "get", jsi::Function::createFromHostFunction(rt, name, 0, [env, getter{p.staticGetter}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count)-> jsi::Value { - return {rt, (*getter)({env, thisVal, args, count, nullptr, data})}; + return details::CallHost(env, rt, [&]() -> jsi::Value { return {rt, (*getter)({env, thisVal, args, count, nullptr, data})}; }); })); } if (p.staticSetter != nullptr) { descriptor.setProperty(rt, "set", jsi::Function::createFromHostFunction(rt, name, 0, [env, setter{p.staticSetter}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count)-> jsi::Value { - (*setter)({env, thisVal, args, count, nullptr, data}, {env, {rt, args[0]}}); - return {}; + return details::CallHost(env, rt, [&]() -> jsi::Value { (*setter)({env, thisVal, args, count, nullptr, data}, {env, {rt, args[0]}}); return {}; }); })); } } else if (!p.staticValue.IsEmpty()) { @@ -2349,30 +2367,38 @@ ObjectWrap::DefineClass(napi_env env, } else if (p.instanceVoidMethod != nullptr) { descriptor.setProperty(rt, "value", jsi::Function::createFromHostFunction(rt, name, 0, [env, method{p.instanceVoidMethod}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { - T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; - (nativeObject->*method)({env, thisVal, args, count, nullptr, data}); - return {}; + return details::CallHost(env, rt, [&]() -> jsi::Value { + T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; + (nativeObject->*method)({env, thisVal, args, count, nullptr, data}); + return {}; + }); })); } else if (p.instanceMethod != nullptr) { descriptor.setProperty(rt, "value", jsi::Function::createFromHostFunction(rt, name, 0, [env, method{p.instanceMethod}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { - T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; - return {rt, (nativeObject->*method)({env, thisVal, args, count, nullptr, data})}; + return details::CallHost(env, rt, [&]() -> jsi::Value { + T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; + return {rt, (nativeObject->*method)({env, thisVal, args, count, nullptr, data})}; + }); })); } else if (p.instanceGetter != nullptr || p.instanceSetter != nullptr) { if (p.instanceGetter != nullptr) { descriptor.setProperty(rt, "get", jsi::Function::createFromHostFunction(rt, name, 0, [env, getter{p.instanceGetter}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { - T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; - return {rt, (nativeObject->*getter)({env, thisVal, args, count, nullptr, data})}; + return details::CallHost(env, rt, [&]() -> jsi::Value { + T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; + return {rt, (nativeObject->*getter)({env, thisVal, args, count, nullptr, data})}; + }); })); } if (p.instanceSetter != nullptr) { descriptor.setProperty(rt, "set", jsi::Function::createFromHostFunction(rt, name, 0, [env, setter{p.instanceSetter}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { - T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; - (nativeObject->*setter)({env, thisVal, args, count, nullptr, data}, {env, {rt, args[0]}}); - return {}; + return details::CallHost(env, rt, [&]() -> jsi::Value { + T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; + (nativeObject->*setter)({env, thisVal, args, count, nullptr, data}, {env, {rt, args[0]}}); + return {}; + }); })); } } else if (!p.instanceValue.IsEmpty()) { diff --git a/Tests/UnitTests/Source/Scripts/globals.d.ts b/Tests/UnitTests/Source/Scripts/globals.d.ts index 78e1a8ce..a651e384 100644 --- a/Tests/UnitTests/Source/Scripts/globals.d.ts +++ b/Tests/UnitTests/Source/Scripts/globals.d.ts @@ -1,4 +1,5 @@ declare const hostPlatform: string; declare const hostEngine: string; +declare const throwPendingAfterCallback: (callback: () => void) => void; declare const napiGetPropertyNames: (object: any) => string[]; declare const napiGetPropertyNamesRaw: (object: any) => string[]; diff --git a/Tests/UnitTests/Source/Scripts/tests.nodeApi.ts b/Tests/UnitTests/Source/Scripts/tests.nodeApi.ts index 28fa8c16..cda8123b 100644 --- a/Tests/UnitTests/Source/Scripts/tests.nodeApi.ts +++ b/Tests/UnitTests/Source/Scripts/tests.nodeApi.ts @@ -1,5 +1,38 @@ import { expect } from "chai"; +describe("native exceptions", function () { + it("reach scripts as the thrown error object, with its class and message, on every engine", function () { + // A polyfill throwing Napi::TypeError from native code must arrive as that TypeError. The JSI + // adapter used to deliver a generic Error("Exception in HostFunction: ...") instead. + let fromMethod: any; + try { + URL.createObjectURL({} as any); + } catch (e) { + fromMethod = e; + } + expect(fromMethod).to.be.an.instanceof(TypeError); + expect(fromMethod.message).to.match(/not a Blob/); + expect(fromMethod.message).to.not.match(/HostFunction/); + + let fromConstructor: any; + try { + new URL("not a url"); + } catch (e) { + fromConstructor = e; + } + expect(fromConstructor).to.be.an.instanceof(TypeError); + expect(fromConstructor.message).to.match(/Invalid URL/); + expect(fromConstructor.message).to.not.match(/HostFunction/); + }); + + it("propagate from the pending exception slot after a reentrant callback", function () { + let callbackRan = false; + expect(() => throwPendingAfterCallback(() => { callbackRan = true; })) + .to.throw(TypeError, "pending exception after callback"); + expect(callbackRan).to.equal(true); + }); +}); + describe("napi class prototype isolation (#172)", function () { // Regression coverage for #172: napi-defined class constructors must each // get a fresh per-class object as their `.prototype` property. Previously diff --git a/Tests/UnitTests/Source/Tests.JavaScript.cpp b/Tests/UnitTests/Source/Tests.JavaScript.cpp index c5b38b40..13300d11 100644 --- a/Tests/UnitTests/Source/Tests.JavaScript.cpp +++ b/Tests/UnitTests/Source/Tests.JavaScript.cpp @@ -95,6 +95,11 @@ TEST(JavaScript, All) "setExitCode"); env.Global().Set("setExitCode", setExitCodeCallback); + env.Global().Set("throwPendingAfterCallback", Napi::Function::New(env, [](const Napi::CallbackInfo& info) { + info[0].As().Call({}); + Napi::TypeError::New(info.Env(), "pending exception after callback").ThrowAsJavaScriptException(); + })); + env.Global().Set("hostPlatform", Napi::Value::From(env, JSRUNTIMEHOST_PLATFORM)); env.Global().Set("hostEngine", Napi::Value::From(env, NAPI_JAVASCRIPT_ENGINE)); diff --git a/Tests/UnitTests/Source/Tests.NodeApi.cpp b/Tests/UnitTests/Source/Tests.NodeApi.cpp index 69d88deb..09ba92a9 100644 --- a/Tests/UnitTests/Source/Tests.NodeApi.cpp +++ b/Tests/UnitTests/Source/Tests.NodeApi.cpp @@ -1,10 +1,75 @@ #include #include +#include #include #include +#include #include #include +#if defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) +TEST(NodeApi, ThrownHostErrorClearsPendingException) +{ + Babylon::AppRuntime runtime{}; + std::promise> result; + + runtime.Dispatch([&result](Napi::Env env) { + env.Global().Set("throwFromVoid", Napi::Function::New(env, [](const Napi::CallbackInfo& info) { + napi_env rawEnv{info.Env()}; + Napi::Error{rawEnv, facebook::jsi::Value{rawEnv->rt, static_cast(info[1])}}.ThrowAsJavaScriptException(); + throw Napi::Error{rawEnv, facebook::jsi::Value{rawEnv->rt, static_cast(info[0])}}; + })); + env.Global().Set("throwFromValue", Napi::Function::New(env, [](const Napi::CallbackInfo& info) -> Napi::Value { + napi_env rawEnv{info.Env()}; + Napi::Error{rawEnv, facebook::jsi::Value{rawEnv->rt, static_cast(info[1])}}.ThrowAsJavaScriptException(); + throw Napi::Error{rawEnv, facebook::jsi::Value{rawEnv->rt, static_cast(info[0])}}; + })); + env.Global().Set("nextCallback", Napi::Function::New(env, [](const Napi::CallbackInfo&) {})); + + std::array passed{true, true, true}; + for (const char* callback : {"throwFromVoid", "throwFromValue"}) + { + env.Global().Set("throwingCallback", env.Global().Get(callback)); + for (bool sameError : {true, false}) + { + env.Global().Set("sameError", sameError); + const bool originalErrorCaught = Napi::Eval(env, R"( + (() => { + const thrown = new TypeError("thrown"); + const pending = sameError ? thrown : new RangeError("pending"); + try { + throwingCallback(thrown, pending); + } catch (error) { + return error === thrown; + } + return false; + })() + )", "").As().Value(); + const bool exceptionCleared{!env.IsExceptionPending()}; + const bool nextCallbackSucceeded = Napi::Eval(env, R"( + (() => { + try { + return nextCallback() === undefined; + } catch (error) { + return false; + } + })() + )", "").As().Value(); + passed[0] = passed[0] && originalErrorCaught; + passed[1] = passed[1] && exceptionCleared; + passed[2] = passed[2] && nextCallbackSucceeded; + } + } + result.set_value(passed); + }); + + const auto passed{result.get_future().get()}; + EXPECT_TRUE(passed[0]) << "The JavaScript catch must receive the thrown error object"; + EXPECT_TRUE(passed[1]) << "The caught host error must not remain pending"; + EXPECT_TRUE(passed[2]) << "The next host callback must return without throwing"; +} +#endif + // The V8JSI Node-API shim does not implement napi_create_dataview / // napi_get_dataview_info (its DataView::New throws "TODO"), so this native test // only builds on the Chakra, V8, and JavaScriptCore backends. The size_t-width