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: nodejs/node-addon-api#621 PR-URL: nodejs/node-addon-api#629 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
kevindavies8 · Dec 17, 2019 · a46a427 · a46a427
1 parent 409a0f9
commit a46a427
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 16 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -2088,28 +2088,26 @@ inline void Buffer<T>::EnsureInfo() const {
 inline Error Error::New(napi_env env) {
   napi_status status;
   napi_value error = nullptr;
-
+  bool is_exception_pending;
   const napi_extended_error_info* info;
+
+  // 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");
 
-  if (info->error_code == napi_pending_exception) {
+  status = napi_is_exception_pending(env, &is_exception_pending);
+  NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_is_exception_pending");
+
+  // A pending exception takes precedence over any internal error status.
+  if (is_exception_pending) {
     status = napi_get_and_clear_last_exception(env, &error);
     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";
 
-    bool isExceptionPending;
-    status = napi_is_exception_pending(env, &isExceptionPending);
-    NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_is_exception_pending");
-
-    if (isExceptionPending) {
-      status = napi_get_and_clear_last_exception(env, &error);
-      NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_and_clear_last_exception");
-    }
-
     napi_value message;
     status = napi_create_string_utf8(
       env,

diff --git a/test/error.cc b/test/error.cc
@@ -158,6 +158,40 @@ void ThrowFatalError(const CallbackInfo& /*info*/) {
   Error::Fatal("Error::ThrowFatalError", "This is a fatal error");
 }
 
+void ThrowDefaultError(const CallbackInfo& info) {
+  napi_value dummy;
+  napi_env env = info.Env();
+  napi_status status = napi_get_undefined(env, &dummy);
+  NAPI_FATAL_IF_FAILED(status, "ThrowDefaultError", "napi_get_undefined");
+
+  if (info[0].As<Boolean>().Value()) {
+    // Provoke N-API into setting an error, then use the `Napi::Error::New`
+    // factory with only the `env` parameter to throw an exception generated
+    // from the last error.
+    uint32_t dummy_uint32;
+    status = napi_get_value_uint32(env, dummy, &dummy_uint32);
+    if (status == napi_ok) {
+      Error::Fatal("ThrowDefaultError", "napi_get_value_uint32");
+    }
+    // We cannot use `NAPI_THROW_IF_FAILED()` here because we do not wish
+    // control to pass back to the engine if we throw an exception here and C++
+    // exceptions are turned on.
+    Napi::Error::New(env).ThrowAsJavaScriptException();
+  }
+
+  // Produce and throw a second error that has different content than the one
+  // above. If the one above was thrown, then throwing the one below should
+  // have the effect of re-throwing the one above.
+  status = napi_get_named_property(env, dummy, "xyzzy", &dummy);
+  if (status == napi_ok) {
+    Error::Fatal("ThrowDefaultError", "napi_get_named_property");
+  }
+
+  // The macro creates a `Napi::Error` using the factory that takes only the
+  // env, however, it heeds the exception mechanism to be used.
+  NAPI_THROW_IF_FAILED_VOID(env, status);
+}
+
 } // end anonymous namespace
 
 Object InitError(Env env) {
@@ -174,5 +208,6 @@ Object InitError(Env env) {
   exports["catchAndRethrowErrorThatEscapesScope"] =
     Function::New(env, CatchAndRethrowErrorThatEscapesScope);
   exports["throwFatalError"] = Function::New(env, ThrowFatalError);
+  exports["throwDefaultError"] = Function::New(env, ThrowDefaultError);
   return exports;
 }
diff --git a/test/error.js b/test/error.js
@@ -70,4 +70,10 @@ function test(bindingPath) {
   assert.ifError(p.error);
   assert.ok(p.stderr.toString().includes(
       'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
+
+  assert.throws(() => binding.error.throwDefaultError(false),
+    /Cannot convert undefined or null to object/);
+
+  assert.throws(() => binding.error.throwDefaultError(true),
+    /A number was expected/);
 }
diff --git a/test/object/delete_property.js b/test/object/delete_property.js
@@ -22,7 +22,7 @@ function test(binding) {
   function testShouldThrowErrorIfKeyIsInvalid(nativeDeleteProperty) {
     assert.throws(() => {
       nativeDeleteProperty(undefined, 'test');
-    }, /object was expected/);
+    }, /Cannot convert undefined or null to object/);
   }
 
   testDeleteProperty(binding.object.deletePropertyWithNapiValue);

diff --git a/test/object/get_property.js b/test/object/get_property.js
@@ -15,7 +15,7 @@ function test(binding) {
   function testShouldThrowErrorIfKeyIsInvalid(nativeGetProperty) {
     assert.throws(() => {
       nativeGetProperty(undefined, 'test');
-    }, /object was expected/);
+    }, /Cannot convert undefined or null to object/);
   }
 
   testGetProperty(binding.object.getPropertyWithNapiValue);

diff --git a/test/object/has_own_property.js b/test/object/has_own_property.js
@@ -21,7 +21,7 @@ function test(binding) {
   function testShouldThrowErrorIfKeyIsInvalid(nativeHasOwnProperty) {
     assert.throws(() => {
       nativeHasOwnProperty(undefined, 'test');
-    }, /object was expected/);
+    }, /Cannot convert undefined or null to object/);
   }
 
   testHasOwnProperty(binding.object.hasOwnPropertyWithNapiValue);

diff --git a/test/object/has_property.js b/test/object/has_property.js
@@ -21,7 +21,7 @@ function test(binding) {
   function testShouldThrowErrorIfKeyIsInvalid(nativeHasProperty) {
     assert.throws(() => {
       nativeHasProperty(undefined, 'test');
-    }, /object was expected/);
+    }, /Cannot convert undefined or null to object/);
   }
 
   testHasProperty(binding.object.hasPropertyWithNapiValue);

diff --git a/test/object/set_property.js b/test/object/set_property.js
@@ -16,7 +16,7 @@ function test(binding) {
   function testShouldThrowErrorIfKeyIsInvalid(nativeSetProperty) {
     assert.throws(() => {
       nativeSetProperty(undefined, 'test', 1);
-    }, /object was expected/);
+    }, /Cannot convert undefined or null to object/);
   }
 
   testSetProperty(binding.object.setPropertyWithNapiValue);