Skip to content

Commit b82e257

Browse files
author
John French
committed
src: fix usage of napi_extended_error_info in Error::New()
All fields of the `napi_extended_error_info` structure gets reset in subsequent Node-API function calls on the same `env`. This includes a call to `napi_is_exception_pending()`. So here it is necessary to make a copy of the information as the `error_code` field is used later on. Signed-off-by: Darshan Sen <[email protected]> PR-URL: nodejs/node-addon-api#1092 Fixes: nodejs/node-addon-api#1089 Reviewed-By: Michael Dawson <[email protected]>
1 parent 627db5e commit b82e257

File tree

4 files changed

+61
-34
lines changed

4 files changed

+61
-34
lines changed

‎napi-inl.h

+29-17
Original file line numberDiff line numberDiff line change
@@ -2520,12 +2520,23 @@ inline Error Error::New(napi_env env) {
25202520
napi_status status;
25212521
napi_value error = nullptr;
25222522
bool is_exception_pending;
2523-
const napi_extended_error_info* info;
2523+
napi_extended_error_info last_error_info_copy;
25242524

2525-
// We must retrieve the last error info before doing anything else, because
2526-
// doing anything else will replace the last error info.
2527-
status = napi_get_last_error_info(env, &info);
2528-
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_last_error_info");
2525+
{
2526+
// We must retrieve the last error info before doing anything else because
2527+
// doing anything else will replace the last error info.
2528+
const napi_extended_error_info* last_error_info;
2529+
status = napi_get_last_error_info(env, &last_error_info);
2530+
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_last_error_info");
2531+
2532+
// All fields of the `napi_extended_error_info` structure gets reset in
2533+
// subsequent Node-API function calls on the same `env`. This includes a
2534+
// call to `napi_is_exception_pending()`. So here it is necessary to make a
2535+
// copy of the information as the `error_code` field is used later on.
2536+
memcpy(&last_error_info_copy,
2537+
last_error_info,
2538+
sizeof(napi_extended_error_info));
2539+
}
25292540

25302541
status = napi_is_exception_pending(env, &is_exception_pending);
25312542
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_is_exception_pending");
@@ -2536,8 +2547,9 @@ inline Error Error::New(napi_env env) {
25362547
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_and_clear_last_exception");
25372548
}
25382549
else {
2539-
const char* error_message = info->error_message != nullptr ?
2540-
info->error_message : "Error in native callback";
2550+
const char* error_message = last_error_info_copy.error_message != nullptr
2551+
? last_error_info_copy.error_message
2552+
: "Error in native callback";
25412553

25422554
napi_value message;
25432555
status = napi_create_string_utf8(
@@ -2547,16 +2559,16 @@ inline Error Error::New(napi_env env) {
25472559
&message);
25482560
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_create_string_utf8");
25492561

2550-
switch (info->error_code) {
2551-
case napi_object_expected:
2552-
case napi_string_expected:
2553-
case napi_boolean_expected:
2554-
case napi_number_expected:
2555-
status = napi_create_type_error(env, nullptr, message, &error);
2556-
break;
2557-
default:
2558-
status = napi_create_error(env, nullptr, message, &error);
2559-
break;
2562+
switch (last_error_info_copy.error_code) {
2563+
case napi_object_expected:
2564+
case napi_string_expected:
2565+
case napi_boolean_expected:
2566+
case napi_number_expected:
2567+
status = napi_create_type_error(env, nullptr, message, &error);
2568+
break;
2569+
default:
2570+
status = napi_create_error(env, nullptr, message, &error);
2571+
break;
25602572
}
25612573
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_create_error");
25622574
}

‎test/error.cc

+12
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,16 @@ void ThrowApiError(const CallbackInfo& info) {
5959
Function(info.Env(), nullptr).Call(std::initializer_list<napi_value>{});
6060
}
6161

62+
void LastExceptionErrorCode(const CallbackInfo& info) {
63+
// Previously, `napi_extended_error_info.error_code` got reset to `napi_ok` in
64+
// subsequent Node-API function calls, so this would have previously thrown an
65+
// `Error` object instead of a `TypeError` object.
66+
Env env = info.Env();
67+
bool res;
68+
napi_get_value_bool(env, Value::From(env, "asd"), &res);
69+
NAPI_THROW_VOID(Error::New(env));
70+
}
71+
6272
#ifdef NAPI_CPP_EXCEPTIONS
6373

6474
void ThrowJSError(const CallbackInfo& info) {
@@ -256,6 +266,8 @@ void ThrowDefaultError(const CallbackInfo& info) {
256266
Object InitError(Env env) {
257267
Object exports = Object::New(env);
258268
exports["throwApiError"] = Function::New(env, ThrowApiError);
269+
exports["lastExceptionErrorCode"] =
270+
Function::New(env, LastExceptionErrorCode);
259271
exports["throwJSError"] = Function::New(env, ThrowJSError);
260272
exports["throwTypeError"] = Function::New(env, ThrowTypeError);
261273
exports["throwRangeError"] = Function::New(env, ThrowRangeError);

‎test/error.js

+15-12
Original file line numberDiff line numberDiff line change
@@ -5,27 +5,30 @@ const assert = require('assert');
55
if (process.argv[2] === 'fatal') {
66
const binding = require(process.argv[3]);
77
binding.error.throwFatalError();
8-
return;
98
}
109

1110
module.exports = require('./common').runTestWithBindingPath(test);
1211

13-
function test(bindingPath) {
12+
function test (bindingPath) {
1413
const binding = require(bindingPath);
1514

16-
assert.throws(() => binding.error.throwApiError('test'), function(err) {
15+
assert.throws(() => binding.error.throwApiError('test'), function (err) {
1716
return err instanceof Error && err.message.includes('Invalid');
1817
});
1918

20-
assert.throws(() => binding.error.throwJSError('test'), function(err) {
19+
assert.throws(() => binding.error.lastExceptionErrorCode(), function (err) {
20+
return err instanceof TypeError && err.message === 'A boolean was expected';
21+
});
22+
23+
assert.throws(() => binding.error.throwJSError('test'), function (err) {
2124
return err instanceof Error && err.message === 'test';
2225
});
2326

24-
assert.throws(() => binding.error.throwTypeError('test'), function(err) {
27+
assert.throws(() => binding.error.throwTypeError('test'), function (err) {
2528
return err instanceof TypeError && err.message === 'test';
2629
});
2730

28-
assert.throws(() => binding.error.throwRangeError('test'), function(err) {
31+
assert.throws(() => binding.error.throwRangeError('test'), function (err) {
2932
return err instanceof RangeError && err.message === 'test';
3033
});
3134

@@ -34,7 +37,7 @@ function test(bindingPath) {
3437
() => {
3538
throw new TypeError('test');
3639
}),
37-
function(err) {
40+
function (err) {
3841
return err instanceof TypeError && err.message === 'test' && !err.caught;
3942
});
4043

@@ -43,7 +46,7 @@ function test(bindingPath) {
4346
() => {
4447
throw new TypeError('test');
4548
}),
46-
function(err) {
49+
function (err) {
4750
return err instanceof TypeError && err.message === 'test' && err.caught;
4851
});
4952

@@ -56,19 +59,19 @@ function test(bindingPath) {
5659
() => { throw new TypeError('test'); });
5760
assert.strictEqual(msg, 'test');
5861

59-
assert.throws(() => binding.error.throwErrorThatEscapesScope('test'), function(err) {
62+
assert.throws(() => binding.error.throwErrorThatEscapesScope('test'), function (err) {
6063
return err instanceof Error && err.message === 'test';
6164
});
6265

63-
assert.throws(() => binding.error.catchAndRethrowErrorThatEscapesScope('test'), function(err) {
66+
assert.throws(() => binding.error.catchAndRethrowErrorThatEscapesScope('test'), function (err) {
6467
return err instanceof Error && err.message === 'test' && err.caught;
6568
});
6669

6770
const p = require('./napi_child').spawnSync(
68-
process.execPath, [ __filename, 'fatal', bindingPath ]);
71+
process.execPath, [__filename, 'fatal', bindingPath]);
6972
assert.ifError(p.error);
7073
assert.ok(p.stderr.toString().includes(
71-
'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
74+
'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
7275

7376
assert.throws(() => binding.error.throwDefaultError(false),
7477
/Cannot convert undefined or null to object/);

‎test/run_script.js

+5-5
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ const testUtil = require('./testUtil');
55

66
module.exports = require('./common').runTest(test);
77

8-
function test(binding) {
8+
function test (binding) {
99
return testUtil.runGCTests([
1010
'Plain C string',
1111
() => {
@@ -21,7 +21,7 @@ function test(binding) {
2121

2222
'JavaScript string',
2323
() => {
24-
const sum = binding.run_script.jsString("1 + 2 + 3");
24+
const sum = binding.run_script.jsString('1 + 2 + 3');
2525
assert.strictEqual(sum, 1 + 2 + 3);
2626
},
2727

@@ -30,15 +30,15 @@ function test(binding) {
3030
assert.throws(() => {
3131
binding.run_script.jsString(true);
3232
}, {
33-
name: 'Error',
33+
name: 'TypeError',
3434
message: 'A string was expected'
3535
});
3636
},
3737

3838
'With context',
3939
() => {
40-
const a = 1, b = 2, c = 3;
41-
const sum = binding.run_script.withContext("a + b + c", { a, b, c });
40+
const a = 1; const b = 2; const c = 3;
41+
const sum = binding.run_script.withContext('a + b + c', { a, b, c });
4242
assert.strictEqual(sum, a + b + c);
4343
}
4444
]);

0 commit comments

Comments
 (0)