From a128219a484f74e4e4a5d3db23824fb9b5efb5cd Mon Sep 17 00:00:00 2001
From: Jason Ginchereau <jasongin@microsoft.com>
Date: Thu, 4 May 2017 15:24:44 -0700
Subject: [PATCH] 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.

Backport-PR-URL: https://github.com/nodejs/node/pull/19447
PR-URL: https://github.com/nodejs/node/pull/12838
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/n-api.md                          |  7 -------
 src/node_api.cc                           | 21 ++++++++++++++++++++-
 test/addons-napi/test_async/test.js       | 18 ++++++++++++++++++
 test/addons-napi/test_async/test_async.cc | 19 -------------------
 4 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/doc/api/n-api.md b/doc/api/n-api.md
index f9e47a7b8d32a0..bc0499e4d09447 100644
--- a/doc/api/n-api.md
+++ b/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
diff --git a/src/node_api.cc b/src/node_api.cc
index 2f8409fc81627c..8600783523c7a5 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -2768,7 +2768,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);
+      }
     }
   }
 
diff --git a/test/addons-napi/test_async/test.js b/test/addons-napi/test_async/test.js
index 2b4577624a371e..eda6963743ff6f 100644
--- a/test/addons-napi/test_async/test.js
+++ b/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());
diff --git a/test/addons-napi/test_async/test_async.cc b/test/addons-napi/test_async/test_async.cc
index 21245e9e835659..cc7bc334304323 100644
--- a/test/addons-napi/test_async/test_async.cc
+++ b/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) {