Skip to content

Commit af50ac2

Browse files
author
Gabriel Schulhof
committed
error: do not replace pending exception
Only construct a `Napi::Error` from the last non-`napi_ok` error code if there is no exception pending. A consequence for the object property test suite is that it must now expect the exception thrown by the engine when N-API core attempts to convert the undefined value to an object. Fixes: #621 PR-URL: #629 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent b72f1d6 commit af50ac2

File tree

8 files changed

+55
-16
lines changed

8 files changed

+55
-16
lines changed

‎napi-inl.h‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2088,28 +2088,26 @@ inline void Buffer<T>::EnsureInfo() const {
20882088
inline Error Error::New(napi_env env) {
20892089
napi_status status;
20902090
napi_value error = nullptr;
2091-
2091+
bool is_exception_pending;
20922092
const napi_extended_error_info* info;
2093+
2094+
// We must retrieve the last error info before doing anything else, because
2095+
// doing anything else will replace the last error info.
20932096
status = napi_get_last_error_info(env, &info);
20942097
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_last_error_info");
20952098

2096-
if (info->error_code == napi_pending_exception) {
2099+
status = napi_is_exception_pending(env, &is_exception_pending);
2100+
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_is_exception_pending");
2101+
2102+
// A pending exception takes precedence over any internal error status.
2103+
if (is_exception_pending) {
20972104
status = napi_get_and_clear_last_exception(env, &error);
20982105
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_and_clear_last_exception");
20992106
}
21002107
else {
21012108
const char* error_message = info->error_message != nullptr ?
21022109
info->error_message : "Error in native callback";
21032110

2104-
bool isExceptionPending;
2105-
status = napi_is_exception_pending(env, &isExceptionPending);
2106-
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_is_exception_pending");
2107-
2108-
if (isExceptionPending) {
2109-
status = napi_get_and_clear_last_exception(env, &error);
2110-
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_and_clear_last_exception");
2111-
}
2112-
21132111
napi_value message;
21142112
status = napi_create_string_utf8(
21152113
env,

‎test/error.cc‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,40 @@ void ThrowFatalError(const CallbackInfo& /*info*/) {
158158
Error::Fatal("Error::ThrowFatalError", "This is a fatal error");
159159
}
160160

161+
void ThrowDefaultError(const CallbackInfo& info) {
162+
napi_value dummy;
163+
napi_env env = info.Env();
164+
napi_status status = napi_get_undefined(env, &dummy);
165+
NAPI_FATAL_IF_FAILED(status, "ThrowDefaultError", "napi_get_undefined");
166+
167+
if (info[0].As<Boolean>().Value()) {
168+
// Provoke N-API into setting an error, then use the `Napi::Error::New`
169+
// factory with only the `env` parameter to throw an exception generated
170+
// from the last error.
171+
uint32_t dummy_uint32;
172+
status = napi_get_value_uint32(env, dummy, &dummy_uint32);
173+
if (status == napi_ok) {
174+
Error::Fatal("ThrowDefaultError", "napi_get_value_uint32");
175+
}
176+
// We cannot use `NAPI_THROW_IF_FAILED()` here because we do not wish
177+
// control to pass back to the engine if we throw an exception here and C++
178+
// exceptions are turned on.
179+
Napi::Error::New(env).ThrowAsJavaScriptException();
180+
}
181+
182+
// Produce and throw a second error that has different content than the one
183+
// above. If the one above was thrown, then throwing the one below should
184+
// have the effect of re-throwing the one above.
185+
status = napi_get_named_property(env, dummy, "xyzzy", &dummy);
186+
if (status == napi_ok) {
187+
Error::Fatal("ThrowDefaultError", "napi_get_named_property");
188+
}
189+
190+
// The macro creates a `Napi::Error` using the factory that takes only the
191+
// env, however, it heeds the exception mechanism to be used.
192+
NAPI_THROW_IF_FAILED_VOID(env, status);
193+
}
194+
161195
} // end anonymous namespace
162196

163197
Object InitError(Env env) {
@@ -174,5 +208,6 @@ Object InitError(Env env) {
174208
exports["catchAndRethrowErrorThatEscapesScope"] =
175209
Function::New(env, CatchAndRethrowErrorThatEscapesScope);
176210
exports["throwFatalError"] = Function::New(env, ThrowFatalError);
211+
exports["throwDefaultError"] = Function::New(env, ThrowDefaultError);
177212
return exports;
178213
}

‎test/error.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,4 +70,10 @@ function test(bindingPath) {
7070
assert.ifError(p.error);
7171
assert.ok(p.stderr.toString().includes(
7272
'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
73+
74+
assert.throws(() => binding.error.throwDefaultError(false),
75+
/Cannot convert undefined or null to object/);
76+
77+
assert.throws(() => binding.error.throwDefaultError(true),
78+
/A number was expected/);
7379
}

‎test/object/delete_property.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ function test(binding) {
2222
function testShouldThrowErrorIfKeyIsInvalid(nativeDeleteProperty) {
2323
assert.throws(() => {
2424
nativeDeleteProperty(undefined, 'test');
25-
}, /object was expected/);
25+
}, /Cannot convert undefined or null to object/);
2626
}
2727

2828
testDeleteProperty(binding.object.deletePropertyWithNapiValue);

‎test/object/get_property.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ function test(binding) {
1515
function testShouldThrowErrorIfKeyIsInvalid(nativeGetProperty) {
1616
assert.throws(() => {
1717
nativeGetProperty(undefined, 'test');
18-
}, /object was expected/);
18+
}, /Cannot convert undefined or null to object/);
1919
}
2020

2121
testGetProperty(binding.object.getPropertyWithNapiValue);

‎test/object/has_own_property.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ function test(binding) {
2121
function testShouldThrowErrorIfKeyIsInvalid(nativeHasOwnProperty) {
2222
assert.throws(() => {
2323
nativeHasOwnProperty(undefined, 'test');
24-
}, /object was expected/);
24+
}, /Cannot convert undefined or null to object/);
2525
}
2626

2727
testHasOwnProperty(binding.object.hasOwnPropertyWithNapiValue);

‎test/object/has_property.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ function test(binding) {
2121
function testShouldThrowErrorIfKeyIsInvalid(nativeHasProperty) {
2222
assert.throws(() => {
2323
nativeHasProperty(undefined, 'test');
24-
}, /object was expected/);
24+
}, /Cannot convert undefined or null to object/);
2525
}
2626

2727
testHasProperty(binding.object.hasPropertyWithNapiValue);

‎test/object/set_property.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ function test(binding) {
1616
function testShouldThrowErrorIfKeyIsInvalid(nativeSetProperty) {
1717
assert.throws(() => {
1818
nativeSetProperty(undefined, 'test', 1);
19-
}, /object was expected/);
19+
}, /Cannot convert undefined or null to object/);
2020
}
2121

2222
testSetProperty(binding.object.setPropertyWithNapiValue);

0 commit comments

Comments
 (0)