From 33c642c1d19a18ff7e1b7437bab855e31777c163 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Wed, 29 Oct 2014 19:55:16 -0700 Subject: [PATCH] domains: fix issues with abort on uncaught 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 #8631. Also fixes #8630. --- deps/v8/include/v8.h | 3 +- deps/v8/src/api.cc | 4 + deps/v8/src/isolate.cc | 9 +- deps/v8/src/isolate.h | 5 + src/node.cc | 30 +++ src/node.js | 87 ++++++--- ...st-domain-top-level-error-handler-throw.js | 74 ++++++++ ...domain-with-abort-on-uncaught-exception.js | 176 ++++++++++++++++++ 8 files changed, 354 insertions(+), 34 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 71a0d01eba57..c4d07cfbf15f 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -2856,7 +2856,8 @@ class V8EXPORT Isolate { * restored when exiting. Re-entering an isolate is allowed. */ void Enter(); - + typedef bool (*should_abort_on_uncaught_exception_handler_t)(); + void SetShouldAbortOnUncaughtExceptionHandler(should_abort_on_uncaught_exception_handler_t handler); /** * Exits this isolate by restoring the previously entered one in the * current thread. The isolate may still stay the same, if it was diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc index 96d564ff8554..e708f2560111 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -5565,6 +5565,10 @@ void Isolate::Enter() { isolate->Enter(); } +void Isolate::SetShouldAbortOnUncaughtExceptionHandler(should_abort_on_uncaught_exception_handler_t handler) { + i::Isolate* isolate = reinterpret_cast(this); + isolate->SetShouldAbortOnUncaughtExceptionHandler(handler); +} void Isolate::Exit() { i::Isolate* isolate = reinterpret_cast(this); diff --git a/deps/v8/src/isolate.cc b/deps/v8/src/isolate.cc index 5a5293eb7140..a83cdb400cdf 100644 --- a/deps/v8/src/isolate.cc +++ b/deps/v8/src/isolate.cc @@ -1158,7 +1158,8 @@ void Isolate::DoThrow(Object* exception, MessageLocation* location) { // print a user-friendly stack trace (not an internal one). if (fatal_exception_depth == 0 && FLAG_abort_on_uncaught_exception && - (report_exception || can_be_caught_externally)) { + (report_exception || can_be_caught_externally) && + (!should_abort_on_uncaught_exception_handler_ || should_abort_on_uncaught_exception_handler_())) { fatal_exception_depth++; fprintf(stderr, "%s\n\nFROM\n", *MessageHandler::GetLocalizedMessage(message_obj)); @@ -1339,6 +1340,9 @@ void Isolate::SetCaptureStackTraceForUncaughtExceptions( stack_trace_for_uncaught_exceptions_options_ = options; } +void Isolate::SetShouldAbortOnUncaughtExceptionHandler(v8::Isolate::should_abort_on_uncaught_exception_handler_t handler) { + should_abort_on_uncaught_exception_handler_ = handler; +} bool Isolate::is_out_of_memory() { if (has_pending_exception()) { @@ -1534,7 +1538,8 @@ Isolate::Isolate() date_cache_(NULL), context_exit_happened_(false), deferred_handles_head_(NULL), - optimizing_compiler_thread_(this) { + optimizing_compiler_thread_(this), + should_abort_on_uncaught_exception_handler_(0) { TRACE_ISOLATE(constructor); memset(isolate_addresses_, 0, diff --git a/deps/v8/src/isolate.h b/deps/v8/src/isolate.h index 2769ca7c9ac4..d34290d20e95 100644 --- a/deps/v8/src/isolate.h +++ b/deps/v8/src/isolate.h @@ -692,6 +692,9 @@ class Isolate { int frame_limit, StackTrace::StackTraceOptions options); + typedef bool (*should_abort_on_uncaught_exception_handler_t)(); + void SetShouldAbortOnUncaughtExceptionHandler(should_abort_on_uncaught_exception_handler_t handler); + // Tells whether the current context has experienced an out of memory // exception. bool is_out_of_memory(); @@ -1292,6 +1295,8 @@ class Isolate { DeferredHandles* deferred_handles_head_; OptimizingCompilerThread optimizing_compiler_thread_; + should_abort_on_uncaught_exception_handler_t should_abort_on_uncaught_exception_handler_; + friend class ExecutionAccess; friend class HandleScopeImplementer; friend class IsolateInitializer; diff --git a/src/node.cc b/src/node.cc index 18c743fc2bea..7283b67b466f 100644 --- a/src/node.cc +++ b/src/node.cc @@ -126,6 +126,7 @@ static Persistent enter_symbol; static Persistent exit_symbol; static Persistent disposed_symbol; +static Persistent emitting_toplevel_domain_error_symbol; static bool print_eval = false; static bool force_repl = false; @@ -904,6 +905,29 @@ Handle FromConstructorTemplate(Persistent t, return scope.Close(t->GetFunction()->NewInstance(argc, argv)); } +static bool IsDomainActive() { + if (domain_symbol.IsEmpty()) + domain_symbol = NODE_PSYMBOL("domain"); + + Local domain_v = process->Get(domain_symbol); + + bool has_domain = domain_v->IsObject(); + if (has_domain) { + Local domain = domain_v->ToObject(); + assert(!domain.IsEmpty()); + if (!domain->IsNull()) { + return true; + } + } + + return false; +} + +bool ShouldAbortOnUncaughtException() { + Local emitting_toplevel_domain_error_v = + process->Get(emitting_toplevel_domain_error_symbol); + return !IsDomainActive() || emitting_toplevel_domain_error_v->BooleanValue(); +} Handle UsingDomains(const Arguments& args) { HandleScope scope; @@ -2437,6 +2461,11 @@ Handle SetupProcessObject(int argc, char *argv[]) { // pre-set _events object for faster emit checks process->Set(String::NewSymbol("_events"), Object::New()); + if (emitting_toplevel_domain_error_symbol.IsEmpty()) + emitting_toplevel_domain_error_symbol = + NODE_PSYMBOL("_emittingTopLevelDomainError"); + process->Set(emitting_toplevel_domain_error_symbol, False()); + return process; } @@ -2959,6 +2988,7 @@ char** Init(int argc, char *argv[]) { // Fetch a reference to the main isolate, so we have a reference to it // even when we need it to access it from another (debugger) thread. node_isolate = Isolate::GetCurrent(); + node_isolate->SetShouldAbortOnUncaughtExceptionHandler(ShouldAbortOnUncaughtException); // If the --debug flag was specified then initialize the debug thread. if (use_debug_agent) { diff --git a/src/node.js b/src/node.js index b54ff51752a9..6d5c49230843 100644 --- a/src/node.js +++ b/src/node.js @@ -236,37 +236,62 @@ er.domain = domain; 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 = domain.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. - var domainModule = NativeModule.require('domain'); - domainStack.length = 0; - domainModule.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 (domain === domainModule.active) - domainStack.pop(); - if (domainStack.length) { - var parentDomain = domainStack[domainStack.length - 1]; - process.domain = domainModule.active = parentDomain; - caught = process._fatalException(er2); - } else - caught = false; + + // 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 (domainStack.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 = domain.emit('error', er); + } finally { + process._emittingTopLevelDomainError = 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 = domain.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. + var domainModule = NativeModule.require('domain'); + domainStack.length = 0; + domainModule.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 (domain === domainModule.active) + domainStack.pop(); + + if (domainStack.length) { + var parentDomain = domainStack[domainStack.length - 1]; + process.domain = domainModule.active = parentDomain; + caught = process._fatalException(er2); + } else { + domainModule.active = process.domain = null; + caught = false; + } + } } } else { caught = process.emit('uncaughtException', er); 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 000000000000..25b261f17088 --- /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 000000000000..6743cd334191 --- /dev/null +++ b/test/simple/test-domain-with-abort-on-uncaught-exception.js @@ -0,0 +1,176 @@ +// 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() { + process.send('triggeredProcessUncaughtEx'); + }); + + d.on('error', function() { + // 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() { + process.nextTick(function () { + throw new Error("You should NOT see me"); + }); + }); +} else { + var fork = require('child_process').fork; + + function testDomainExceptionHandling(cmdLineOption, options) { + if (typeof cmdLineOption === 'object') { + options = cmdLineOption; + cmdLineOption = undefined; + } + + var forkOptions; + if (cmdLineOption) { + forkOptions = { execArgv: [cmdLineOption] }; + } + + var throwInDomainErrHandlerOpt; + if (options.throwInDomainErrHandler) + throwInDomainErrHandlerOpt = 'throwInDomainErrHandler'; + + var useTryCatchOpt; + if (options.useTryCatch) + useTryCatchOpt = 'useTryCatch'; + + var child = fork(process.argv[1], [ + 'child', + throwInDomainErrHandlerOpt, + useTryCatchOpt + ], + forkOptions); + + if (child) { + var childTriggeredOnUncaughtExceptionHandler = false; + child.on('message', function onChildMsg(msg) { + if (msg === 'triggeredProcessUncaughtEx') { + childTriggeredOnUncaughtExceptionHandler = true; + } + }); + + child.on('exit', function onChildExited(exitCode, signal) { + // The process' uncaughtException event must not be emitted when + // an error handler is setup on the top-level domain. + assert(childTriggeredOnUncaughtExceptionHandler === false, + "Process' uncaughtException should not be emitted when " + + "an error handler is set on the top-level domain"); + + // 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 + var expectedExitCode = 0; + var expectedSignal = null; + + // When not throwing errors from the top-level domain error handler + // or if throwing them within a try/catch block, the process + // should exit gracefully + if (!options.useTryCatch && options.throwInDomainErrHandler) { + 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. + expectedExitCode = null; + expectedSignal = 'SIGABRT'; + } + } + + assert.equal(exitCode, expectedExitCode); + assert.equal(signal, expectedSignal); + }); + } + } + + 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 + }); +}