From c560db9ece6980eaf05b212d7893cc5b4ef74a51 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Wed, 19 Apr 2017 14:58:58 -0700 Subject: [PATCH] n-api: Enable scope and ref APIs during exception N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The NAPI_PREAMBLE macro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent to v8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending. We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios. Fixes: https://github.com/nodejs/abi-stable-node/issues/122 Fixes: https://github.com/nodejs/abi-stable-node/issues/228 Backport-PR-URL: https://github.com/nodejs/node/pull/19447 PR-URL: https://github.com/nodejs/node/pull/12524 Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson Reviewed-By: Gabriel Schulhof --- src/node_api.cc | 60 ++++++++++++------- test/addons-napi/test_handle_scope/test.js | 7 +++ .../test_handle_scope/test_handle_scope.c | 25 ++++++++ 3 files changed, 72 insertions(+), 20 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 3e88057fba97d4..a540284bfef99f 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -2044,7 +2044,9 @@ napi_status napi_create_reference(napi_env env, napi_value value, uint32_t initial_refcount, napi_ref* result) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, value); CHECK_ARG(env, result); @@ -2052,7 +2054,7 @@ napi_status napi_create_reference(napi_env env, env, v8impl::V8LocalValueFromJsValue(value), initial_refcount, false); *result = reinterpret_cast(reference); - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } // Deletes a reference. The referenced value is released, and may be GC'd unless @@ -2074,7 +2076,9 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) { // Calling this when the refcount is 0 and the object is unavailable // results in an error. napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, ref); v8impl::Reference* reference = reinterpret_cast(ref); @@ -2084,7 +2088,7 @@ napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) { *result = count; } - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } // Decrements the reference count, optionally returning the resulting count. If @@ -2092,7 +2096,9 @@ napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) { // time if there are no other references. Calling this when the refcount is // already 0 results in an error. napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, ref); v8impl::Reference* reference = reinterpret_cast(ref); @@ -2107,7 +2113,7 @@ napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) { *result = count; } - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } // Attempts to get a referenced value. If the reference is weak, the value might @@ -2116,59 +2122,71 @@ napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) { napi_status napi_get_reference_value(napi_env env, napi_ref ref, napi_value* result) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, ref); CHECK_ARG(env, result); v8impl::Reference* reference = reinterpret_cast(ref); *result = v8impl::JsValueFromV8LocalValue(reference->Get()); - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } napi_status napi_open_handle_scope(napi_env env, napi_handle_scope* result) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, result); *result = v8impl::JsHandleScopeFromV8HandleScope( new v8impl::HandleScopeWrapper(env->isolate)); - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } napi_status napi_close_handle_scope(napi_env env, napi_handle_scope scope) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, scope); delete v8impl::V8HandleScopeFromJsHandleScope(scope); - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } napi_status napi_open_escapable_handle_scope( napi_env env, napi_escapable_handle_scope* result) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, result); *result = v8impl::JsEscapableHandleScopeFromV8EscapableHandleScope( new v8impl::EscapableHandleScopeWrapper(env->isolate)); - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } napi_status napi_close_escapable_handle_scope( napi_env env, napi_escapable_handle_scope scope) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, scope); delete v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope); - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } napi_status napi_escape_handle(napi_env env, napi_escapable_handle_scope scope, napi_value escapee, napi_value* result) { - NAPI_PREAMBLE(env); + // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw + // JS exceptions. + CHECK_ENV(env); CHECK_ARG(env, scope); CHECK_ARG(env, escapee); CHECK_ARG(env, result); @@ -2177,7 +2195,7 @@ napi_status napi_escape_handle(napi_env env, v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope); *result = v8impl::JsValueFromV8LocalValue( s->Escape(v8impl::V8LocalValueFromJsValue(escapee))); - return GET_RETURN_STATUS(env); + return napi_clear_last_error(env); } napi_status napi_new_instance(napi_env env, @@ -2387,7 +2405,6 @@ napi_status napi_create_buffer(napi_env env, void** data, napi_value* result) { NAPI_PREAMBLE(env); - CHECK_ARG(env, data); CHECK_ARG(env, result); auto maybe = node::Buffer::New(env->isolate, length); @@ -2397,7 +2414,10 @@ napi_status napi_create_buffer(napi_env env, v8::Local buffer = maybe.ToLocalChecked(); *result = v8impl::JsValueFromV8LocalValue(buffer); - *data = node::Buffer::Data(buffer); + + if (data != nullptr) { + *data = node::Buffer::Data(buffer); + } return GET_RETURN_STATUS(env); } diff --git a/test/addons-napi/test_handle_scope/test.js b/test/addons-napi/test_handle_scope/test.js index ed52410f8075dd..20bee78882f071 100644 --- a/test/addons-napi/test_handle_scope/test.js +++ b/test/addons-napi/test_handle_scope/test.js @@ -7,4 +7,11 @@ const testHandleScope = require(`./build/${common.buildType}/test_handle_scope`); testHandleScope.NewScope(); + assert.ok(testHandleScope.NewScopeEscape() instanceof Object); + +assert.throws( + () => { + testHandleScope.NewScopeWithException(() => { throw new RangeError(); }); + }, + RangeError); diff --git a/test/addons-napi/test_handle_scope/test_handle_scope.c b/test/addons-napi/test_handle_scope/test_handle_scope.c index f88c04705ebdd2..6bcee16fda713e 100644 --- a/test/addons-napi/test_handle_scope/test_handle_scope.c +++ b/test/addons-napi/test_handle_scope/test_handle_scope.c @@ -29,10 +29,35 @@ napi_value NewScopeEscape(napi_env env, napi_callback_info info) { return escapee; } +napi_value NewScopeWithException(napi_env env, napi_callback_info info) { + napi_handle_scope scope; + size_t argc; + napi_value exception_function; + napi_status status; + napi_value output = NULL; + + NAPI_CALL(env, napi_open_handle_scope(env, &scope)); + NAPI_CALL(env, napi_create_object(env, &output)); + + argc = 1; + NAPI_CALL(env, napi_get_cb_info( + env, info, &argc, &exception_function, NULL, NULL)); + + status = napi_call_function( + env, output, exception_function, 0, NULL, NULL); + NAPI_ASSERT(env, status == napi_pending_exception, + "Function should have thrown."); + + // Closing a handle scope should still work while an exception is pending. + NAPI_CALL(env, napi_close_handle_scope(env, scope)); + return NULL; +} + void Init(napi_env env, napi_value exports, napi_value module, void* priv) { napi_property_descriptor properties[] = { DECLARE_NAPI_PROPERTY("NewScope", NewScope), DECLARE_NAPI_PROPERTY("NewScopeEscape", NewScopeEscape), + DECLARE_NAPI_PROPERTY("NewScopeWithException", NewScopeWithException), }; NAPI_CALL_RETURN_VOID(env, napi_define_properties(