Skip to content

Commit

Permalink
src: fix usage of napi_extended_error_info in Error::New()
Browse files Browse the repository at this point in the history 
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 <darshan.sen@postman.com>

PR-URL: nodejs/node-addon-api#1092
Fixes: nodejs/node-addon-api#1089
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
  • Loading branch information
John French committed Oct 21, 2021
1 parent 627db5e commit b82e257
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 34 deletions.
46 changes: 29 additions & 17 deletions napi-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2520,12 +2520,23 @@ inline Error Error::New(napi_env env) {
napi_status status;
napi_value error = nullptr;
bool is_exception_pending;
const napi_extended_error_info* info;
napi_extended_error_info last_error_info_copy;

// We must retrieve the last error info before doing anything else, because
// doing anything else will replace the last error info.
status = napi_get_last_error_info(env, &info);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_last_error_info");
{
// We must retrieve the last error info before doing anything else because
// doing anything else will replace the last error info.
const napi_extended_error_info* last_error_info;
status = napi_get_last_error_info(env, &last_error_info);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_last_error_info");

// 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.
memcpy(&last_error_info_copy,
last_error_info,
sizeof(napi_extended_error_info));
}

status = napi_is_exception_pending(env, &is_exception_pending);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_is_exception_pending");
Expand All @@ -2536,8 +2547,9 @@ inline Error Error::New(napi_env env) {
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_and_clear_last_exception");
}
else {
const char* error_message = info->error_message != nullptr ?
info->error_message : "Error in native callback";
const char* error_message = last_error_info_copy.error_message != nullptr
? last_error_info_copy.error_message
: "Error in native callback";

napi_value message;
status = napi_create_string_utf8(
Expand All @@ -2547,16 +2559,16 @@ inline Error Error::New(napi_env env) {
&message);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_create_string_utf8");

switch (info->error_code) {
case napi_object_expected:
case napi_string_expected:
case napi_boolean_expected:
case napi_number_expected:
status = napi_create_type_error(env, nullptr, message, &error);
break;
default:
status = napi_create_error(env, nullptr, message, &error);
break;
switch (last_error_info_copy.error_code) {
case napi_object_expected:
case napi_string_expected:
case napi_boolean_expected:
case napi_number_expected:
status = napi_create_type_error(env, nullptr, message, &error);
break;
default:
status = napi_create_error(env, nullptr, message, &error);
break;
}
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_create_error");
}
Expand Down
12 changes: 12 additions & 0 deletions test/error.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@ void ThrowApiError(const CallbackInfo& info) {
Function(info.Env(), nullptr).Call(std::initializer_list<napi_value>{});
}

void LastExceptionErrorCode(const CallbackInfo& info) {
// Previously, `napi_extended_error_info.error_code` got reset to `napi_ok` in
// subsequent Node-API function calls, so this would have previously thrown an
// `Error` object instead of a `TypeError` object.
Env env = info.Env();
bool res;
napi_get_value_bool(env, Value::From(env, "asd"), &res);
NAPI_THROW_VOID(Error::New(env));
}

#ifdef NAPI_CPP_EXCEPTIONS

void ThrowJSError(const CallbackInfo& info) {
Expand Down Expand Up @@ -256,6 +266,8 @@ void ThrowDefaultError(const CallbackInfo& info) {
Object InitError(Env env) {
Object exports = Object::New(env);
exports["throwApiError"] = Function::New(env, ThrowApiError);
exports["lastExceptionErrorCode"] =
Function::New(env, LastExceptionErrorCode);
exports["throwJSError"] = Function::New(env, ThrowJSError);
exports["throwTypeError"] = Function::New(env, ThrowTypeError);
exports["throwRangeError"] = Function::New(env, ThrowRangeError);
Expand Down
27 changes: 15 additions & 12 deletions test/error.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,30 @@ const assert = require('assert');
if (process.argv[2] === 'fatal') {
const binding = require(process.argv[3]);
binding.error.throwFatalError();
return;
}

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

function test(bindingPath) {
function test (bindingPath) {
const binding = require(bindingPath);

assert.throws(() => binding.error.throwApiError('test'), function(err) {
assert.throws(() => binding.error.throwApiError('test'), function (err) {
return err instanceof Error && err.message.includes('Invalid');
});

assert.throws(() => binding.error.throwJSError('test'), function(err) {
assert.throws(() => binding.error.lastExceptionErrorCode(), function (err) {
return err instanceof TypeError && err.message === 'A boolean was expected';
});

assert.throws(() => binding.error.throwJSError('test'), function (err) {
return err instanceof Error && err.message === 'test';
});

assert.throws(() => binding.error.throwTypeError('test'), function(err) {
assert.throws(() => binding.error.throwTypeError('test'), function (err) {
return err instanceof TypeError && err.message === 'test';
});

assert.throws(() => binding.error.throwRangeError('test'), function(err) {
assert.throws(() => binding.error.throwRangeError('test'), function (err) {
return err instanceof RangeError && err.message === 'test';
});

Expand All @@ -34,7 +37,7 @@ function test(bindingPath) {
() => {
throw new TypeError('test');
}),
function(err) {
function (err) {
return err instanceof TypeError && err.message === 'test' && !err.caught;
});

Expand All @@ -43,7 +46,7 @@ function test(bindingPath) {
() => {
throw new TypeError('test');
}),
function(err) {
function (err) {
return err instanceof TypeError && err.message === 'test' && err.caught;
});

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

assert.throws(() => binding.error.throwErrorThatEscapesScope('test'), function(err) {
assert.throws(() => binding.error.throwErrorThatEscapesScope('test'), function (err) {
return err instanceof Error && err.message === 'test';
});

assert.throws(() => binding.error.catchAndRethrowErrorThatEscapesScope('test'), function(err) {
assert.throws(() => binding.error.catchAndRethrowErrorThatEscapesScope('test'), function (err) {
return err instanceof Error && err.message === 'test' && err.caught;
});

const p = require('./napi_child').spawnSync(
process.execPath, [ __filename, 'fatal', bindingPath ]);
process.execPath, [__filename, 'fatal', bindingPath]);
assert.ifError(p.error);
assert.ok(p.stderr.toString().includes(
'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));

assert.throws(() => binding.error.throwDefaultError(false),
/Cannot convert undefined or null to object/);
Expand Down
10 changes: 5 additions & 5 deletions test/run_script.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const testUtil = require('./testUtil');

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

function test(binding) {
function test (binding) {
return testUtil.runGCTests([
'Plain C string',
() => {
Expand All @@ -21,7 +21,7 @@ function test(binding) {

'JavaScript string',
() => {
const sum = binding.run_script.jsString("1 + 2 + 3");
const sum = binding.run_script.jsString('1 + 2 + 3');
assert.strictEqual(sum, 1 + 2 + 3);
},

Expand All @@ -30,15 +30,15 @@ function test(binding) {
assert.throws(() => {
binding.run_script.jsString(true);
}, {
name: 'Error',
name: 'TypeError',
message: 'A string was expected'
});
},

'With context',
() => {
const a = 1, b = 2, c = 3;
const sum = binding.run_script.withContext("a + b + c", { a, b, c });
const a = 1; const b = 2; const c = 3;
const sum = binding.run_script.withContext('a + b + c', { a, b, c });
assert.strictEqual(sum, a + b + c);
}
]);
Expand Down

0 comments on commit b82e257

Please sign in to comment.