domains: fix handling of uncaught exceptions

lib/domain.js

@@ -32,19 +32,20 @@ var endMethods = ['end', 'abort', 'destroy', 'destroySoon'];
 // a few side effects.
 events.usingDomains = true;
 
+// it's possible to enter one domain while already inside
+// another one.  the stack is each entered domain.
+var stack = [];
+exports._stack = stack;
+
 // let the process know we're using domains
-process._usingDomains();
+process._usingDomains(stack);
 
 exports.Domain = Domain;
 
 exports.create = exports.createDomain = function(cb) {
   return new Domain(cb);
 };
 
-// it's possible to enter one domain while already inside
-// another one.  the stack is each entered domain.
-var stack = [];
-exports._stack = stack;
 // the active domain is always the one that we're currently in.
 exports.active = null;
 

src/node.cc

@@ -105,6 +105,8 @@ Persistent<String> domain_symbol;
 // declared in node_internals.h
 Persistent<Object> process;
 
+static Persistent<Array> domains_stack;
+
 static Persistent<Function> process_tickFromSpinner;
 static Persistent<Function> process_tickCallback;
 
@@ -126,6 +128,8 @@ static Persistent<String> exit_symbol;
 static Persistent<String> disposed_symbol;
 
 static Persistent<String> emitting_toplevel_domain_error_symbol;
+static Persistent<String> _events_symbol;
+static Persistent<String> error_symbol;
 
 static bool print_eval = false;
 static bool force_repl = false;
@@ -904,25 +908,77 @@ Handle<Value> FromConstructorTemplate(Persistent<FunctionTemplate> t,
   return scope.Close(t->GetFunction()->NewInstance(argc, argv));
 }
 
-static bool IsDomainActive() {
-  if (domain_symbol.IsEmpty())
-    domain_symbol = NODE_PSYMBOL("domain");
+static bool DomainHasErrorHandler(const Local<Object>& domain) {
+  HandleScope scope;
+
+  if (_events_symbol.IsEmpty())
+    _events_symbol = NODE_PSYMBOL("_events");
+
+  Local<Value> domain_event_listeners_v = domain->Get(_events_symbol);
+  if (!domain_event_listeners_v->IsObject())
+    return false;
+
+  Local<Object> domain_event_listeners_o =
+      domain_event_listeners_v.As<Object>();
+
+  if (error_symbol.IsEmpty())
+    error_symbol = NODE_PSYMBOL("error");
+
+  Local<Value> domain_error_listeners_v =
+      domain_event_listeners_o->Get(error_symbol);
+
+  if (domain_error_listeners_v->IsFunction() ||
+      (domain_error_listeners_v->IsArray() &&
+      domain_error_listeners_v.As<Array>()->Length() > 0))
+    return true;
+
+  return false;
+}
+
+static bool TopDomainHasErrorHandler() {
+  HandleScope scope;
+
+  if (!using_domains)
+    return false;
 
-  Local<Value> domain_v = process->Get(domain_symbol);
+  Local<Array> domains_stack_array = Local<Array>::New(domains_stack);
+  if (domains_stack_array->Length() == 0)
+    return false;
 
-  return domain_v->IsObject();
+  uint32_t domains_stack_length = domains_stack_array->Length();
+  if (domains_stack_length == 0)
+    return false;
+
+  Local<Value> domain_v = domains_stack_array->Get(domains_stack_length - 1);
+  if (!domain_v->IsObject())
+    return false;
+
+  Local<Object> domain = domain_v.As<Object>();
+  if (DomainHasErrorHandler(domain))
+    return true;
+
+  return false;
 }
 
 bool ShouldAbortOnUncaughtException() {
   Local<Value> emitting_toplevel_domain_error_v =
     process->Get(emitting_toplevel_domain_error_symbol);
-  return !IsDomainActive() || emitting_toplevel_domain_error_v->BooleanValue();
+
+  return emitting_toplevel_domain_error_v->BooleanValue() ||
+    !TopDomainHasErrorHandler();
 }
 
 Handle<Value> UsingDomains(const Arguments& args) {
   HandleScope scope;
+
   if (using_domains)
     return scope.Close(Undefined());
+
+  if (!args[0]->IsArray()) {
+    fprintf(stderr, "domains stack must be an array\n");
+    abort();
+  }
+
   using_domains = true;
   Local<Value> tdc_v = process->Get(String::New("_tickDomainCallback"));
   Local<Value> ndt_v = process->Get(String::New("_nextDomainTick"));
@@ -934,6 +990,9 @@ Handle<Value> UsingDomains(const Arguments& args) {
     fprintf(stderr, "process._nextDomainTick assigned to non-function\n");
     abort();
   }
+
+  domains_stack = Persistent<Array>::New(args[0].As<Array>());
+
   Local<Function> tdc = tdc_v.As<Function>();
   Local<Function> ndt = ndt_v.As<Function>();
   process->Set(String::New("_tickCallback"), tdc);
@@ -2449,7 +2508,10 @@ Handle<Object> SetupProcessObject(int argc, char *argv[]) {
   process->Set(String::NewSymbol("_tickInfoBox"), info_box);
 
   // pre-set _events object for faster emit checks
-  process->Set(String::NewSymbol("_events"), Object::New());
+  if (_events_symbol.IsEmpty())
+    _events_symbol = NODE_PSYMBOL("_events");
+
+  process->Set(_events_symbol, Object::New());
 
   if (emitting_toplevel_domain_error_symbol.IsEmpty())
     emitting_toplevel_domain_error_symbol =

src/node.js

@@ -229,6 +229,7 @@
     // and exit if there are no listeners.
     process._fatalException = function(er) {
       var caught = false;
+
       if (process.domain) {
         var domain = process.domain;
         var domainModule = NativeModule.require('domain');
@@ -256,11 +257,17 @@
         // 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);
+            // If there's no error handler, do not emit an 'error' event
+            // as this would throw an error, make the process exit, and thus
+            // prevent the process 'uncaughtException' event from being emitted
+            // if a listener is set.
+            if (process.EventEmitter.listenerCount(domain, 'error') > 0) {
+              // 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;
           }
@@ -297,9 +304,12 @@
         // current tick and no domains should be left on the stack
         // between ticks.
         _clearDomainsStack();
-      } else {
+      }
+
+      if (!caught) {
         caught = process.emit('uncaughtException', er);
       }
+
       // if someone handled it, then great.  otherwise, die in C++ land
       // since that means that we'll exit the process, emit the 'exit' event
       if (!caught) {

test/common.js

@@ -226,3 +226,37 @@ exports.busyLoop = function busyLoop(time) {
     ;
   }
 };
+
+// Returns true if the exit code "exitCode" and/or signal name "signal"
+// represent the exit code and/or signal name of a node process that aborted,
+// false otherwise.
+exports.nodeProcessAborted = function nodeProcessAborted(exitCode, signal) {
+  // Depending on the compiler used, node will exit with either
+  // exit code 132 (SIGILL), 133 (SIGTRAP) or 134 (SIGABRT).
+  var expectedExitCodes = [132, 133, 134];
+
+  // On platforms using KSH as 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 SIGILL, SIGTRAP,
+  // or SIGABRT (depending on the compiler).
+  var expectedSignals = ['SIGILL', 'SIGTRAP', 'SIGABRT'];
+
+  // On Windows, v8's base::OS::Abort triggers a debug breakpoint,
+  // which makes the process exit with code -2147483645.
+  if (process.platform === 'win32')
+    expectedExitCodes = [-2147483645];
+
+  // When using --abort-on-uncaught-exception, V8 will use
+  // base::OS::Abort to terminate the process.
+  // Depending on the compiler used, the shell or other aspects of
+  // the platform used to build the node binary, this will actually
+  // make V8 exit by aborting or by raising a signal. In any case,
+  // one of them (exit code or signal) needs to be set to one of
+  // the expected exit codes or signals.
+  if (signal !== null) {
+    return expectedSignals.indexOf(signal) > -1;
+  } else {
+    return expectedExitCodes.indexOf(exitCode) > -1;
+  }
+};