From 91e373f8e9ef3305ff3a3e1faef111f079e5ef67 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Thu, 15 Jun 2023 20:02:58 +0800 Subject: [PATCH] node-api: return napi_exception_pending on proxy handlers Accessing JS objects can be trapped with proxy handlers. These handlers can throw when no node-api errors occur. In such cases, `napi_pending_exception` should be returned. PR-URL: https://github.com/nodejs/node/pull/48607 Reviewed-By: Gabriel Schulhof Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 65 +++++++-------- test/js-native-api/common-inl.h | 25 ++++-- test/js-native-api/test_object/binding.gyp | 6 ++ .../test_object/test_exceptions.c | 82 +++++++++++++++++++ .../test_object/test_exceptions.js | 18 ++++ 5 files changed, 155 insertions(+), 41 deletions(-) create mode 100644 test/js-native-api/test_object/test_exceptions.c create mode 100644 test/js-native-api/test_object/test_exceptions.js diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 044244eccfe9ea..7f96a7edd82ef7 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -1105,7 +1105,8 @@ napi_status NAPI_CDECL napi_set_property(napi_env env, v8::Maybe set_maybe = obj->Set(context, k, val); - RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure); + RETURN_STATUS_IF_FALSE_WITH_PREAMBLE( + env, set_maybe.FromMaybe(false), napi_generic_failure); return GET_RETURN_STATUS(env); } @@ -1125,7 +1126,7 @@ napi_status NAPI_CDECL napi_has_property(napi_env env, v8::Local k = v8impl::V8LocalValueFromJsValue(key); v8::Maybe has_maybe = obj->Has(context, k); - CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure); + CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure); *result = has_maybe.FromMaybe(false); return GET_RETURN_STATUS(env); @@ -1147,7 +1148,7 @@ napi_status NAPI_CDECL napi_get_property(napi_env env, auto get_maybe = obj->Get(context, k); - CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure); + CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure); v8::Local val = get_maybe.ToLocalChecked(); *result = v8impl::JsValueFromV8LocalValue(val); @@ -1167,7 +1168,7 @@ napi_status NAPI_CDECL napi_delete_property(napi_env env, CHECK_TO_OBJECT(env, context, obj, object); v8::Maybe delete_maybe = obj->Delete(context, k); - CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure); + CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, delete_maybe, napi_generic_failure); if (result != nullptr) *result = delete_maybe.FromMaybe(false); @@ -1189,7 +1190,7 @@ napi_status NAPI_CDECL napi_has_own_property(napi_env env, v8::Local k = v8impl::V8LocalValueFromJsValue(key); RETURN_STATUS_IF_FALSE(env, k->IsName(), napi_name_expected); v8::Maybe has_maybe = obj->HasOwnProperty(context, k.As()); - CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure); + CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure); *result = has_maybe.FromMaybe(false); return GET_RETURN_STATUS(env); @@ -1214,7 +1215,8 @@ napi_status NAPI_CDECL napi_set_named_property(napi_env env, v8::Maybe set_maybe = obj->Set(context, key, val); - RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure); + RETURN_STATUS_IF_FALSE_WITH_PREAMBLE( + env, set_maybe.FromMaybe(false), napi_generic_failure); return GET_RETURN_STATUS(env); } @@ -1235,7 +1237,7 @@ napi_status NAPI_CDECL napi_has_named_property(napi_env env, v8::Maybe has_maybe = obj->Has(context, key); - CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure); + CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure); *result = has_maybe.FromMaybe(false); return GET_RETURN_STATUS(env); @@ -1259,7 +1261,7 @@ napi_status NAPI_CDECL napi_get_named_property(napi_env env, auto get_maybe = obj->Get(context, key); - CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure); + CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure); v8::Local val = get_maybe.ToLocalChecked(); *result = v8impl::JsValueFromV8LocalValue(val); @@ -1281,7 +1283,8 @@ napi_status NAPI_CDECL napi_set_element(napi_env env, v8::Local val = v8impl::V8LocalValueFromJsValue(value); auto set_maybe = obj->Set(context, index, val); - RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure); + RETURN_STATUS_IF_FALSE_WITH_PREAMBLE( + env, set_maybe.FromMaybe(false), napi_generic_failure); return GET_RETURN_STATUS(env); } @@ -1300,7 +1303,7 @@ napi_status NAPI_CDECL napi_has_element(napi_env env, v8::Maybe has_maybe = obj->Has(context, index); - CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure); + CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure); *result = has_maybe.FromMaybe(false); return GET_RETURN_STATUS(env); @@ -1320,7 +1323,7 @@ napi_status NAPI_CDECL napi_get_element(napi_env env, auto get_maybe = obj->Get(context, index); - CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure); + CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure); *result = v8impl::JsValueFromV8LocalValue(get_maybe.ToLocalChecked()); return GET_RETURN_STATUS(env); @@ -1337,7 +1340,7 @@ napi_status NAPI_CDECL napi_delete_element(napi_env env, CHECK_TO_OBJECT(env, context, obj, object); v8::Maybe delete_maybe = obj->Delete(context, index); - CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure); + CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, delete_maybe, napi_generic_failure); if (result != nullptr) *result = delete_maybe.FromMaybe(false); @@ -1385,9 +1388,8 @@ napi_define_properties(napi_env env, auto define_maybe = obj->DefineProperty(context, property_name, descriptor); - if (!define_maybe.FromMaybe(false)) { - return napi_set_last_error(env, napi_invalid_arg); - } + RETURN_STATUS_IF_FALSE_WITH_PREAMBLE( + env, define_maybe.FromMaybe(false), napi_invalid_arg); } else if (p->method != nullptr) { v8::Local method; STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction( @@ -1400,34 +1402,28 @@ napi_define_properties(napi_env env, auto define_maybe = obj->DefineProperty(context, property_name, descriptor); - if (!define_maybe.FromMaybe(false)) { - return napi_set_last_error(env, napi_generic_failure); - } + RETURN_STATUS_IF_FALSE_WITH_PREAMBLE( + env, define_maybe.FromMaybe(false), napi_generic_failure); } else { v8::Local value = v8impl::V8LocalValueFromJsValue(p->value); - bool defined_successfully = false; + v8::Maybe define_maybe = v8::Just(false); if ((p->attributes & napi_enumerable) && (p->attributes & napi_writable) && (p->attributes & napi_configurable)) { // Use a fast path for this type of data property. - auto define_maybe = - obj->CreateDataProperty(context, property_name, value); - defined_successfully = define_maybe.FromMaybe(false); + define_maybe = obj->CreateDataProperty(context, property_name, value); } else { v8::PropertyDescriptor descriptor(value, (p->attributes & napi_writable) != 0); descriptor.set_enumerable((p->attributes & napi_enumerable) != 0); descriptor.set_configurable((p->attributes & napi_configurable) != 0); - auto define_maybe = - obj->DefineProperty(context, property_name, descriptor); - defined_successfully = define_maybe.FromMaybe(false); + define_maybe = obj->DefineProperty(context, property_name, descriptor); } - if (!defined_successfully) { - return napi_set_last_error(env, napi_invalid_arg); - } + RETURN_STATUS_IF_FALSE_WITH_PREAMBLE( + env, define_maybe.FromMaybe(false), napi_invalid_arg); } } @@ -1525,6 +1521,7 @@ napi_status NAPI_CDECL napi_get_prototype(napi_env env, v8::Local obj; CHECK_TO_OBJECT(env, context, obj, object); + // This doesn't invokes Proxy's [[GetPrototypeOf]] handler. v8::Local val = obj->GetPrototype(); *result = v8impl::JsValueFromV8LocalValue(val); return GET_RETURN_STATUS(env); @@ -2058,15 +2055,11 @@ napi_status NAPI_CDECL napi_call_function(napi_env env, argc, reinterpret_cast*>(const_cast(argv))); - if (try_catch.HasCaught()) { - return napi_set_last_error(env, napi_pending_exception); - } else { - if (result != nullptr) { - CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure); - *result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked()); - } - return napi_clear_last_error(env); + CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, maybe, napi_generic_failure); + if (result != nullptr) { + *result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked()); } + return napi_clear_last_error(env); } napi_status NAPI_CDECL napi_get_global(napi_env env, napi_value* result) { diff --git a/test/js-native-api/common-inl.h b/test/js-native-api/common-inl.h index d4db4a3e58bdc6..51ed4cdd4e11ed 100644 --- a/test/js-native-api/common-inl.h +++ b/test/js-native-api/common-inl.h @@ -38,17 +38,32 @@ inline void add_last_status(napi_env env, const char* key, napi_value return_value) { napi_value prop_value; + napi_value exception; const napi_extended_error_info* p_last_error; NODE_API_CALL_RETURN_VOID(env, napi_get_last_error_info(env, &p_last_error)); + // Content of p_last_error can be updated in subsequent node-api calls. + // Retrieve it immediately. + const char* error_message = p_last_error->error_message == NULL + ? "napi_ok" + : p_last_error->error_message; + + bool is_exception_pending; + NODE_API_CALL_RETURN_VOID( + env, napi_is_exception_pending(env, &is_exception_pending)); + if (is_exception_pending) { + NODE_API_CALL_RETURN_VOID( + env, napi_get_and_clear_last_exception(env, &exception)); + char exception_key[50]; + snprintf(exception_key, sizeof(exception_key), "%s%s", key, "Exception"); + NODE_API_CALL_RETURN_VOID( + env, + napi_set_named_property(env, return_value, exception_key, exception)); + } NODE_API_CALL_RETURN_VOID( env, napi_create_string_utf8( - env, - (p_last_error->error_message == NULL ? "napi_ok" - : p_last_error->error_message), - NAPI_AUTO_LENGTH, - &prop_value)); + env, error_message, NAPI_AUTO_LENGTH, &prop_value)); NODE_API_CALL_RETURN_VOID( env, napi_set_named_property(env, return_value, key, prop_value)); } diff --git a/test/js-native-api/test_object/binding.gyp b/test/js-native-api/test_object/binding.gyp index b81f502584619e..e63c1649a05140 100644 --- a/test/js-native-api/test_object/binding.gyp +++ b/test/js-native-api/test_object/binding.gyp @@ -6,6 +6,12 @@ "test_null.c", "test_object.c" ] + }, + { + "target_name": "test_exceptions", + "sources": [ + "test_exceptions.c", + ] } ] } diff --git a/test/js-native-api/test_object/test_exceptions.c b/test/js-native-api/test_object/test_exceptions.c new file mode 100644 index 00000000000000..7d2f18fd6ff469 --- /dev/null +++ b/test/js-native-api/test_object/test_exceptions.c @@ -0,0 +1,82 @@ +#include +#include +#include +#include "../common.h" +#include "../entry_point.h" + +static napi_value TestExceptions(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value args[1]; + NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL)); + + napi_value target = args[0]; + napi_value exception, key, value; + napi_status status; + bool is_exception_pending; + bool bool_result; + + NODE_API_CALL(env, + napi_create_string_utf8(env, "key", NAPI_AUTO_LENGTH, &key)); + NODE_API_CALL( + env, napi_create_string_utf8(env, "value", NAPI_AUTO_LENGTH, &value)); + +#define PROCEDURE(call) \ + { \ + status = (call); \ + NODE_API_ASSERT( \ + env, status == napi_pending_exception, "expect exception pending"); \ + NODE_API_CALL(env, napi_is_exception_pending(env, &is_exception_pending)); \ + NODE_API_ASSERT(env, is_exception_pending, "expect exception pending"); \ + NODE_API_CALL(env, napi_get_and_clear_last_exception(env, &exception)); \ + } + // discard the exception values. + + // properties + PROCEDURE(napi_set_property(env, target, key, value)); + PROCEDURE(napi_set_named_property(env, target, "key", value)); + PROCEDURE(napi_has_property(env, target, key, &bool_result)); + PROCEDURE(napi_has_own_property(env, target, key, &bool_result)); + PROCEDURE(napi_has_named_property(env, target, "key", &bool_result)); + PROCEDURE(napi_get_property(env, target, key, &value)); + PROCEDURE(napi_get_named_property(env, target, "key", &value)); + PROCEDURE(napi_delete_property(env, target, key, &bool_result)); + + // elements + PROCEDURE(napi_set_element(env, target, 0, value)); + PROCEDURE(napi_has_element(env, target, 0, &bool_result)); + PROCEDURE(napi_get_element(env, target, 0, &value)); + PROCEDURE(napi_delete_element(env, target, 0, &bool_result)); + + napi_property_descriptor descriptors[] = { + DECLARE_NODE_API_PROPERTY_VALUE("key", value), + }; + PROCEDURE(napi_define_properties( + env, target, sizeof(descriptors) / sizeof(*descriptors), descriptors)); + + PROCEDURE(napi_get_all_property_names(env, + target, + napi_key_own_only, + napi_key_enumerable, + napi_key_keep_numbers, + &value)); + PROCEDURE(napi_get_property_names(env, target, &value)); + + return NULL; +} + +EXTERN_C_START +napi_value Init(napi_env env, napi_value exports) { + napi_property_descriptor descriptors[] = { + DECLARE_NODE_API_PROPERTY("testExceptions", TestExceptions), + }; + + NODE_API_CALL( + env, + napi_define_properties(env, + exports, + sizeof(descriptors) / sizeof(*descriptors), + descriptors)); + + return exports; +} +EXTERN_C_END diff --git a/test/js-native-api/test_object/test_exceptions.js b/test/js-native-api/test_object/test_exceptions.js new file mode 100644 index 00000000000000..1e45d64368b01c --- /dev/null +++ b/test/js-native-api/test_object/test_exceptions.js @@ -0,0 +1,18 @@ +'use strict'; +const common = require('../../common'); + +// Test +const { testExceptions } = require(`./build/${common.buildType}/test_exceptions`); + +function throws() { + throw new Error('foobar'); +} +testExceptions(new Proxy({}, { + get: common.mustCallAtLeast(throws, 1), + getOwnPropertyDescriptor: common.mustCallAtLeast(throws, 1), + defineProperty: common.mustCallAtLeast(throws, 1), + deleteProperty: common.mustCallAtLeast(throws, 1), + has: common.mustCallAtLeast(throws, 1), + set: common.mustCallAtLeast(throws, 1), + ownKeys: common.mustCallAtLeast(throws, 1), +}));