domains: fix handling of uncaught exceptions

Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Fixes #3607 and #3653.

PR: #3884
PR-URL: #3884
Reviewed-By: James M Snell <[email protected]>

Author: Julien Gilli

lib/domain.js

@@ -27,19 +27,20 @@ Object.defineProperty(process, 'domain', {
   }
 });
 
+// It's possible to enter one domain while already inside
+// another one.  the stack is each entered domain.
+const stack = [];
+exports._stack = stack;
+
 // let the process know we're using domains
-const _domain_flag = process._setupDomainUse(_domain);
+const _domain_flag = process._setupDomainUse(_domain, stack);
 
 exports.Domain = Domain;
 
 exports.create = exports.createDomain = function() {
   return new Domain();
 };
 
-// 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;
 
@@ -96,14 +97,20 @@ Domain.prototype._errorHandler = function errorHandler(er) {
   // 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 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 (EventEmitter.listenerCount(self, 'error') > 0) {
+      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;
+      }
     }
   } else {
     // wrap this in a try/catch so we don't get infinite throwing

src/env.h

@@ -236,6 +236,7 @@ namespace node {
   V(buffer_prototype_object, v8::Object)                                      \
   V(context, v8::Context)                                                     \
   V(domain_array, v8::Array)                                                  \
+  V(domains_stack_array, v8::Array)                                           \
   V(fs_stats_constructor_function, v8::Function)                              \
   V(generic_internal_field_template, v8::ObjectTemplate)                      \
   V(jsstream_constructor_template, v8::FunctionTemplate)                      \

src/node.cc

@@ -947,17 +947,53 @@ void* ArrayBufferAllocator::Allocate(size_t size) {
   return malloc(size);
 }
 
+static bool DomainHasErrorHandler(const Environment* env,
+                                  const Local<Object>& domain) {
+  HandleScope scope(env->isolate());
+
+  Local<Value> domain_event_listeners_v = domain->Get(env->events_string());
+  if (!domain_event_listeners_v->IsObject())
+    return false;
+
+  Local<Object> domain_event_listeners_o =
+      domain_event_listeners_v.As<Object>();
+
+  Local<Value> domain_error_listeners_v =
+      domain_event_listeners_o->Get(env->error_string());
+
+  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(const Environment* env) {
+  HandleScope scope(env->isolate());
 
-static bool IsDomainActive(const Environment* env) {
   if (!env->using_domains())
     return false;
 
-  Local<Array> domain_array = env->domain_array().As<Array>();
-  if (domain_array->Length() == 0)
+  Local<Array> domains_stack_array = env->domains_stack_array().As<Array>();
+  if (domains_stack_array->Length() == 0)
+    return false;
+
+  uint32_t domains_stack_length = domains_stack_array->Length();
+  if (domains_stack_length == 0)
+    return false;
+
+  Local<Value> top_domain_v =
+      domains_stack_array->Get(domains_stack_length - 1);
+
+  if (!top_domain_v->IsObject())
     return false;
 
-  Local<Value> domain_v = domain_array->Get(0);
-  return !domain_v->IsNull();
+  Local<Object> top_domain = top_domain_v.As<Object>();
+  if (DomainHasErrorHandler(env, top_domain))
+    return true;
+
+  return false;
 }
 
 
@@ -971,7 +1007,7 @@ static bool ShouldAbortOnUncaughtException(Isolate* isolate) {
   bool isEmittingTopLevelDomainError =
       process_object->Get(emitting_top_level_domain_error_key)->BooleanValue();
 
-  return !IsDomainActive(env) || isEmittingTopLevelDomainError;
+  return isEmittingTopLevelDomainError || !TopDomainHasErrorHandler(env);
 }
 
 
@@ -1000,6 +1036,9 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsArray());
   env->set_domain_array(args[0].As<Array>());
 
+  CHECK(args[1]->IsArray());
+  env->set_domains_stack_array(args[1].As<Array>());
+
   // Do a little housekeeping.
   env->process_object()->Delete(
       FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupDomainUse"));

test/common.js

@@ -449,3 +449,37 @@ exports.fileExists = function(pathname) {
 exports.fail = function(msg) {
   assert.fail(null, null, msg);
 };
+
+// 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).
+  const expectedSignals = ['SIGILL', 'SIGTRAP', 'SIGABRT'];
+
+  // On Windows, v8's base::OS::Abort triggers an access violation,
+  // which corresponds to exit code 3221225477 (0xC0000005)
+  if (process.platform === 'win32')
+    expectedExitCodes = [3221225477];
+
+  // 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;
+  }
+};