n-api: Handle fatal exception in async callback

 - Create a handle scope before invoking the async completion
   callback, because it is basically always needed, easy for user
   code to forget, and this makes it more consistent with ordinary
   N-API function callbacks.

 - Check for an unhandled JS exception after invoking an async
   completion callback, and report it via `node::FatalException()`.

 - Add a corresponding test case for an exception in async callback.

Previously, any unhandled JS exception thrown from a
`napi_async_complete_callback` would be silently ignored. Among other
things this meant assertions in some test cases could be undetected.

PR-URL: #12838
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>

doc/api/n-api.md

@@ -2855,13 +2855,6 @@ will be invoked with a status value of `napi_cancelled`.
 The work should not be deleted before the `complete`
 callback invocation, even when it was cancelled.
 
-**Note:** As mentioned in the section on memory management, if
-the code to be run in the callbacks will create N-API values, then
-N-API handle scope functions must be used to create/destroy a
-`napi_handle_scope` such that the scope is active when
-objects can be created.
-
-
 ### napi_create_async_work
 <!-- YAML
 added: v8.0.0

src/node_api.cc

@@ -2766,7 +2766,26 @@ class Work {
     Work* work = static_cast<Work*>(req->data);
 
     if (work->_complete != nullptr) {
-      work->_complete(work->_env, ConvertUVErrorCode(status), work->_data);
+      napi_env env = work->_env;
+
+      // Establish a handle scope here so that every callback doesn't have to.
+      // Also it is needed for the exception-handling below.
+      v8::HandleScope scope(env->isolate);
+
+      work->_complete(env, ConvertUVErrorCode(status), work->_data);
+
+      // Note: Don't access `work` after this point because it was
+      // likely deleted by the complete callback.
+
+      // 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.)
+      if (!env->last_exception.IsEmpty()) {
+        v8::TryCatch try_catch;
+        env->isolate->ThrowException(
+          v8::Local<v8::Value>::New(env->isolate, env->last_exception));
+        node::FatalException(env->isolate, try_catch);
+      }
     }
   }
 

test/addons-napi/test_async/test.js

@@ -1,12 +1,30 @@
 'use strict';
 const common = require('../../common');
 const assert = require('assert');
+const child_process = require('child_process');
 const test_async = require(`./build/${common.buildType}/test_async`);
 
+const testException = 'test_async_cb_exception';
+
+// Exception thrown from async completion callback.
+// (Tested in a spawned process because the exception is fatal.)
+if (process.argv[2] === 'child') {
+  test_async.Test(1, common.mustCall(function(err, val) {
+    throw new Error(testException);
+  }));
+  return;
+}
+const p = child_process.spawnSync(
+  process.execPath, [ '--napi-modules', __filename, 'child' ]);
+assert.ifError(p.error);
+assert.ok(p.stderr.toString().includes(testException));
+
+// Successful async execution and completion callback.
 test_async.Test(5, common.mustCall(function(err, val) {
   assert.strictEqual(err, null);
   assert.strictEqual(val, 10);
   process.nextTick(common.mustCall());
 }));
 
+// Async work item cancellation with callback.
 test_async.TestCancel(common.mustCall());

test/addons-napi/test_async/test_async.cc

@@ -20,22 +20,6 @@ typedef struct {
 carrier the_carrier;
 carrier async_carrier[MAX_CANCEL_THREADS];
 
-struct AutoHandleScope {
-  explicit AutoHandleScope(napi_env env)
-  : _env(env),
-  _scope(nullptr) {
-    napi_open_handle_scope(_env, &_scope);
-  }
-  ~AutoHandleScope() {
-    napi_close_handle_scope(_env, _scope);
-  }
- private:
-  AutoHandleScope() { }
-
-  napi_env _env;
-  napi_handle_scope _scope;
-};
-
 void Execute(napi_env env, void* data) {
 #if defined _WIN32
   Sleep(1000);
@@ -53,7 +37,6 @@ void Execute(napi_env env, void* data) {
 }
 
 void Complete(napi_env env, napi_status status, void* data) {
-  AutoHandleScope scope(env);
   carrier* c = static_cast<carrier*>(data);
 
   if (c != &the_carrier) {
@@ -116,13 +99,11 @@ napi_value Test(napi_env env, napi_callback_info info) {
 }
 
 void BusyCancelComplete(napi_env env, napi_status status, void* data) {
-  AutoHandleScope scope(env);
   carrier* c = static_cast<carrier*>(data);
   NAPI_CALL_RETURN_VOID(env, napi_delete_async_work(env, c->_request));
 }
 
 void CancelComplete(napi_env env, napi_status status, void* data) {
-  AutoHandleScope scope(env);
   carrier* c = static_cast<carrier*>(data);
 
   if (status == napi_cancelled) {