From 2ca12c83b403e7313b314bf1bda6bed82f0e3ee1 Mon Sep 17 00:00:00 2001
From: Michael Dawson <mdawson@devrus.com>
Date: Tue, 20 Jul 2021 21:41:58 -0400
Subject: [PATCH] node-api: handle pending exception in cb wrapper

fixes: https://github.com/nodejs/node-addon-api/issues/1025

The functionreference test from the node-api tests
was reporting a failed v8 check when Node.js was compiled
as debug. The failure was because an exception was
pending but the C++ wrappers were returning
a return value that was invalid.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: https://github.com/nodejs/node/pull/39476
Fixes: https://github.com/nodejs/node-addon-api/issues/1025
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
---
 src/js_native_api_v8.cc                       |  8 +++++--
 test/js-native-api/test_function/test.js      |  8 +++++++
 .../test_function/test_function.c             | 23 +++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index 703c6bcc2c3898..e71c2222232e1b 100644
--- a/src/js_native_api_v8.cc
+++ b/src/js_native_api_v8.cc
@@ -306,12 +306,16 @@ class CallbackWrapperBase : public CallbackWrapper {
     napi_env env = _bundle->env;
     napi_callback cb = _bundle->cb;
 
-    napi_value result;
+    napi_value result = nullptr;
+    bool exceptionOccurred = false;
     env->CallIntoModule([&](napi_env env) {
       result = cb(env, cbinfo_wrapper);
+    }, [&](napi_env env, v8::Local<v8::Value> value) {
+      exceptionOccurred = true;
+      env->isolate->ThrowException(value);
     });
 
-    if (result != nullptr) {
+    if (!exceptionOccurred && (result != nullptr)) {
       this->SetReturnValue(result);
     }
   }
diff --git a/test/js-native-api/test_function/test.js b/test/js-native-api/test_function/test.js
index 988f128404e429..972adcd79f54b2 100644
--- a/test/js-native-api/test_function/test.js
+++ b/test/js-native-api/test_function/test.js
@@ -42,3 +42,11 @@ assert.deepStrictEqual(test_function.TestCreateFunctionParameters(), {
   cbIsNull: 'Invalid argument',
   resultIsNull: 'Invalid argument'
 });
+
+assert.throws(
+  () => test_function.TestBadReturnExceptionPending(),
+  {
+    code: 'throwing exception',
+    name: 'Error'
+  }
+);
diff --git a/test/js-native-api/test_function/test_function.c b/test/js-native-api/test_function/test_function.c
index 713e935e08c972..922d298136aa43 100644
--- a/test/js-native-api/test_function/test_function.c
+++ b/test/js-native-api/test_function/test_function.c
@@ -153,6 +153,19 @@ static napi_value MakeTrackedFunction(napi_env env, napi_callback_info info) {
   return result;
 }
 
+static napi_value TestBadReturnExceptionPending(napi_env env, napi_callback_info info) {
+  napi_throw_error(env, "throwing exception", "throwing exception");
+
+  // addons should only ever return a valid napi_value even if an
+  // exception occurs, but we have seen that the C++ wrapper
+  // with exceptions enabled sometimes returns an invalid value
+  // when an exception is thrown. Test that we ignore the return
+  // value then an exeption is pending. We use 0xFFFFFFFF as a value
+  // that should never be a valid napi_value and node seems to
+  // crash if it is not ignored indicating that it is indeed invalid.
+  return (napi_value)(0xFFFFFFFFF);
+}
+
 EXTERN_C_START
 napi_value Init(napi_env env, napi_value exports) {
   napi_value fn1;
@@ -183,6 +196,12 @@ napi_value Init(napi_env env, napi_value exports) {
                                       NULL,
                                       &fn5));
 
+  napi_value fn6;
+  NAPI_CALL(env,
+      napi_create_function(
+          env, "TestBadReturnExceptionPending", NAPI_AUTO_LENGTH,
+          TestBadReturnExceptionPending, NULL, &fn6));
+
   NAPI_CALL(env, napi_set_named_property(env, exports, "TestCall", fn1));
   NAPI_CALL(env, napi_set_named_property(env, exports, "TestName", fn2));
   NAPI_CALL(env, napi_set_named_property(env, exports, "TestNameShort", fn3));
@@ -196,6 +215,10 @@ napi_value Init(napi_env env, napi_value exports) {
                                          "TestCreateFunctionParameters",
                                          fn5));
 
+  NAPI_CALL(env,
+      napi_set_named_property(
+          env, exports, "TestBadReturnExceptionPending", fn6));
+
   return exports;
 }
 EXTERN_C_END