Skip to content

Commit 4b74fa5

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 48fcb20 commit 4b74fa5

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
@@ -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-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)