From dad139164df365aafa551ebda57e34b1800999fd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 9 Sep 2019 23:43:21 +0200 Subject: [PATCH 1/3] 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). --- src/node_task_queue.cc | 10 ++++++ .../test-promise-reject-callback-exception.js | 31 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 test/parallel/test-promise-reject-callback-exception.js diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index 4ff51bc3b80b56..b4e8c1de928bd9 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 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-promise-reject-callback-exception.js b/test/parallel/test-promise-reject-callback-exception.js new file mode 100644 index 00000000000000..a0e5d71482b5e4 --- /dev/null +++ b/test/parallel/test-promise-reject-callback-exception.js @@ -0,0 +1,31 @@ +'use strict'; +const common = 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]) { + 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); +} From b990248fb9f23085ef3d39db19ba1c1577d81e24 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 12 Sep 2019 22:04:27 +0200 Subject: [PATCH 2/3] fixup! src: print exceptions from PromiseRejectCallback --- test/parallel/test-async-wrap-pop-id-during-load.js | 2 +- .../parallel/test-promise-reject-callback-exception.js | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) 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 1ab98104796557..bf75451817164b 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 index a0e5d71482b5e4..14a084451da39b 100644 --- a/test/parallel/test-promise-reject-callback-exception.js +++ b/test/parallel/test-promise-reject-callback-exception.js @@ -1,5 +1,5 @@ 'use strict'; -const common = require('../common'); +require('../common'); const tmpdir = require('../common/tmpdir'); const assert = require('assert'); const path = require('path'); @@ -21,11 +21,11 @@ for (const NODE_V8_COVERAGE of ['', tmpdir.path]) { { env: { ...process.env, NODE_V8_COVERAGE } }); assert(stdout.toString('utf8') - .startsWith('RangeError: Maximum call stack size exceeded'), - `stdout: <${stdout}>`); + .startsWith('RangeError: Maximum call stack size exceeded'), + `stdout: <${stdout}>`); assert(stderr.toString('utf8') - .startsWith('Exception in PromiseRejectCallback'), - `stderr: <${stderr}>`); + .startsWith('Exception in PromiseRejectCallback'), + `stderr: <${stderr}>`); assert.strictEqual(status, 0); assert.strictEqual(signal, null); } From b236ffbf853b51eed8da9b3013663af91a6ce11f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 13 Sep 2019 13:51:41 +0200 Subject: [PATCH 3/3] fixup! src: print exceptions from PromiseRejectCallback --- test/parallel/test-promise-reject-callback-exception.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/parallel/test-promise-reject-callback-exception.js b/test/parallel/test-promise-reject-callback-exception.js index 14a084451da39b..0d205807de4f20 100644 --- a/test/parallel/test-promise-reject-callback-exception.js +++ b/test/parallel/test-promise-reject-callback-exception.js @@ -15,6 +15,10 @@ tmpdir.refresh(); // 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')],