Skip to content

Commit 3b3bc46

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 e8810b9 commit 3b3bc46

File tree

5 files changed

+151
-39
lines changed

5 files changed

+151
-39
lines changed

‎src/js_native_api_v8.cc

+29-36
Original file line numberDiff line numberDiff line change
@@ -1069,7 +1069,8 @@ napi_status NAPI_CDECL napi_set_property(napi_env env,
10691069

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

1072-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1072+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1073+
env, set_maybe.FromMaybe(false), napi_generic_failure);
10731074
return GET_RETURN_STATUS(env);
10741075
}
10751076

@@ -1089,7 +1090,7 @@ napi_status NAPI_CDECL napi_has_property(napi_env env,
10891090
v8::Local<v8::Value> k = v8impl::V8LocalValueFromJsValue(key);
10901091
v8::Maybe<bool> has_maybe = obj->Has(context, k);
10911092

1092-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1093+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
10931094

10941095
*result = has_maybe.FromMaybe(false);
10951096
return GET_RETURN_STATUS(env);
@@ -1111,7 +1112,7 @@ napi_status NAPI_CDECL napi_get_property(napi_env env,
11111112

11121113
auto get_maybe = obj->Get(context, k);
11131114

1114-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1115+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
11151116

11161117
v8::Local<v8::Value> val = get_maybe.ToLocalChecked();
11171118
*result = v8impl::JsValueFromV8LocalValue(val);
@@ -1131,7 +1132,7 @@ napi_status NAPI_CDECL napi_delete_property(napi_env env,
11311132

11321133
CHECK_TO_OBJECT(env, context, obj, object);
11331134
v8::Maybe<bool> delete_maybe = obj->Delete(context, k);
1134-
CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure);
1135+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, delete_maybe, napi_generic_failure);
11351136

11361137
if (result != nullptr) *result = delete_maybe.FromMaybe(false);
11371138

@@ -1153,7 +1154,7 @@ napi_status NAPI_CDECL napi_has_own_property(napi_env env,
11531154
v8::Local<v8::Value> k = v8impl::V8LocalValueFromJsValue(key);
11541155
RETURN_STATUS_IF_FALSE(env, k->IsName(), napi_name_expected);
11551156
v8::Maybe<bool> has_maybe = obj->HasOwnProperty(context, k.As<v8::Name>());
1156-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1157+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
11571158
*result = has_maybe.FromMaybe(false);
11581159

11591160
return GET_RETURN_STATUS(env);
@@ -1178,7 +1179,8 @@ napi_status NAPI_CDECL napi_set_named_property(napi_env env,
11781179

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

1181-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1182+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1183+
env, set_maybe.FromMaybe(false), napi_generic_failure);
11821184
return GET_RETURN_STATUS(env);
11831185
}
11841186

@@ -1199,7 +1201,7 @@ napi_status NAPI_CDECL napi_has_named_property(napi_env env,
11991201

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

1202-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1204+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
12031205

12041206
*result = has_maybe.FromMaybe(false);
12051207
return GET_RETURN_STATUS(env);
@@ -1223,7 +1225,7 @@ napi_status NAPI_CDECL napi_get_named_property(napi_env env,
12231225

12241226
auto get_maybe = obj->Get(context, key);
12251227

1226-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1228+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
12271229

12281230
v8::Local<v8::Value> val = get_maybe.ToLocalChecked();
12291231
*result = v8impl::JsValueFromV8LocalValue(val);
@@ -1245,7 +1247,8 @@ napi_status NAPI_CDECL napi_set_element(napi_env env,
12451247
v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
12461248
auto set_maybe = obj->Set(context, index, val);
12471249

1248-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1250+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1251+
env, set_maybe.FromMaybe(false), napi_generic_failure);
12491252

12501253
return GET_RETURN_STATUS(env);
12511254
}
@@ -1264,7 +1267,7 @@ napi_status NAPI_CDECL napi_has_element(napi_env env,
12641267

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

1267-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1270+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
12681271

12691272
*result = has_maybe.FromMaybe(false);
12701273
return GET_RETURN_STATUS(env);
@@ -1284,7 +1287,7 @@ napi_status NAPI_CDECL napi_get_element(napi_env env,
12841287

12851288
auto get_maybe = obj->Get(context, index);
12861289

1287-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1290+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
12881291

12891292
*result = v8impl::JsValueFromV8LocalValue(get_maybe.ToLocalChecked());
12901293
return GET_RETURN_STATUS(env);
@@ -1301,7 +1304,7 @@ napi_status NAPI_CDECL napi_delete_element(napi_env env,
13011304

13021305
CHECK_TO_OBJECT(env, context, obj, object);
13031306
v8::Maybe<bool> delete_maybe = obj->Delete(context, index);
1304-
CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure);
1307+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, delete_maybe, napi_generic_failure);
13051308

13061309
if (result != nullptr) *result = delete_maybe.FromMaybe(false);
13071310

@@ -1349,9 +1352,8 @@ napi_define_properties(napi_env env,
13491352
auto define_maybe =
13501353
obj->DefineProperty(context, property_name, descriptor);
13511354

1352-
if (!define_maybe.FromMaybe(false)) {
1353-
return napi_set_last_error(env, napi_invalid_arg);
1354-
}
1355+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1356+
env, define_maybe.FromMaybe(false), napi_invalid_arg);
13551357
} else if (p->method != nullptr) {
13561358
v8::Local<v8::Function> method;
13571359
STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction(
@@ -1364,34 +1366,28 @@ napi_define_properties(napi_env env,
13641366
auto define_maybe =
13651367
obj->DefineProperty(context, property_name, descriptor);
13661368

1367-
if (!define_maybe.FromMaybe(false)) {
1368-
return napi_set_last_error(env, napi_generic_failure);
1369-
}
1369+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1370+
env, define_maybe.FromMaybe(false), napi_generic_failure);
13701371
} else {
13711372
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
1372-
bool defined_successfully = false;
1373+
v8::Maybe<bool> define_maybe = v8::Just(false);
13731374

13741375
if ((p->attributes & napi_enumerable) &&
13751376
(p->attributes & napi_writable) &&
13761377
(p->attributes & napi_configurable)) {
13771378
// Use a fast path for this type of data property.
1378-
auto define_maybe =
1379-
obj->CreateDataProperty(context, property_name, value);
1380-
defined_successfully = define_maybe.FromMaybe(false);
1379+
define_maybe = obj->CreateDataProperty(context, property_name, value);
13811380
} else {
13821381
v8::PropertyDescriptor descriptor(value,
13831382
(p->attributes & napi_writable) != 0);
13841383
descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
13851384
descriptor.set_configurable((p->attributes & napi_configurable) != 0);
13861385

1387-
auto define_maybe =
1388-
obj->DefineProperty(context, property_name, descriptor);
1389-
defined_successfully = define_maybe.FromMaybe(false);
1386+
define_maybe = obj->DefineProperty(context, property_name, descriptor);
13901387
}
13911388

1392-
if (!defined_successfully) {
1393-
return napi_set_last_error(env, napi_invalid_arg);
1394-
}
1389+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1390+
env, define_maybe.FromMaybe(false), napi_invalid_arg);
13951391
}
13961392
}
13971393

@@ -1488,6 +1484,7 @@ napi_status NAPI_CDECL napi_get_prototype(napi_env env,
14881484
v8::Local<v8::Object> obj;
14891485
CHECK_TO_OBJECT(env, context, obj, object);
14901486

1487+
// This doesn't invokes Proxy's [[GetPrototypeOf]] handler.
14911488
v8::Local<v8::Value> val = obj->GetPrototype();
14921489
*result = v8impl::JsValueFromV8LocalValue(val);
14931490
return GET_RETURN_STATUS(env);
@@ -2000,15 +1997,11 @@ napi_status NAPI_CDECL napi_call_function(napi_env env,
20001997
argc,
20011998
reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv)));
20021999

2003-
if (try_catch.HasCaught()) {
2004-
return napi_set_last_error(env, napi_pending_exception);
2005-
} else {
2006-
if (result != nullptr) {
2007-
CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure);
2008-
*result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
2009-
}
2010-
return napi_clear_last_error(env);
2000+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, maybe, napi_generic_failure);
2001+
if (result != nullptr) {
2002+
*result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
20112003
}
2004+
return napi_clear_last_error(env);
20122005
}
20132006

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

‎test/js-native-api/common.c

+16-3
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,28 @@ void add_returned_status(napi_env env,
3232

3333
void add_last_status(napi_env env, const char* key, napi_value return_value) {
3434
napi_value prop_value;
35+
napi_value exception;
3536
const napi_extended_error_info* p_last_error;
3637
NODE_API_CALL_RETURN_VOID(env,
3738
napi_get_last_error_info(env, &p_last_error));
39+
// Content of p_last_error can be updated in subsequent node-api calls.
40+
const char* error_message = p_last_error->error_message == NULL ?
41+
"napi_ok" :
42+
p_last_error->error_message;
43+
44+
bool is_exception_pending;
45+
NODE_API_CALL_RETURN_VOID(env, napi_is_exception_pending(env, &is_exception_pending));
46+
if (is_exception_pending) {
47+
NODE_API_CALL_RETURN_VOID(env, napi_get_and_clear_last_exception(env, &exception));
48+
char exception_key[50];
49+
snprintf(exception_key, sizeof(exception_key), "%s%s", key, "Exception");
50+
NODE_API_CALL_RETURN_VOID(env,
51+
napi_set_named_property(env, return_value, exception_key, exception));
52+
}
3853

3954
NODE_API_CALL_RETURN_VOID(env,
4055
napi_create_string_utf8(env,
41-
(p_last_error->error_message == NULL ?
42-
"napi_ok" :
43-
p_last_error->error_message),
56+
error_message,
4457
NAPI_AUTO_LENGTH,
4558
&prop_value));
4659
NODE_API_CALL_RETURN_VOID(env,

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

+8
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,14 @@
88
"test_null.c",
99
"test_object.c"
1010
]
11+
},
12+
{
13+
"target_name": "test_exceptions",
14+
"sources": [
15+
"../common.c",
16+
"../entry_point.c",
17+
"test_exceptions.c",
18+
]
1119
}
1220
]
1321
}

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

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

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

+18
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)