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
PR-URL: https://github.com/nodejs/node/pull/12524
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

src/node_api.cc

@@ -2043,7 +2043,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);
 
@@ -2051,7 +2053,7 @@ napi_status napi_create_reference(napi_env env,
       env, v8impl::V8LocalValueFromJsValue(value), initial_refcount, false);
 
   *result = reinterpret_cast<napi_ref>(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
@@ -2073,7 +2075,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<v8impl::Reference*>(ref);
@@ -2083,7 +2087,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
@@ -2091,7 +2095,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<v8impl::Reference*>(ref);
@@ -2106,7 +2112,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
@@ -2115,59 +2121,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<v8impl::Reference*>(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);
@@ -2176,7 +2194,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,
@@ -2386,7 +2404,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);
@@ -2396,7 +2413,10 @@ napi_status napi_create_buffer(napi_env env,
   v8::Local<v8::Object> buffer = maybe.ToLocalChecked();
 
   *result = v8impl::JsValueFromV8LocalValue(buffer);
+
+  if (data != nullptr) {
     *data = node::Buffer::Data(buffer);
+  }
 
   return GET_RETURN_STATUS(env);
 }

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

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(