Skip to content

Commit a4f2dbf

Browse files
legendecaslegendecas
committed
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.
1 parent bf0f078 commit a4f2dbf

File tree

5 files changed

+155
-41
lines changed

5 files changed

+155
-41
lines changed

‎src/js_native_api_v8.cc‎

Lines changed: 29 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1105,7 +1105,8 @@ napi_status NAPI_CDECL napi_set_property(napi_env env,
11051105

11061106
v8::Maybe<bool> set_maybe = obj->Set(context, k, val);
11071107

1108-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1108+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1109+
env, set_maybe.FromMaybe(false), napi_generic_failure);
11091110
return GET_RETURN_STATUS(env);
11101111
}
11111112

@@ -1125,7 +1126,7 @@ napi_status NAPI_CDECL napi_has_property(napi_env env,
11251126
v8::Local<v8::Value> k = v8impl::V8LocalValueFromJsValue(key);
11261127
v8::Maybe<bool> has_maybe = obj->Has(context, k);
11271128

1128-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1129+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
11291130

11301131
*result = has_maybe.FromMaybe(false);
11311132
return GET_RETURN_STATUS(env);
@@ -1147,7 +1148,7 @@ napi_status NAPI_CDECL napi_get_property(napi_env env,
11471148

11481149
auto get_maybe = obj->Get(context, k);
11491150

1150-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1151+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
11511152

11521153
v8::Local<v8::Value> val = get_maybe.ToLocalChecked();
11531154
*result = v8impl::JsValueFromV8LocalValue(val);
@@ -1167,7 +1168,7 @@ napi_status NAPI_CDECL napi_delete_property(napi_env env,
11671168

11681169
CHECK_TO_OBJECT(env, context, obj, object);
11691170
v8::Maybe<bool> delete_maybe = obj->Delete(context, k);
1170-
CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure);
1171+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, delete_maybe, napi_generic_failure);
11711172

11721173
if (result != nullptr) *result = delete_maybe.FromMaybe(false);
11731174

@@ -1189,7 +1190,7 @@ napi_status NAPI_CDECL napi_has_own_property(napi_env env,
11891190
v8::Local<v8::Value> k = v8impl::V8LocalValueFromJsValue(key);
11901191
RETURN_STATUS_IF_FALSE(env, k->IsName(), napi_name_expected);
11911192
v8::Maybe<bool> has_maybe = obj->HasOwnProperty(context, k.As<v8::Name>());
1192-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1193+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
11931194
*result = has_maybe.FromMaybe(false);
11941195

11951196
return GET_RETURN_STATUS(env);
@@ -1214,7 +1215,8 @@ napi_status NAPI_CDECL napi_set_named_property(napi_env env,
12141215

12151216
v8::Maybe<bool> set_maybe = obj->Set(context, key, val);
12161217

1217-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1218+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1219+
env, set_maybe.FromMaybe(false), napi_generic_failure);
12181220
return GET_RETURN_STATUS(env);
12191221
}
12201222

@@ -1235,7 +1237,7 @@ napi_status NAPI_CDECL napi_has_named_property(napi_env env,
12351237

12361238
v8::Maybe<bool> has_maybe = obj->Has(context, key);
12371239

1238-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1240+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
12391241

12401242
*result = has_maybe.FromMaybe(false);
12411243
return GET_RETURN_STATUS(env);
@@ -1259,7 +1261,7 @@ napi_status NAPI_CDECL napi_get_named_property(napi_env env,
12591261

12601262
auto get_maybe = obj->Get(context, key);
12611263

1262-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1264+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
12631265

12641266
v8::Local<v8::Value> val = get_maybe.ToLocalChecked();
12651267
*result = v8impl::JsValueFromV8LocalValue(val);
@@ -1281,7 +1283,8 @@ napi_status NAPI_CDECL napi_set_element(napi_env env,
12811283
v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
12821284
auto set_maybe = obj->Set(context, index, val);
12831285

1284-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1286+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1287+
env, set_maybe.FromMaybe(false), napi_generic_failure);
12851288

12861289
return GET_RETURN_STATUS(env);
12871290
}
@@ -1300,7 +1303,7 @@ napi_status NAPI_CDECL napi_has_element(napi_env env,
13001303

13011304
v8::Maybe<bool> has_maybe = obj->Has(context, index);
13021305

1303-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1306+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
13041307

13051308
*result = has_maybe.FromMaybe(false);
13061309
return GET_RETURN_STATUS(env);
@@ -1320,7 +1323,7 @@ napi_status NAPI_CDECL napi_get_element(napi_env env,
13201323

13211324
auto get_maybe = obj->Get(context, index);
13221325

1323-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1326+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
13241327

13251328
*result = v8impl::JsValueFromV8LocalValue(get_maybe.ToLocalChecked());
13261329
return GET_RETURN_STATUS(env);
@@ -1337,7 +1340,7 @@ napi_status NAPI_CDECL napi_delete_element(napi_env env,
13371340

13381341
CHECK_TO_OBJECT(env, context, obj, object);
13391342
v8::Maybe<bool> delete_maybe = obj->Delete(context, index);
1340-
CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure);
1343+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, delete_maybe, napi_generic_failure);
13411344

13421345
if (result != nullptr) *result = delete_maybe.FromMaybe(false);
13431346

@@ -1385,9 +1388,8 @@ napi_define_properties(napi_env env,
13851388
auto define_maybe =
13861389
obj->DefineProperty(context, property_name, descriptor);
13871390

1388-
if (!define_maybe.FromMaybe(false)) {
1389-
return napi_set_last_error(env, napi_invalid_arg);
1390-
}
1391+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1392+
env, define_maybe.FromMaybe(false), napi_invalid_arg);
13911393
} else if (p->method != nullptr) {
13921394
v8::Local<v8::Function> method;
13931395
STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction(
@@ -1400,34 +1402,28 @@ napi_define_properties(napi_env env,
14001402
auto define_maybe =
14011403
obj->DefineProperty(context, property_name, descriptor);
14021404

1403-
if (!define_maybe.FromMaybe(false)) {
1404-
return napi_set_last_error(env, napi_generic_failure);
1405-
}
1405+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1406+
env, define_maybe.FromMaybe(false), napi_generic_failure);
14061407
} else {
14071408
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
1408-
bool defined_successfully = false;
1409+
v8::Maybe<bool> define_maybe = v8::Just(false);
14091410

14101411
if ((p->attributes & napi_enumerable) &&
14111412
(p->attributes & napi_writable) &&
14121413
(p->attributes & napi_configurable)) {
14131414
// Use a fast path for this type of data property.
1414-
auto define_maybe =
1415-
obj->CreateDataProperty(context, property_name, value);
1416-
defined_successfully = define_maybe.FromMaybe(false);
1415+
define_maybe = obj->CreateDataProperty(context, property_name, value);
14171416
} else {
14181417
v8::PropertyDescriptor descriptor(value,
14191418
(p->attributes & napi_writable) != 0);
14201419
descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
14211420
descriptor.set_configurable((p->attributes & napi_configurable) != 0);
14221421

1423-
auto define_maybe =
1424-
obj->DefineProperty(context, property_name, descriptor);
1425-
defined_successfully = define_maybe.FromMaybe(false);
1422+
define_maybe = obj->DefineProperty(context, property_name, descriptor);
14261423
}
14271424

1428-
if (!defined_successfully) {
1429-
return napi_set_last_error(env, napi_invalid_arg);
1430-
}
1425+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1426+
env, define_maybe.FromMaybe(false), napi_invalid_arg);
14311427
}
14321428
}
14331429

@@ -1525,6 +1521,7 @@ napi_status NAPI_CDECL napi_get_prototype(napi_env env,
15251521
v8::Local<v8::Object> obj;
15261522
CHECK_TO_OBJECT(env, context, obj, object);
15271523

1524+
// This doesn't invokes Proxy's [[GetPrototypeOf]] handler.
15281525
v8::Local<v8::Value> val = obj->GetPrototype();
15291526
*result = v8impl::JsValueFromV8LocalValue(val);
15301527
return GET_RETURN_STATUS(env);
@@ -2058,15 +2055,11 @@ napi_status NAPI_CDECL napi_call_function(napi_env env,
20582055
argc,
20592056
reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv)));
20602057

2061-
if (try_catch.HasCaught()) {
2062-
return napi_set_last_error(env, napi_pending_exception);
2063-
} else {
2064-
if (result != nullptr) {
2065-
CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure);
2066-
*result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
2067-
}
2068-
return napi_clear_last_error(env);
2058+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, maybe, napi_generic_failure);
2059+
if (result != nullptr) {
2060+
*result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
20692061
}
2062+
return napi_clear_last_error(env);
20702063
}
20712064

20722065
napi_status NAPI_CDECL napi_get_global(napi_env env, napi_value* result) {

‎test/js-native-api/common-inl.h‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,17 +38,32 @@ inline void add_last_status(napi_env env,
3838
const char* key,
3939
napi_value return_value) {
4040
napi_value prop_value;
41+
napi_value exception;
4142
const napi_extended_error_info* p_last_error;
4243
NODE_API_CALL_RETURN_VOID(env, napi_get_last_error_info(env, &p_last_error));
44+
// Content of p_last_error can be updated in subsequent node-api calls.
45+
// Retrieve it immediately.
46+
const char* error_message = p_last_error->error_message == NULL
47+
? "napi_ok"
48+
: p_last_error->error_message;
49+
50+
bool is_exception_pending;
51+
NODE_API_CALL_RETURN_VOID(
52+
env, napi_is_exception_pending(env, &is_exception_pending));
53+
if (is_exception_pending) {
54+
NODE_API_CALL_RETURN_VOID(
55+
env, napi_get_and_clear_last_exception(env, &exception));
56+
char exception_key[50];
57+
snprintf(exception_key, sizeof(exception_key), "%s%s", key, "Exception");
58+
NODE_API_CALL_RETURN_VOID(
59+
env,
60+
napi_set_named_property(env, return_value, exception_key, exception));
61+
}
4362

4463
NODE_API_CALL_RETURN_VOID(
4564
env,
4665
napi_create_string_utf8(
47-
env,
48-
(p_last_error->error_message == NULL ? "napi_ok"
49-
: p_last_error->error_message),
50-
NAPI_AUTO_LENGTH,
51-
&prop_value));
66+
env, error_message, NAPI_AUTO_LENGTH, &prop_value));
5267
NODE_API_CALL_RETURN_VOID(
5368
env, napi_set_named_property(env, return_value, key, prop_value));
5469
}

‎test/js-native-api/test_object/binding.gyp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,12 @@
66
"test_null.c",
77
"test_object.c"
88
]
9+
},
10+
{
11+
"target_name": "test_exceptions",
12+
"sources": [
13+
"test_exceptions.c",
14+
]
915
}
1016
]
1117
}

‎test/js-native-api/test_object/test_exceptions.c‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
#include <assert.h>
2+
#include <js_native_api.h>
3+
#include <string.h>
4+
#include "../common.h"
5+
#include "../entry_point.h"
6+
7+
static napi_value TestExceptions(napi_env env, napi_callback_info info) {
8+
size_t argc = 1;
9+
napi_value args[1];
10+
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
11+
12+
napi_value target = args[0];
13+
napi_value exception, key, value;
14+
napi_status status;
15+
bool is_exception_pending;
16+
bool bool_result;
17+
18+
NODE_API_CALL(env,
19+
napi_create_string_utf8(env, "key", NAPI_AUTO_LENGTH, &key));
20+
NODE_API_CALL(
21+
env, napi_create_string_utf8(env, "value", NAPI_AUTO_LENGTH, &value));
22+
23+
#define PROCEDURE(call) \
24+
{ \
25+
status = (call); \
26+
NODE_API_ASSERT( \
27+
env, status == napi_pending_exception, "expect exception pending"); \
28+
NODE_API_CALL(env, napi_is_exception_pending(env, &is_exception_pending)); \
29+
NODE_API_ASSERT(env, is_exception_pending, "expect exception pending"); \
30+
NODE_API_CALL(env, napi_get_and_clear_last_exception(env, &exception)); \
31+
}
32+
// discard the exception values.
33+
34+
// properties
35+
PROCEDURE(napi_set_property(env, target, key, value));
36+
PROCEDURE(napi_set_named_property(env, target, "key", value));
37+
PROCEDURE(napi_has_property(env, target, key, &bool_result));
38+
PROCEDURE(napi_has_own_property(env, target, key, &bool_result));
39+
PROCEDURE(napi_has_named_property(env, target, "key", &bool_result));
40+
PROCEDURE(napi_get_property(env, target, key, &value));
41+
PROCEDURE(napi_get_named_property(env, target, "key", &value));
42+
PROCEDURE(napi_delete_property(env, target, key, &bool_result));
43+
44+
// elements
45+
PROCEDURE(napi_set_element(env, target, 0, value));
46+
PROCEDURE(napi_has_element(env, target, 0, &bool_result));
47+
PROCEDURE(napi_get_element(env, target, 0, &value));
48+
PROCEDURE(napi_delete_element(env, target, 0, &bool_result));
49+
50+
napi_property_descriptor descriptors[] = {
51+
DECLARE_NODE_API_PROPERTY_VALUE("key", value),
52+
};
53+
PROCEDURE(napi_define_properties(
54+
env, target, sizeof(descriptors) / sizeof(*descriptors), descriptors));
55+
56+
PROCEDURE(napi_get_all_property_names(env,
57+
target,
58+
napi_key_own_only,
59+
napi_key_enumerable,
60+
napi_key_keep_numbers,
61+
&value));
62+
PROCEDURE(napi_get_property_names(env, target, &value));
63+
64+
return NULL;
65+
}
66+
67+
EXTERN_C_START
68+
napi_value Init(napi_env env, napi_value exports) {
69+
napi_property_descriptor descriptors[] = {
70+
DECLARE_NODE_API_PROPERTY("testExceptions", TestExceptions),
71+
};
72+
73+
NODE_API_CALL(
74+
env,
75+
napi_define_properties(env,
76+
exports,
77+
sizeof(descriptors) / sizeof(*descriptors),
78+
descriptors));
79+
80+
return exports;
81+
}
82+
EXTERN_C_END

‎test/js-native-api/test_object/test_exceptions.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
'use strict';
2+
const common = require('../../common');
3+
4+
// Test
5+
const { testExceptions } = require(`./build/${common.buildType}/test_exceptions`);
6+
7+
function throws() {
8+
throw new Error('foobar');
9+
}
10+
testExceptions(new Proxy({}, {
11+
get: common.mustCallAtLeast(throws, 1),
12+
getOwnPropertyDescriptor: common.mustCallAtLeast(throws, 1),
13+
defineProperty: common.mustCallAtLeast(throws, 1),
14+
deleteProperty: common.mustCallAtLeast(throws, 1),
15+
has: common.mustCallAtLeast(throws, 1),
16+
set: common.mustCallAtLeast(throws, 1),
17+
ownKeys: common.mustCallAtLeast(throws, 1),
18+
}));

0 commit comments

Comments
 (0)