Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node-api: correctly return pending exception status on napi_new_instance #38798

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2660,7 +2660,7 @@ napi_status napi_new_instance(napi_env env,
auto maybe = ctor->NewInstance(context, argc,
reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv)));

CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure);
CHECK_MAYBE_EMPTY(env, maybe, napi_pending_exception);

*result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
return GET_RETURN_STATUS(env);
Expand Down
47 changes: 47 additions & 0 deletions test/js-native-api/test_exception/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,34 @@ const test_exception = (function() {
` thrown, but ${returnedError} was passed`);
}


{
const throwTheError = class { constructor() { throw theError; } };

// Test that the native side successfully captures the exception
let returnedError = test_exception.constructReturnException(throwTheError);
assert.strictEqual(returnedError, theError);

// Test that the native side passes the exception through
assert.throws(
() => { test_exception.constructAllowException(throwTheError); },
(err) => err === theError
);

// Test that the exception thrown above was marked as pending
// before it was handled on the JS side
const exception_pending = test_exception.wasPending();
assert.strictEqual(exception_pending, true,
'Exception not pending as expected,' +
` .wasPending() returned ${exception_pending}`);

// Test that the native side does not capture a non-existing exception
returnedError = test_exception.constructReturnException(common.mustCall());
assert.strictEqual(returnedError, undefined,
'Returned error should be undefined when no exception is' +
` thrown, but ${returnedError} was passed`);
}

{
// Test that no exception appears that was not thrown by us
let caughtError;
Expand All @@ -66,3 +94,22 @@ const test_exception = (function() {
'Exception state did not remain clear as expected,' +
` .wasPending() returned ${exception_pending}`);
}

{
// Test that no exception appears that was not thrown by us
let caughtError;
try {
test_exception.constructAllowException(common.mustCall());
} catch (anError) {
caughtError = anError;
}
assert.strictEqual(caughtError, undefined,
'No exception originated on the native side, but' +
` ${caughtError} was passed`);

// Test that the exception state remains clear when no exception is thrown
const exception_pending = test_exception.wasPending();
assert.strictEqual(exception_pending, false,
'Exception state did not remain clear as expected,' +
` .wasPending() returned ${exception_pending}`);
}
31 changes: 31 additions & 0 deletions test/js-native-api/test_exception/test_exception.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,22 @@ static napi_value returnException(napi_env env, napi_callback_info info) {
return NULL;
}

static napi_value constructReturnException(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 result;
napi_status status = napi_new_instance(env, args[0], 0, 0, &result);
if (status == napi_pending_exception) {
napi_value ex;
NODE_API_CALL(env, napi_get_and_clear_last_exception(env, &ex));
return ex;
}

return NULL;
}

static napi_value allowException(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
Expand All @@ -38,6 +54,19 @@ static napi_value allowException(napi_env env, napi_callback_info info) {
return NULL;
}

static napi_value constructAllowException(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 result;
napi_new_instance(env, args[0], 0, 0, &result);
// Ignore status and check napi_is_exception_pending() instead.

Comment on lines +64 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment looks a bit misplaced. Am I correct?

Suggested change
// Ignore status and check napi_is_exception_pending() instead.
// Ignore status and check napi_is_exception_pending() instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, maybe not. The comment is actually for the line above, which means that the omitting of the checks on the returning status is intentional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Perhaps it would be better to put the comment above the call (and the variable declaration)? If this is a convention in the codebase no problem then.

NODE_API_CALL(env, napi_is_exception_pending(env, &exceptionWasPending));
return NULL;
}

static napi_value wasPending(napi_env env, napi_callback_info info) {
napi_value result;
NODE_API_CALL(env, napi_get_boolean(env, exceptionWasPending, &result));
Expand All @@ -64,6 +93,8 @@ napi_value Init(napi_env env, napi_value exports) {
napi_property_descriptor descriptors[] = {
DECLARE_NODE_API_PROPERTY("returnException", returnException),
DECLARE_NODE_API_PROPERTY("allowException", allowException),
DECLARE_NODE_API_PROPERTY("constructReturnException", constructReturnException),
DECLARE_NODE_API_PROPERTY("constructAllowException", constructAllowException),
DECLARE_NODE_API_PROPERTY("wasPending", wasPending),
DECLARE_NODE_API_PROPERTY("createExternal", createExternal),
};
Expand Down