Fix reporting of API error messages

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

napi-inl.h

@@ -1373,17 +1373,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;

src/node_api.cc

@@ -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,

test/error.cc

@@ -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);
 }
@@ -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);

test/error.js

@@ -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';
 });