Skip to content

Commit

Permalink
src: print exceptions from PromiseRejectCallback
Browse files Browse the repository at this point in the history
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: #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>
  • Loading branch information
addaleax authored and BridgeAR committed Sep 25, 2019
1 parent f90740d commit 51a1dfa
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 1 deletion.
10 changes: 10 additions & 0 deletions src/node_task_queue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

namespace node {

using errors::TryCatchScope;
using v8::Array;
using v8::Context;
using v8::Function;
Expand Down Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-async-wrap-pop-id-during-load.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
35 changes: 35 additions & 0 deletions test/parallel/test-promise-reject-callback-exception.js
Original file line number Diff line number Diff line change
@@ -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);
}

0 comments on commit 51a1dfa

Please sign in to comment.