Skip to content

Commit

Permalink
Fix reporting of API error messages
Browse files Browse the repository at this point in the history
When a call to an N-API function caused an error for some reason other
than a JS exception, the fallback error message "Error in native
callback" was always reported because of incorrect logic in
`Napi::Error::New()`.

Then that fix exposed a bug in `napi_get_last_error_info()`, which I
have fixed here and also at nodejs/node#13087
  • Loading branch information
wroy7860 committed Jun 6, 2017
1 parent 21501c3 commit 4b4b708
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 14 deletions.
20 changes: 10 additions & 10 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1355,17 +1355,17 @@ inline void Buffer<T>::EnsureInfo() const {
inline Error Error::New(napi_env env) {
napi_status status;
napi_value error = nullptr;
if (Napi::Env(env).IsExceptionPending()) {
status = napi_get_and_clear_last_exception(env, &error);
assert(status == napi_ok);
}
else {
// No JS exception is pending, so check for NAPI error info.
const napi_extended_error_info* info;
status = napi_get_last_error_info(env, &info);
assert(status == napi_ok);

if (status == napi_ok) {
const napi_extended_error_info* info;
status = napi_get_last_error_info(env, &info);
assert(status == napi_ok);

if (status == napi_ok) {
if (info->error_code == napi_pending_exception) {
status = napi_get_and_clear_last_exception(env, &error);
assert(status == napi_ok);
}
else {
const char* error_message = info->error_message != nullptr ?
info->error_message : "Error in native callback";
napi_value message;
Expand Down
2 changes: 1 addition & 1 deletion src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ napi_status napi_get_last_error_info(napi_env env,
error_messages[env->last_error.error_code];

*result = &(env->last_error);
return napi_clear_last_error(env);
return napi_ok;
}

napi_status napi_create_function(napi_env env,
Expand Down
10 changes: 8 additions & 2 deletions test/error.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ using namespace Napi;

namespace {

void ThrowError(const CallbackInfo& info) {
void ThrowApiError(const CallbackInfo& info) {
// Attempting to call an empty function value will throw an API error.
Function(info.Env(), nullptr).Call({});
}

void ThrowJSError(const CallbackInfo& info) {
std::string message = info[0].As<String>().Utf8Value();
throw Error::New(info.Env(), message);
}
Expand Down Expand Up @@ -76,7 +81,8 @@ void CatchAndRethrowErrorThatEscapesScope(const CallbackInfo& info) {

Object InitError(Env env) {
Object exports = Object::New(env);
exports["throwError"] = Function::New(env, ThrowError);
exports["throwApiError"] = Function::New(env, ThrowApiError);
exports["throwJSError"] = Function::New(env, ThrowJSError);
exports["throwTypeError"] = Function::New(env, ThrowTypeError);
exports["throwRangeError"] = Function::New(env, ThrowRangeError);
exports["catchError"] = Function::New(env, CatchError);
Expand Down
6 changes: 5 additions & 1 deletion test/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ const buildType = process.config.target_defaults.default_configuration;
const binding = require(`./build/${buildType}/binding.node`);
const assert = require('assert');

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

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

Expand Down

0 comments on commit 4b4b708

Please sign in to comment.