n-api: emit uncaught-exception on unhandled tsfn callbacks

legendecas · legendecas · commit ae41fe34ae17 · 2021-01-29T22:25:11.000+08:00
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -585,6 +585,13 @@ added: v9.0.0
 Disables runtime checks for `async_hooks`. These will still be enabled
 dynamically when `async_hooks` is enabled.
 
+### `--no-force-napi-uncaught-exceptions-policy`
+<!-- YAML
+added: REPLACEME
+-->
+
+Disables 'uncaughtException' event on N-API asynchronous callbacks.
+
 ### `--no-warnings`
 <!-- YAML
 added: v6.0.0
@@ -1378,6 +1385,7 @@ Node.js options that are allowed are:
 * `--napi-modules`
 * `--no-deprecation`
 * `--no-force-async-hooks-checks`
+* `--no-force-napi-uncaught-exceptions-policy`
 * `--no-warnings`
 * `--node-memory-debug`
 * `--openssl-config`
diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
@@ -57,15 +57,7 @@ struct napi_env__ {
         context_persistent(isolate, context) {
     CHECK_EQ(isolate, context->GetIsolate());
   }
-  virtual ~napi_env__() {
-    // First we must finalize those references that have `napi_finalizer`
-    // callbacks. The reason is that addons might store other references which
-    // they delete during their `napi_finalizer` callbacks. If we deleted such
-    // references here first, they would be doubly deleted when the
-    // `napi_finalizer` deleted them subsequently.
-    v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
-    v8impl::RefTracker::FinalizeAll(&reflist);
-  }
+  virtual ~napi_env__() { FinalizeAll(); }
   v8::Isolate* const isolate;  // Shortcut for context()->GetIsolate()
   v8impl::Persistent<v8::Context> context_persistent;
 
@@ -102,10 +94,17 @@ struct napi_env__ {
   }
 
   virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
-    v8::HandleScope handle_scope(isolate);
-    CallIntoModule([&](napi_env env) {
-      cb(env, data, hint);
-    });
+    // Forward declaration virtual member. Implemented in node_napi_env__.
+  }
+
+  void FinalizeAll() {
+    // First we must finalize those references that have `napi_finalizer`
+    // callbacks. The reason is that addons might store other references which
+    // they delete during their `napi_finalizer` callbacks. If we deleted such
+    // references here first, they would be doubly deleted when the
+    // `napi_finalizer` deleted them subsequently.
+    v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
+    v8impl::RefTracker::FinalizeAll(&reflist);
   }
 
   v8impl::Persistent<v8::Value> last_exception;
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -8,6 +8,7 @@
 #include "node_buffer.h"
 #include "node_errors.h"
 #include "node_internals.h"
+#include "node_process.h"
 #include "threadpoolwork-inl.h"
 #include "tracing/traced_value.h"
 #include "util-inl.h"
@@ -20,6 +21,8 @@ struct node_napi_env__ : public napi_env__ {
     CHECK_NOT_NULL(node_env());
   }
 
+  ~node_napi_env__() { FinalizeAll(); }
+
   inline node::Environment* node_env() const {
     return node::Environment::GetCurrent(context());
   }
@@ -36,15 +39,55 @@ struct node_napi_env__ : public napi_env__ {
         v8::True(isolate));
   }
 
+  inline void trigger_fatal_exception(v8::Local<v8::Value> local_err) {
+    v8::Local<v8::Message> local_msg =
+        v8::Exception::CreateMessage(isolate, local_err);
+    node::errors::TriggerUncaughtException(isolate, local_err, local_msg);
+  }
+
+  // option enforceUncaughtExceptionPolicy is added for not breaking existing
+  // running n-api add-ons, and should be deprecated in the next major Node.js
+  // release.
+  template <typename T>
+  inline void CallbackIntoModule(T&& call,
+                                 bool enforceUncaughtExceptionPolicy = false) {
+    CallIntoModule(
+        call,
+        [enforceUncaughtExceptionPolicy](napi_env env_,
+                                         v8::Local<v8::Value> local_err) {
+          node_napi_env__* env = static_cast<node_napi_env__*>(env_);
+          node::Environment* node_env = env->node_env();
+          if (node_env->options()->no_force_napi_uncaught_exception_policy &&
+              !enforceUncaughtExceptionPolicy) {
+            ProcessEmitDeprecationWarning(
+                node_env,
+                "Uncaught N-API callback exception detected, please run node "
+                "without option "
+                "--no-force-napi-uncaught-exceptions-policy to handle those"
+                "exceptions properly.",
+                "DEP0XXX");
+            return;
+          }
+          // If there was an unhandled exception in the complete callback,
+          // report it as a fatal exception. (There is no JavaScript on the
+          // callstack that can possibly handle it.)
+          env->trigger_fatal_exception(local_err);
+        });
+  }
+
   void CallFinalizer(napi_finalize cb, void* data, void* hint) override {
+    CallFinalizer(cb, data, hint, true);
+  }
+
+  inline void CallFinalizer(napi_finalize cb,
+                            void* data,
+                            void* hint,
+                            bool enforceUncaughtExceptionPolicy) {
     napi_env env = static_cast<napi_env>(this);
-    node_env()->SetImmediate([=](node::Environment* node_env) {
-      v8::HandleScope handle_scope(env->isolate);
-      v8::Context::Scope context_scope(env->context());
-      env->CallIntoModule([&](napi_env env) {
-        cb(env, data, hint);
-      });
-    });
+    v8::HandleScope handle_scope(env->isolate);
+    v8::Context::Scope context_scope(env->context());
+    CallbackIntoModule([&](napi_env env) { cb(env, data, hint); },
+                       enforceUncaughtExceptionPolicy);
   }
 };
 
@@ -71,12 +114,9 @@ class BufferFinalizer : private Finalizer {
       v8::HandleScope handle_scope(finalizer->_env->isolate);
       v8::Context::Scope context_scope(finalizer->_env->context());
 
-      finalizer->_env->CallIntoModule([&](napi_env env) {
-        finalizer->_finalize_callback(
-            env,
-            finalizer->_finalize_data,
-            finalizer->_finalize_hint);
-      });
+      finalizer->_env->CallFinalizer(finalizer->_finalize_callback,
+                                     finalizer->_finalize_data,
+                                     finalizer->_finalize_hint);
     });
   }
 
@@ -107,13 +147,6 @@ static inline napi_env NewEnv(v8::Local<v8::Context> context) {
   return result;
 }
 
-static inline void trigger_fatal_exception(
-    napi_env env, v8::Local<v8::Value> local_err) {
-  v8::Local<v8::Message> local_msg =
-    v8::Exception::CreateMessage(env->isolate, local_err);
-  node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg);
-}
-
 class ThreadSafeFunction : public node::AsyncResource {
  public:
   ThreadSafeFunction(v8::Local<v8::Function> func,
@@ -312,19 +345,16 @@ class ThreadSafeFunction : public node::AsyncResource {
           v8::Local<v8::Function>::New(env->isolate, ref);
         js_callback = v8impl::JsValueFromV8LocalValue(js_cb);
       }
-      env->CallIntoModule([&](napi_env env) {
-        call_js_cb(env, js_callback, context, data);
-      });
+      env->CallbackIntoModule(
+          [&](napi_env env) { call_js_cb(env, js_callback, context, data); });
     }
   }
 
   void Finalize() {
     v8::HandleScope scope(env->isolate);
     if (finalize_cb) {
       CallbackScope cb_scope(this);
-      env->CallIntoModule([&](napi_env env) {
-        finalize_cb(env, finalize_data, context);
-      });
+      env->CallFinalizer(finalize_cb, finalize_data, context, false);
     }
     EmptyQueueAndDelete();
   }
@@ -693,7 +723,7 @@ napi_status napi_fatal_exception(napi_env env, napi_value err) {
   CHECK_ARG(env, err);
 
   v8::Local<v8::Value> local_err = v8impl::V8LocalValueFromJsValue(err);
-  v8impl::trigger_fatal_exception(env, local_err);
+  static_cast<node_napi_env>(env)->trigger_fatal_exception(local_err);
 
   return napi_clear_last_error(env);
 }
@@ -1043,14 +1073,11 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork {
 
     CallbackScope callback_scope(this);
 
-    _env->CallIntoModule([&](napi_env env) {
-      _complete(env, ConvertUVErrorCode(status), _data);
-    }, [](napi_env env, v8::Local<v8::Value> local_err) {
-      // If there was an unhandled exception in the complete callback,
-      // report it as a fatal exception. (There is no JavaScript on the
-      // callstack that can possibly handle it.)
-      v8impl::trigger_fatal_exception(env, local_err);
-    });
+    _env->CallbackIntoModule(
+        [&](napi_env env) {
+          _complete(env, ConvertUVErrorCode(status), _data);
+        },
+        true);
 
     // Note: Don't access `work` after this point because it was
     // likely deleted by the complete callback.
diff --git a/src/node_options.cc b/src/node_options.cc
@@ -391,6 +391,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "disable checks for async_hooks",
             &EnvironmentOptions::no_force_async_hooks_checks,
             kAllowedInEnvironment);
+  AddOption("--no-force-napi-uncaught-exceptions-policy",
+            "disable 'uncaughtException' event on N-API asynchronous callbacks",
+            &EnvironmentOptions::no_force_napi_uncaught_exception_policy,
+            kAllowedInEnvironment);
   AddOption("--no-warnings",
             "silence all process warnings",
             &EnvironmentOptions::no_warnings,
diff --git a/src/node_options.h b/src/node_options.h
@@ -120,6 +120,7 @@ class EnvironmentOptions : public Options {
   uint64_t max_http_header_size = 16 * 1024;
   bool no_deprecation = false;
   bool no_force_async_hooks_checks = false;
+  bool no_force_napi_uncaught_exception_policy = false;
   bool no_warnings = false;
   bool force_context_aware = false;
   bool pending_deprecation = false;
diff --git a/test/js-native-api/test_reference/test_finalizer.js b/test/js-native-api/test_reference/test_finalizer.js
@@ -0,0 +1,20 @@
+'use strict';
+// Flags: --expose-gc --no-concurrent-array-buffer-freeing --no-concurrent-array-buffer-sweeping
+
+const common = require('../../common');
+const test_reference = require(`./build/${common.buildType}/test_reference`);
+const assert = require('assert');
+
+process.on('uncaughtException', common.mustCall((err) => {
+  assert.throws(() => { throw err; }, /finalizer error/);
+}));
+
+(async function() {
+  {
+    test_reference.createExternalWithJsFinalize(
+      common.mustCall(() => {
+        throw new Error('finalizer error');
+      }));
+  }
+  global.gc();
+})().then(common.mustCall());
diff --git a/test/js-native-api/test_reference/test_reference.c b/test/js-native-api/test_reference/test_reference.c
@@ -20,6 +20,20 @@ static void FinalizeExternal(napi_env env, void* data, void* hint) {
   finalize_count++;
 }
 
+static void FinalizeExternalCallJs(napi_env env, void* data, void* hint) {
+  int *actual_value = data;
+  NAPI_ASSERT_RETURN_VOID(env, actual_value == &test_value,
+      "The correct pointer was passed to the finalizer");
+
+  napi_ref finalizer_ref = (napi_ref)hint;
+  napi_value js_finalizer;
+  napi_value recv;
+  NAPI_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
+  NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
+  NAPI_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
+  NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
+}
+
 static napi_value CreateExternal(napi_env env, napi_callback_info info) {
   int* data = &test_value;
 
@@ -49,6 +63,31 @@ CreateExternalWithFinalize(napi_env env, napi_callback_info info) {
   return result;
 }
 
+static napi_value
+CreateExternalWithJsFinalize(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value args[1];
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
+  NAPI_ASSERT(env, argc == 1, "Wrong number of arguments");
+  napi_value finalizer = args[0];
+  napi_valuetype finalizer_valuetype;
+  NAPI_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
+  NAPI_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
+  napi_ref finalizer_ref;
+  NAPI_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
+
+  napi_value result;
+  NAPI_CALL(env,
+      napi_create_external(env,
+                           &test_value,
+                           FinalizeExternalCallJs,
+                           finalizer_ref, /* finalize_hint */
+                           &result));
+
+  finalize_count = 0;
+  return result;
+}
+
 static napi_value CheckExternal(napi_env env, napi_callback_info info) {
   size_t argc = 1;
   napi_value arg;
@@ -176,6 +215,8 @@ napi_value Init(napi_env env, napi_value exports) {
     DECLARE_NAPI_PROPERTY("createExternal", CreateExternal),
     DECLARE_NAPI_PROPERTY("createExternalWithFinalize",
         CreateExternalWithFinalize),
+    DECLARE_NAPI_PROPERTY("createExternalWithJsFinalize",
+        CreateExternalWithJsFinalize),
     DECLARE_NAPI_PROPERTY("checkExternal", CheckExternal),
     DECLARE_NAPI_PROPERTY("createReference", CreateReference),
     DECLARE_NAPI_PROPERTY("deleteReference", DeleteReference),
diff --git a/test/node-api/test_buffer/test_buffer.c b/test/node-api/test_buffer/test_buffer.c
@@ -20,6 +20,17 @@ static void noopDeleter(napi_env env, void* data, void* finalize_hint) {
   deleterCallCount++;
 }
 
+static void malignDeleter(napi_env env, void* data, void* finalize_hint) {
+  NAPI_ASSERT_RETURN_VOID(env, data != NULL && strcmp(data, theText) == 0, "invalid data");
+  napi_ref finalizer_ref = (napi_ref)finalize_hint;
+  napi_value js_finalizer;
+  napi_value recv;
+  NAPI_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
+  NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
+  NAPI_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
+  NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
+}
+
 static napi_value newBuffer(napi_env env, napi_callback_info info) {
   napi_value theBuffer;
   char* theCopy;
@@ -119,6 +130,30 @@ static napi_value staticBuffer(napi_env env, napi_callback_info info) {
   return theBuffer;
 }
 
+static napi_value malignFinalizerBuffer(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value args[1];
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
+  NAPI_ASSERT(env, argc == 1, "Wrong number of arguments");
+  napi_value finalizer = args[0];
+  napi_valuetype finalizer_valuetype;
+  NAPI_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
+  NAPI_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
+  napi_ref finalizer_ref;
+  NAPI_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
+
+  napi_value theBuffer;
+  NAPI_CALL(
+      env,
+      napi_create_external_buffer(env,
+                                  sizeof(theText),
+                                  (void*)theText,
+                                  malignDeleter,
+                                  finalizer_ref,  // finalize_hint
+                                  &theBuffer));
+  return theBuffer;
+}
+
 static napi_value Init(napi_env env, napi_value exports) {
   napi_value theValue;
 
@@ -134,6 +169,7 @@ static napi_value Init(napi_env env, napi_value exports) {
     DECLARE_NAPI_PROPERTY("bufferHasInstance", bufferHasInstance),
     DECLARE_NAPI_PROPERTY("bufferInfo", bufferInfo),
     DECLARE_NAPI_PROPERTY("staticBuffer", staticBuffer),
+    DECLARE_NAPI_PROPERTY("malignFinalizerBuffer", malignFinalizerBuffer),
   };
 
   NAPI_CALL(env, napi_define_properties(
diff --git a/test/node-api/test_buffer/test_finalizer.js b/test/node-api/test_buffer/test_finalizer.js
diff --git a/test/node-api/test_threadsafe_function/binding.c b/test/node-api/test_threadsafe_function/binding.c
diff --git a/test/node-api/test_threadsafe_function/test_no_force_uncaught_exception.js b/test/node-api/test_threadsafe_function/test_no_force_uncaught_exception.js
diff --git a/test/node-api/test_threadsafe_function/test_uncaught_exception.js b/test/node-api/test_threadsafe_function/test_uncaught_exception.js
