From af50ac281bae17f22f32ca14af1cab5f00c891c6 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 12 Dec 2019 09:30:37 -0800 Subject: [PATCH] error: do not replace pending exception MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node-addon-api/issues/621 PR-URL: https://github.com/nodejs/node-addon-api/pull/629 Reviewed-By: Tobias Nießen Reviewed-By: Chengzhong Wu --- napi-inl.h | 20 +++++++++---------- test/error.cc | 35 +++++++++++++++++++++++++++++++++ test/error.js | 6 ++++++ test/object/delete_property.js | 2 +- test/object/get_property.js | 2 +- test/object/has_own_property.js | 2 +- test/object/has_property.js | 2 +- test/object/set_property.js | 2 +- 8 files changed, 55 insertions(+), 16 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 8647cd8dc..bef889f9a 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2088,12 +2088,19 @@ inline void Buffer::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"); } @@ -2101,15 +2108,6 @@ inline Error Error::New(napi_env env) { 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 index 44b1a2e96..832cad525 100644 --- 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().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 index f41874d55..031db081a 100644 --- 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 index f560d2698..8c313d03e 100644 --- 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 index bec009d74..7ec9b119e 100644 --- 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 index 11a3ff049..570b0ee46 100644 --- 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 index 66024b392..a1b942dfb 100644 --- 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 index 7606ddf59..9b64cc5dd 100644 --- 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);