From 76577e70777d176efcf70c6a9df8119eeb3f5a1b Mon Sep 17 00:00:00 2001 From: Jeremy Whitlock Date: Mon, 10 Aug 2015 11:47:12 -0600 Subject: domains: port fix abort on uncaught to v0.12 Port fbff705 and caeb677 from v0.10 to v0.12, original commit messages: fbff705 Add v8::Isolate::SetAbortOnUncaughtException() so the user can be notified when an uncaught exception has bubbled. caeb677 Do not abort the process if an error is thrown from within a domain, an error handler is setup for the domain and --abort-on-uncaught-exception was passed on the command line. However, if an error is thrown from within the top-level domain's error handler and --abort-on-uncaught-exception was passed on the command line, make the process abort. Fixes: #8877 Reviewed-By: Julien Gilli PR-URL: https://github.com/joyent/node/pull/25835 --- deps/v8/include/v8.h | 11 + deps/v8/src/api.cc | 7 + deps/v8/src/isolate.cc | 42 ++-- deps/v8/src/isolate.h | 5 + lib/domain.js | 88 +++++--- src/env.h | 1 + src/node.cc | 32 +++ .../test-domain-top-level-error-handler-throw.js | 74 +++++++ ...test-domain-with-abort-on-uncaught-exception.js | 229 +++++++++++++++++++++ 9 files changed, 448 insertions(+), 41 deletions(-) create mode 100644 test/simple/test-domain-top-level-error-handler-throw.js create mode 100644 test/simple/test-domain-with-abort-on-uncaught-exception.js diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h index ef0bda63f..8ae075797 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -4186,6 +4186,17 @@ class V8_EXPORT Isolate { */ static Isolate* GetCurrent(); + /** + * Custom callback used by embedders to help V8 determine if it should abort + * when it throws and no internal handler can catch the exception. + * If FLAG_abort_on_uncaught_exception is true, then V8 will abort if either: + * - no custom callback is set. + * - the custom callback set returns true. + * Otherwise it won't abort. + */ + typedef bool (*abort_on_uncaught_exception_t)(Isolate*); + void SetAbortOnUncaughtException(abort_on_uncaught_exception_t callback); + /** * Methods below this point require holding a lock (using Locker) in * a multi-threaded environment. diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc index 3d9e9d831..e6597246e 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -6556,6 +6556,13 @@ void Isolate::Enter() { } +void Isolate::SetAbortOnUncaughtException( + abort_on_uncaught_exception_t callback) { + i::Isolate* isolate = reinterpret_cast(this); + isolate->SetAbortOnUncaughtException(callback); +} + + void Isolate::Exit() { i::Isolate* isolate = reinterpret_cast(this); isolate->Exit(); diff --git a/deps/v8/src/isolate.cc b/deps/v8/src/isolate.cc index 215296d73..2905a0828 100644 --- a/deps/v8/src/isolate.cc +++ b/deps/v8/src/isolate.cc @@ -1090,19 +1090,30 @@ void Isolate::DoThrow(Object* exception, MessageLocation* location) { thread_local_top()->pending_message_end_pos_ = location->end_pos(); } - // If the abort-on-uncaught-exception flag is specified, abort on any - // exception not caught by JavaScript, even when an external handler is - // present. This flag is intended for use by JavaScript developers, so - // print a user-friendly stack trace (not an internal one). + // If the abort-on-uncaught-exception flag is specified, and if the + // exception is not caught by JavaScript (even when an external handler is + // present). if (fatal_exception_depth == 0 && - FLAG_abort_on_uncaught_exception && + (FLAG_abort_on_uncaught_exception) && (report_exception || can_be_caught_externally)) { - fatal_exception_depth++; - PrintF(stderr, - "%s\n\nFROM\n", - MessageHandler::GetLocalizedMessage(this, message_obj).get()); - PrintCurrentStackTrace(stderr); - base::OS::Abort(); + // If the embedder didn't specify a custom uncaught exception callback, + // or if the custom callback determined that V8 should abort, then + // abort + bool should_abort = !abort_on_uncaught_exception_callback_ || + abort_on_uncaught_exception_callback_( + reinterpret_cast(this) + ); + + if (should_abort) { + fatal_exception_depth++; + // This flag is intended for use by JavaScript developers, so + // print a user-friendly stack trace (not an internal one). + PrintF(stderr, + "%s\n\nFROM\n", + MessageHandler::GetLocalizedMessage(this, message_obj).get()); + PrintCurrentStackTrace(stderr); + base::OS::Abort(); + } } } else if (location != NULL && !location->script().is_null()) { // We are bootstrapping and caught an error where the location is set @@ -1299,6 +1310,12 @@ void Isolate::SetCaptureStackTraceForUncaughtExceptions( } +void Isolate::SetAbortOnUncaughtException( + v8::Isolate::abort_on_uncaught_exception_t callback) { + abort_on_uncaught_exception_callback_ = callback; +} + + Handle Isolate::native_context() { return handle(context()->native_context()); } @@ -1474,7 +1491,8 @@ Isolate::Isolate() num_sweeper_threads_(0), stress_deopt_count_(0), next_optimization_id_(0), - use_counter_callback_(NULL) { + use_counter_callback_(NULL), + abort_on_uncaught_exception_callback_(NULL) { id_ = base::NoBarrier_AtomicIncrement(&isolate_counter_, 1); TRACE_ISOLATE(constructor); diff --git a/deps/v8/src/isolate.h b/deps/v8/src/isolate.h index eae5cad28..c621fcc2f 100644 --- a/deps/v8/src/isolate.h +++ b/deps/v8/src/isolate.h @@ -707,6 +707,9 @@ class Isolate { int frame_limit, StackTrace::StackTraceOptions options); + typedef bool (*abort_on_uncaught_exception_t)(v8::Isolate*); + void SetAbortOnUncaughtException(abort_on_uncaught_exception_t callback); + void PrintCurrentStackTrace(FILE* out); void PrintStack(StringStream* accumulator); void PrintStack(FILE* out); @@ -1331,6 +1334,8 @@ class Isolate { v8::Isolate::UseCounterCallback use_counter_callback_; + abort_on_uncaught_exception_t abort_on_uncaught_exception_callback_; + friend class ExecutionAccess; friend class HandleScopeImplementer; friend class IsolateInitializer; diff --git a/lib/domain.js b/lib/domain.js index 5924b44eb..87a47076d 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -77,6 +77,20 @@ Domain.prototype._disposed = undefined; // Called by process._fatalException in case an error was thrown. Domain.prototype._errorHandler = function errorHandler(er) { var caught = false; + var self = this; + + function emitError() { + var handled = self.emit('error', er); + + // Exit all domains on the stack. Uncaught exceptions end the + // current tick and no domains should be left on the stack + // between ticks. + stack.length = 0; + exports.active = process.domain = null; + + return handled; + } + // ignore errors on disposed domains. // // XXX This is a bit stupid. We should probably get rid of @@ -89,38 +103,54 @@ Domain.prototype._errorHandler = function errorHandler(er) { er.domain = this; er.domainThrown = true; } - // wrap this in a try/catch so we don't get infinite throwing - try { - // One of three things will happen here. - // - // 1. There is a handler, caught = true - // 2. There is no handler, caught = false - // 3. It throws, caught = false - // - // If caught is false after this, then there's no need to exit() - // the domain, because we're going to crash the process anyway. - caught = this.emit('error', er); - // Exit all domains on the stack. Uncaught exceptions end the - // current tick and no domains should be left on the stack - // between ticks. - stack.length = 0; - exports.active = process.domain = null; - } catch (er2) { - // The domain error handler threw! oh no! - // See if another domain can catch THIS error, - // or else crash on the original one. - // If the user already exited it, then don't double-exit. - if (this === exports.active) { - stack.pop(); + // The top-level domain-handler is handled separately. + // + // The reason is that if V8 was passed a command line option + // asking it to abort on an uncaught exception (currently + // "--abort-on-uncaught-exception"), we want an uncaught exception + // in the top-level domain error handler to make the + // process abort. Using try/catch here would always make V8 think + // that these exceptions are caught, and thus would prevent it from + // aborting in these cases. + if (stack.length === 1) { + try { + // Set the _emittingTopLevelDomainError so that we know that, even + // if technically the top-level domain is still active, it would + // be ok to abort on an uncaught exception at this point + process._emittingTopLevelDomainError = true; + caught = emitError(); + } finally { + process._emittingTopLevelDomainError = false; } - if (stack.length) { - exports.active = process.domain = stack[stack.length - 1]; - caught = process._fatalException(er2); - } else { - caught = false; + } else { + // wrap this in a try/catch so we don't get infinite throwing + try { + // One of three things will happen here. + // + // 1. There is a handler, caught = true + // 2. There is no handler, caught = false + // 3. It throws, caught = false + // + // If caught is false after this, then there's no need to exit() + // the domain, because we're going to crash the process anyway. + caught = emitError(); + } catch (er2) { + // The domain error handler threw! oh no! + // See if another domain can catch THIS error, + // or else crash on the original one. + // If the user already exited it, then don't double-exit. + if (this === exports.active) { + stack.pop(); + } + if (stack.length) { + exports.active = process.domain = stack[stack.length - 1]; + caught = process._fatalException(er2); + } else { + caught = false; + } + return caught; } - return caught; } return caught; }; diff --git a/src/env.h b/src/env.h index fb243b0eb..2768388f3 100644 --- a/src/env.h +++ b/src/env.h @@ -86,6 +86,7 @@ namespace node { V(dev_string, "dev") \ V(disposed_string, "_disposed") \ V(domain_string, "domain") \ + V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError") \ V(exchange_string, "exchange") \ V(idle_string, "idle") \ V(irq_string, "irq") \ diff --git a/src/node.cc b/src/node.cc index 1ad02e993..45193498d 100644 --- a/src/node.cc +++ b/src/node.cc @@ -909,6 +909,33 @@ Local WinapiErrnoException(Isolate* isolate, #endif +static bool IsDomainActive(const Environment* env) { + if (!env->using_domains()) { + return false; + } + + Local domain_array = env->domain_array().As(); + uint32_t domainsArrayLength = domain_array->Length(); + if (domainsArrayLength == 0) + return false; + + Local domain_v = domain_array->Get(0); + return !domain_v->IsNull(); +} + + +bool ShouldAbortOnUncaughtException(v8::Isolate* isolate) { + Environment* env = Environment::GetCurrent(isolate); + Local process_object = env->process_object(); + Local emitting_top_level_domain_error_key = + env->emitting_top_level_domain_error_string(); + bool isEmittingTopLevelDomainError = + process_object->Get(emitting_top_level_domain_error_key)->BooleanValue(); + + return !IsDomainActive(env) || isEmittingTopLevelDomainError; +} + + void SetupDomainUse(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args.GetIsolate()); @@ -2772,6 +2799,9 @@ void SetupProcessObject(Environment* env, // pre-set _events object for faster emit checks process->Set(env->events_string(), Object::New(env->isolate())); + + process->Set(env->emitting_top_level_domain_error_string(), + False(env->isolate())); } @@ -3453,6 +3483,8 @@ void Init(int* argc, node_isolate = Isolate::New(); Isolate::Scope isolate_scope(node_isolate); + node_isolate->SetAbortOnUncaughtException(ShouldAbortOnUncaughtException); + #ifdef __POSIX__ // Raise the open file descriptor limit. { // NOLINT (whitespace/braces) diff --git a/test/simple/test-domain-top-level-error-handler-throw.js b/test/simple/test-domain-top-level-error-handler-throw.js new file mode 100644 index 000000000..6d4b69c19 --- /dev/null +++ b/test/simple/test-domain-top-level-error-handler-throw.js @@ -0,0 +1,74 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +/* + * The goal of this test is to make sure that when a top-level error + * handler throws an error following the handling of a previous error, + * the process reports the error message from the error thrown in the + * top-level error handler, not the one from the previous error. + */ + +var domainErrHandlerExMessage = 'exception from domain error handler'; +var internalExMessage = 'You should NOT see me'; + +if (process.argv[2] === 'child') { + var domain = require('domain'); + var d = domain.create(); + + d.on('error', function() { + throw new Error(domainErrHandlerExMessage); + }); + + d.run(function doStuff() { + process.nextTick(function () { + throw new Error(internalExMessage); + }); + }); +} else { + var fork = require('child_process').fork; + var assert = require('assert'); + + function test() { + var child = fork(process.argv[1], ['child'], {silent:true}); + var gotDataFromStderr = false; + var stderrOutput = ''; + if (child) { + child.stderr.on('data', function onStderrData(data) { + gotDataFromStderr = true; + stderrOutput += data.toString(); + }); + + child.on('exit', function onChildExited(exitCode, signal) { + assert(gotDataFromStderr); + assert(stderrOutput.indexOf(domainErrHandlerExMessage) !== -1); + assert(stderrOutput.indexOf(internalExMessage) === -1); + + var expectedExitCode = 7; + var expectedSignal = null; + + assert.equal(exitCode, expectedExitCode); + assert.equal(signal, expectedSignal); + }); + } + } + + test(); +} diff --git a/test/simple/test-domain-with-abort-on-uncaught-exception.js b/test/simple/test-domain-with-abort-on-uncaught-exception.js new file mode 100644 index 000000000..d1ac09bf5 --- /dev/null +++ b/test/simple/test-domain-with-abort-on-uncaught-exception.js @@ -0,0 +1,229 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var assert = require('assert'); + +/* + * The goal of this test is to make sure that: + * + * - Even if --abort_on_uncaught_exception is passed on the command line, + * setting up a top-level domain error handler and throwing an error + * within this domain does *not* make the process abort. The process exits + * gracefully. + * + * - When passing --abort_on_uncaught_exception on the command line and + * setting up a top-level domain error handler, an error thrown + * within this domain's error handler *does* make the process abort. + * + * - When *not* passing --abort_on_uncaught_exception on the command line and + * setting up a top-level domain error handler, an error thrown within this + * domain's error handler does *not* make the process abort, but makes it exit + * with the proper failure exit code. + * + * - When throwing an error within the top-level domain's error handler + * within a try/catch block, the process should exit gracefully, whether or + * not --abort_on_uncaught_exception is passed on the command line. + */ + +var domainErrHandlerExMessage = 'exception from domain error handler'; + +if (process.argv[2] === 'child') { + var domain = require('domain'); + var d = domain.create(); + var triggeredProcessUncaughtException = false; + + process.on('uncaughtException', function onUncaughtException() { + // The process' uncaughtException event must not be emitted when + // an error handler is setup on the top-level domain. + // Exiting with exit code of 42 here so that it would assert when + // the parent checks the child exit code. + process.exit(42); + }); + + d.on('error', function(err) { + // Swallowing the error on purpose if 'throwInDomainErrHandler' is not + // set + if (process.argv.indexOf('throwInDomainErrHandler') !== -1) { + if (process.argv.indexOf('useTryCatch') !== -1) { + try { + throw new Error(domainErrHandlerExMessage); + } catch (e) { + } + } else { + throw new Error(domainErrHandlerExMessage); + } + } + }); + + d.run(function doStuff() { + // Throwing from within different types of callbacks as each of them + // handles domains differently + process.nextTick(function () { + throw new Error("Error from nextTick callback"); + }); + + var fs = require('fs'); + fs.exists('/non/existing/file', function onExists(exists) { + throw new Error("Error from fs.exists callback"); + }); + + setImmediate(function onSetImmediate() { + throw new Error("Error from setImmediate callback"); + }); + + throw new Error("Error from domain.run callback"); + }); +} else { + var exec = require('child_process').exec; + + function testDomainExceptionHandling(cmdLineOption, options) { + if (typeof cmdLineOption === 'object') { + options = cmdLineOption; + cmdLineOption = undefined; + } + + var throwInDomainErrHandlerOpt; + if (options.throwInDomainErrHandler) + throwInDomainErrHandlerOpt = 'throwInDomainErrHandler'; + + var cmdToExec = ''; + if (process.platform !== 'win32') { + // Do not create core files, as it can take a lot of disk space on + // continuous testing and developers' machines + cmdToExec += 'ulimit -c 0 && '; + } + + var useTryCatchOpt; + if (options.useTryCatch) + useTryCatchOpt = 'useTryCatch'; + + cmdToExec += process.argv[0] + ' '; + cmdToExec += (cmdLineOption ? cmdLineOption : '') + ' '; + cmdToExec += process.argv[1] + ' '; + cmdToExec += [ + 'child', + throwInDomainErrHandlerOpt, + useTryCatchOpt + ].join(' '); + + var child = exec(cmdToExec); + + if (child) { + var childTriggeredOnUncaughtExceptionHandler = false; + child.on('message', function onChildMsg(msg) { + if (msg === 'triggeredProcessUncaughtEx') { + childTriggeredOnUncaughtExceptionHandler = true; + } + }); + + child.on('exit', function onChildExited(exitCode, signal) { + var expectedExitCode = 0; + // We use an array of values since the actual signal can differ across + // compilers. + var expectedSignal = [null]; + + // When throwing errors from the top-level domain error handler + // outside of a try/catch block, the process should not exit gracefully + if (!options.useTryCatch && options.throwInDomainErrHandler) { + // If the top-level domain's error handler does not throw, + // the process must exit gracefully, whether or not + // --abort_on_uncaught_exception was passed on the command line + expectedExitCode = 7; + if (cmdLineOption === '--abort_on_uncaught_exception') { + // If the top-level domain's error handler throws, and only if + // --abort_on_uncaught_exception is passed on the command line, + // the process must abort. + // + // We use an array of values since the actual exit code can differ + // across compilers. + expectedExitCode = [132, 134]; + + // On Linux, v8 raises SIGTRAP when aborting because + // the "debug break" flag is on by default + if (process.platform === 'linux') + expectedExitCode.push(133); + + // On some platforms with KSH being the default shell + // (like SmartOS), when a process aborts, KSH exits with an exit + // code that is greater than 256, and thus the exit code emitted + // with the 'exit' event is null and the signal is set to either + // SIGABRT or SIGILL. + if (process.platform === 'sunos') { + expectedExitCode = null; + expectedSignal = ['SIGABRT', 'SIGILL']; + } + + // On Windows, v8's base::OS::Abort also triggers a debug breakpoint + // which makes the process exit with code -2147483645 + if (process.platform === 'win32') + expectedExitCode = [-2147483645, 3221225477]; + } + } + + if (Array.isArray(expectedSignal)) { + assert.ok(expectedSignal.indexOf(signal) > -1); + } else { + assert.equal(signal, expectedSignal); + } + + if (Array.isArray(expectedExitCode)) { + assert.ok(expectedExitCode.indexOf(exitCode) > -1); + } else { + assert.equal(exitCode, expectedExitCode); + } + }); + } + } + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: false, + useTryCatch: false + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: false, + useTryCatch: true + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: true, + useTryCatch: false + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: true, + useTryCatch: true + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: false + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: false, + useTryCatch: false + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: true, + useTryCatch: true + }); +} -- cgit v1.2.1