Skip to content

Commit 48095ca

Browse files
legendecasmarco-ippolito
legendecas
authored and
marco-ippolito
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. PR-URL: #48607 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
1 parent 9fda8e2 commit 48095ca

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
@@ -1161,7 +1161,8 @@ napi_status NAPI_CDECL napi_set_property(napi_env env,
11611161

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

1164-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1164+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1165+
env, set_maybe.FromMaybe(false), napi_generic_failure);
11651166
return GET_RETURN_STATUS(env);
11661167
}
11671168

@@ -1181,7 +1182,7 @@ napi_status NAPI_CDECL napi_has_property(napi_env env,
11811182
v8::Local<v8::Value> k = v8impl::V8LocalValueFromJsValue(key);
11821183
v8::Maybe<bool> has_maybe = obj->Has(context, k);
11831184

1184-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1185+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
11851186

11861187
*result = has_maybe.FromMaybe(false);
11871188
return GET_RETURN_STATUS(env);
@@ -1203,7 +1204,7 @@ napi_status NAPI_CDECL napi_get_property(napi_env env,
12031204

12041205
auto get_maybe = obj->Get(context, k);
12051206

1206-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1207+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
12071208

12081209
v8::Local<v8::Value> val = get_maybe.ToLocalChecked();
12091210
*result = v8impl::JsValueFromV8LocalValue(val);
@@ -1223,7 +1224,7 @@ napi_status NAPI_CDECL napi_delete_property(napi_env env,
12231224

12241225
CHECK_TO_OBJECT(env, context, obj, object);
12251226
v8::Maybe<bool> delete_maybe = obj->Delete(context, k);
1226-
CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure);
1227+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, delete_maybe, napi_generic_failure);
12271228

12281229
if (result != nullptr) *result = delete_maybe.FromMaybe(false);
12291230

@@ -1245,7 +1246,7 @@ napi_status NAPI_CDECL napi_has_own_property(napi_env env,
12451246
v8::Local<v8::Value> k = v8impl::V8LocalValueFromJsValue(key);
12461247
RETURN_STATUS_IF_FALSE(env, k->IsName(), napi_name_expected);
12471248
v8::Maybe<bool> has_maybe = obj->HasOwnProperty(context, k.As<v8::Name>());
1248-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1249+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
12491250
*result = has_maybe.FromMaybe(false);
12501251

12511252
return GET_RETURN_STATUS(env);
@@ -1270,7 +1271,8 @@ napi_status NAPI_CDECL napi_set_named_property(napi_env env,
12701271

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

1273-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1274+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1275+
env, set_maybe.FromMaybe(false), napi_generic_failure);
12741276
return GET_RETURN_STATUS(env);
12751277
}
12761278

@@ -1291,7 +1293,7 @@ napi_status NAPI_CDECL napi_has_named_property(napi_env env,
12911293

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

1294-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1296+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
12951297

12961298
*result = has_maybe.FromMaybe(false);
12971299
return GET_RETURN_STATUS(env);
@@ -1315,7 +1317,7 @@ napi_status NAPI_CDECL napi_get_named_property(napi_env env,
13151317

13161318
auto get_maybe = obj->Get(context, key);
13171319

1318-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1320+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
13191321

13201322
v8::Local<v8::Value> val = get_maybe.ToLocalChecked();
13211323
*result = v8impl::JsValueFromV8LocalValue(val);
@@ -1337,7 +1339,8 @@ napi_status NAPI_CDECL napi_set_element(napi_env env,
13371339
v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
13381340
auto set_maybe = obj->Set(context, index, val);
13391341

1340-
RETURN_STATUS_IF_FALSE(env, set_maybe.FromMaybe(false), napi_generic_failure);
1342+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1343+
env, set_maybe.FromMaybe(false), napi_generic_failure);
13411344

13421345
return GET_RETURN_STATUS(env);
13431346
}
@@ -1356,7 +1359,7 @@ napi_status NAPI_CDECL napi_has_element(napi_env env,
13561359

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

1359-
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
1362+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, has_maybe, napi_generic_failure);
13601363

13611364
*result = has_maybe.FromMaybe(false);
13621365
return GET_RETURN_STATUS(env);
@@ -1376,7 +1379,7 @@ napi_status NAPI_CDECL napi_get_element(napi_env env,
13761379

13771380
auto get_maybe = obj->Get(context, index);
13781381

1379-
CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);
1382+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, get_maybe, napi_generic_failure);
13801383

13811384
*result = v8impl::JsValueFromV8LocalValue(get_maybe.ToLocalChecked());
13821385
return GET_RETURN_STATUS(env);
@@ -1393,7 +1396,7 @@ napi_status NAPI_CDECL napi_delete_element(napi_env env,
13931396

13941397
CHECK_TO_OBJECT(env, context, obj, object);
13951398
v8::Maybe<bool> delete_maybe = obj->Delete(context, index);
1396-
CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure);
1399+
CHECK_MAYBE_NOTHING_WITH_PREAMBLE(env, delete_maybe, napi_generic_failure);
13971400

13981401
if (result != nullptr) *result = delete_maybe.FromMaybe(false);
13991402

@@ -1441,9 +1444,8 @@ napi_define_properties(napi_env env,
14411444
auto define_maybe =
14421445
obj->DefineProperty(context, property_name, descriptor);
14431446

1444-
if (!define_maybe.FromMaybe(false)) {
1445-
return napi_set_last_error(env, napi_invalid_arg);
1446-
}
1447+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1448+
env, define_maybe.FromMaybe(false), napi_invalid_arg);
14471449
} else if (p->method != nullptr) {
14481450
v8::Local<v8::Function> method;
14491451
STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction(
@@ -1456,34 +1458,28 @@ napi_define_properties(napi_env env,
14561458
auto define_maybe =
14571459
obj->DefineProperty(context, property_name, descriptor);
14581460

1459-
if (!define_maybe.FromMaybe(false)) {
1460-
return napi_set_last_error(env, napi_generic_failure);
1461-
}
1461+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1462+
env, define_maybe.FromMaybe(false), napi_generic_failure);
14621463
} else {
14631464
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
1464-
bool defined_successfully = false;
1465+
v8::Maybe<bool> define_maybe = v8::Just(false);
14651466

14661467
if ((p->attributes & napi_enumerable) &&
14671468
(p->attributes & napi_writable) &&
14681469
(p->attributes & napi_configurable)) {
14691470
// Use a fast path for this type of data property.
1470-
auto define_maybe =
1471-
obj->CreateDataProperty(context, property_name, value);
1472-
defined_successfully = define_maybe.FromMaybe(false);
1471+
define_maybe = obj->CreateDataProperty(context, property_name, value);
14731472
} else {
14741473
v8::PropertyDescriptor descriptor(value,
14751474
(p->attributes & napi_writable) != 0);
14761475
descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
14771476
descriptor.set_configurable((p->attributes & napi_configurable) != 0);
14781477

1479-
auto define_maybe =
1480-
obj->DefineProperty(context, property_name, descriptor);
1481-
defined_successfully = define_maybe.FromMaybe(false);
1478+
define_maybe = obj->DefineProperty(context, property_name, descriptor);
14821479
}
14831480

1484-
if (!defined_successfully) {
1485-
return napi_set_last_error(env, napi_invalid_arg);
1486-
}
1481+
RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(
1482+
env, define_maybe.FromMaybe(false), napi_invalid_arg);
14871483
}
14881484
}
14891485

@@ -1580,6 +1576,7 @@ napi_status NAPI_CDECL napi_get_prototype(napi_env env,
15801576
v8::Local<v8::Object> obj;
15811577
CHECK_TO_OBJECT(env, context, obj, object);
15821578

1579+
// This doesn't invokes Proxy's [[GetPrototypeOf]] handler.
15831580
v8::Local<v8::Value> val = obj->GetPrototype();
15841581
*result = v8impl::JsValueFromV8LocalValue(val);
15851582
return GET_RETURN_STATUS(env);
@@ -2132,15 +2129,11 @@ napi_status NAPI_CDECL napi_call_function(napi_env env,
21322129
argc,
21332130
reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv)));
21342131

2135-
if (try_catch.HasCaught()) {
2136-
return napi_set_last_error(env, napi_pending_exception);
2137-
} else {
2138-
if (result != nullptr) {
2139-
CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure);
2140-
*result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
2141-
}
2142-
return napi_clear_last_error(env);
2132+
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, maybe, napi_generic_failure);
2133+
if (result != nullptr) {
2134+
*result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
21432135
}
2136+
return napi_clear_last_error(env);
21442137
}
21452138

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