Skip to content

Commit

Permalink
process: split routines used to enhance fatal exception stack traces
Browse files Browse the repository at this point in the history
Previously the enhancement were done right after emitting
`'uncaughtException'`, which meant by the time we knew the
exception was fatal in C++, the error.stack had already been
patched.

This patch moves those routines to be called later during the
fatal exception handling, and split them into two stages:
before and after the inspector is notified by the invocation of
`V8Inspector::exceptionThrown`. We now expand the stack to include
additional informations about unhandled 'error' events before
the inspector is notified, but delay the highlighting of the
frames until after the inspector is notified, so that the
ANSI escape sequences won't show up in the inspector console.

PR-URL: #28308
Fixes: #28287
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
joyeecheung authored and targos committed Jul 2, 2019
1 parent b744bd9 commit 0fd6524
Show file tree
Hide file tree
Showing 10 changed files with 166 additions and 55 deletions.
13 changes: 8 additions & 5 deletions lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,15 @@ const { Math, Object, Reflect } = primordials;

var spliceOne;

const {
kEnhanceStackBeforeInspector,
codes
} = require('internal/errors');
const {
ERR_INVALID_ARG_TYPE,
ERR_OUT_OF_RANGE,
ERR_UNHANDLED_ERROR
} = require('internal/errors').codes;
} = codes;

const {
inspect
Expand Down Expand Up @@ -142,8 +146,8 @@ function enhanceStackTrace(err, own) {
ownStack.splice(off + 1, len - 2,
' [... lines matching original stack trace ...]');
}
// Do this last, because it is the only operation with side effects.
err.stack = err.stack + sep + ownStack.join('\n');

return err.stack + sep + ownStack.join('\n');
}

EventEmitter.prototype.emit = function emit(type, ...args) {
Expand All @@ -162,11 +166,10 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
er = args[0];
if (er instanceof Error) {
try {
const { kExpandStackSymbol } = require('internal/util');
const capture = {};
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(capture, EventEmitter.prototype.emit);
Object.defineProperty(er, kExpandStackSymbol, {
Object.defineProperty(er, kEnhanceStackBeforeInspector, {
value: enhanceStackTrace.bind(null, er, capture),
configurable: true
});
Expand Down
17 changes: 15 additions & 2 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,22 @@ process.emitWarning = emitWarning;
}

function setupPrepareStackTrace() {
const { setPrepareStackTraceCallback } = internalBinding('errors');
const { prepareStackTrace } = require('internal/errors');
const {
setEnhanceStackForFatalException,
setPrepareStackTraceCallback
} = internalBinding('errors');
const {
prepareStackTrace,
fatalExceptionStackEnhancers: {
beforeInspector,
afterInspector
}
} = require('internal/errors');
// Tell our PrepareStackTraceCallback passed to the V8 API
// to call prepareStackTrace().
setPrepareStackTraceCallback(prepareStackTrace);
// Set the function used to enhance the error stack for printing
setEnhanceStackForFatalException(beforeInspector, afterInspector);
}

function setupProcessObject() {
Expand Down
41 changes: 41 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,45 @@ function addNumericalSeparator(val) {
return `${val.slice(0, i)}${res}`;
}

// Used to enhance the stack that will be picked up by the inspector
const kEnhanceStackBeforeInspector = Symbol('kEnhanceStackBeforeInspector');

// These are supposed to be called only on fatal exceptions before
// the process exits.
const fatalExceptionStackEnhancers = {
beforeInspector(error) {
if (typeof error[kEnhanceStackBeforeInspector] !== 'function') {
return error.stack;
}

try {
// Set the error.stack here so it gets picked up by the
// inspector.
error.stack = error[kEnhanceStackBeforeInspector]();
} catch {
// We are just enhancing the error. If it fails, ignore it.
}
return error.stack;
},
afterInspector(error) {
const originalStack = error.stack;
const {
inspect,
inspectDefaultOptions: {
colors: defaultColors
}
} = lazyInternalUtilInspect();
const colors = internalBinding('util').guessHandleType(2) === 'TTY' &&
require('internal/tty').hasColors() ||
defaultColors;
try {
return inspect(error, { colors });
} catch {
return originalStack;
}
}
};

module.exports = {
addCodeToName, // Exported for NghttpError
codes,
Expand All @@ -633,6 +672,8 @@ module.exports = {
// This is exported only to facilitate testing.
E,
prepareStackTrace,
kEnhanceStackBeforeInspector,
fatalExceptionStackEnhancers
};

// To declare an error message, use the E(sym, val, def) function above. The sym
Expand Down
12 changes: 0 additions & 12 deletions lib/internal/process/execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,6 @@ function createOnGlobalUncaughtException() {
} else if (!process.emit('uncaughtException', er, type)) {
// If someone handled it, then great. Otherwise, die in C++ land
// since that means that we'll exit the process, emit the 'exit' event.
const { inspect } = require('internal/util/inspect');
const colors = internalBinding('util').guessHandleType(2) === 'TTY' &&
require('internal/tty').hasColors() ||
inspect.defaultOptions.colors;
try {
if (!process._exiting) {
process._exiting = true;
Expand All @@ -163,14 +159,6 @@ function createOnGlobalUncaughtException() {
} catch {
// Nothing to be done about it at this point.
}
try {
const { kExpandStackSymbol } = require('internal/util');
if (typeof er[kExpandStackSymbol] === 'function')
er[kExpandStackSymbol]();
er.stack = inspect(er, { colors });
} catch {
// Nothing to be done about it at this point.
}
return false;
}

Expand Down
1 change: 0 additions & 1 deletion lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,5 @@ module.exports = {
// Used by the buffer module to capture an internal reference to the
// default isEncoding implementation, just in case userland overrides it.
kIsEncodingSymbol: Symbol('kIsEncodingSymbol'),
kExpandStackSymbol: Symbol('kExpandStackSymbol'),
kVmBreakFirstLineSymbol: Symbol('kVmBreakFirstLineSymbol')
};
3 changes: 2 additions & 1 deletion lib/internal/util/inspect.js
Original file line number Diff line number Diff line change
Expand Up @@ -1658,5 +1658,6 @@ function formatWithOptions(inspectOptions, ...args) {
module.exports = {
inspect,
format,
formatWithOptions
formatWithOptions,
inspectDefaultOptions
};
2 changes: 2 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,8 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
V(crypto_key_object_constructor, v8::Function) \
V(domain_callback, v8::Function) \
V(domexception_function, v8::Function) \
V(enhance_fatal_stack_after_inspector, v8::Function) \
V(enhance_fatal_stack_before_inspector, v8::Function) \
V(fs_use_promises_symbol, v8::Symbol) \
V(host_import_module_dynamically_callback, v8::Function) \
V(host_initialize_import_meta_object_callback, v8::Function) \
Expand Down
7 changes: 4 additions & 3 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ class NodeInspectorClient : public V8InspectorClient {
}
}

void FatalException(Local<Value> error, Local<Message> message) {
void ReportUncaughtException(Local<Value> error, Local<Message> message) {
Isolate* isolate = env_->isolate();
Local<Context> context = env_->context();

Expand Down Expand Up @@ -836,10 +836,11 @@ void Agent::WaitForDisconnect() {
}
}

void Agent::FatalException(Local<Value> error, Local<Message> message) {
void Agent::ReportUncaughtException(Local<Value> error,
Local<Message> message) {
if (!IsListening())
return;
client_->FatalException(error, message);
client_->ReportUncaughtException(error, message);
WaitForDisconnect();
}

Expand Down
4 changes: 2 additions & 2 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ class Agent {
void WaitForConnect();
// Blocks till all the sessions with "WaitForDisconnectOnShutdown" disconnect
void WaitForDisconnect();
void FatalException(v8::Local<v8::Value> error,
v8::Local<v8::Message> message);
void ReportUncaughtException(v8::Local<v8::Value> error,
v8::Local<v8::Message> message);

// Async stack traces instrumentation.
void AsyncTaskScheduled(const v8_inspector::StringView& taskName, void* task,
Expand Down
Loading

0 comments on commit 0fd6524

Please sign in to comment.