src: print exceptions from PromiseRejectCallback

Previously, leaving the exception lying around would leave the JS
engine in an invalid state, as it was not expecting exceptions to
be thrown from the C++ `PromiseRejectCallback`, and lead to hard
crashes under some conditions (e.g. with coverage enabled).

PR-URL: https://github.com/nodejs/node/pull/29513
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

3 files changed, 46 insertions, 1 deletions

diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc
index 4ff51bc3b8..b4e8c1de92 100644
--- a/src/node_task_queue.cc
+++ b/src/node_task_queue.cc
@@ -10,6 +10,7 @@
 
 namespace node {
 
+using errors::TryCatchScope;
 using v8::Array;
 using v8::Context;
 using v8::Function;
@@ -111,8 +112,17 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
   }
 
   Local<Value> args[] = { type, promise, value };
+
+  // V8 does not expect this callback to have a scheduled exceptions once it
+  // returns, so we print them out in a best effort to do something about it
+  // without failing silently and without crashing the process.
+  TryCatchScope try_catch(env);
   USE(callback->Call(
       env->context(), Undefined(isolate), arraysize(args), args));
+  if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
+    fprintf(stderr, "Exception in PromiseRejectCallback:\n");
+    PrintCaughtException(isolate, env->context(), try_catch);
+  }
 }
 
 static void SetPromiseRejectCallback(
diff --git a/test/parallel/test-async-wrap-pop-id-during-load.js b/test/parallel/test-async-wrap-pop-id-during-load.js
index 1ab9810479..bf75451817 100644
--- a/test/parallel/test-async-wrap-pop-id-during-load.js
+++ b/test/parallel/test-async-wrap-pop-id-during-load.js
@@ -23,4 +23,4 @@ assert.strictEqual(ret.status, 0,
                    `EXIT CODE: ${ret.status}, STDERR:\n${ret.stderr}`);
 const stderr = ret.stderr.toString('utf8', 0, 2048);
 assert.ok(!/async.*hook/i.test(stderr));
-assert.ok(stderr.includes('UnhandledPromiseRejectionWarning: Error'), stderr);
+assert.ok(stderr.includes('Maximum call stack size exceeded'), stderr);
diff --git a/test/parallel/test-promise-reject-callback-exception.js b/test/parallel/test-promise-reject-callback-exception.js
new file mode 100644
index 0000000000..0d205807de
--- /dev/null
+++ b/test/parallel/test-promise-reject-callback-exception.js
@@ -0,0 +1,35 @@
+'use strict';
+require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const path = require('path');
+const child_process = require('child_process');
+
+tmpdir.refresh();
+
+// Tests that exceptions from the PromiseRejectCallback are printed to stderr
+// when they occur as a best-effort way of handling them, and that calling
+// `console.log()` works after that. Earlier, the latter did not work because
+// of the exception left lying around by the PromiseRejectCallback when its JS
+// part exceeded the call stack limit, and when the inspector/built-in coverage
+// was enabled, it resulted in a hard crash.
+
+for (const NODE_V8_COVERAGE of ['', tmpdir.path]) {
+  // NODE_V8_COVERAGE does not work without the inspector.
+  // Refs: https://github.com/nodejs/node/issues/29542
+  if (!process.features.inspector && NODE_V8_COVERAGE !== '') continue;
+
+  const { status, signal, stdout, stderr } =
+    child_process.spawnSync(process.execPath,
+                            [path.join(__dirname, 'test-ttywrap-stack.js')],
+                            { env: { ...process.env, NODE_V8_COVERAGE } });
+
+  assert(stdout.toString('utf8')
+         .startsWith('RangeError: Maximum call stack size exceeded'),
+         `stdout: <${stdout}>`);
+  assert(stderr.toString('utf8')
+         .startsWith('Exception in PromiseRejectCallback'),
+         `stderr: <${stderr}>`);
+  assert.strictEqual(status, 0);
+  assert.strictEqual(signal, null);
+}