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 nodejs#3607 and nodejs#3653.

PR: nodejs#3885
PR-URL: nodejs/node#3885
Reviewed-By: James M Snell <[email protected]>

src/node.cc

@@ -1072,21 +1072,6 @@ static bool TopDomainHasErrorHandler(const Environment* env) {
 }
 
 
-static bool IsDomainActive(const Environment* env) {
-  if (!env->using_domains()) {
-    return false;
-  }
-
-  Local<Array> domain_array = env->domain_array().As<Array>();
-  uint32_t domains_array_length = domain_array->Length();
-  if (domains_array_length == 0)
-    return false;
-
-  Local<Value> domain_v = domain_array->Get(0);
-  return !domain_v->IsNull();
-}
-
-
 bool ShouldAbortOnUncaughtException(v8::Isolate* isolate) {
   Environment* env = Environment::GetCurrent(isolate);
   Local<Object> process_object = env->process_object();
@@ -1095,7 +1080,7 @@ bool ShouldAbortOnUncaughtException(v8::Isolate* isolate) {
   bool isEmittingTopLevelDomainError =
     process_object->Get(emitting_top_level_domain_error_key)->BooleanValue();
 
-  return !IsDomainActive(env) || isEmittingTopLevelDomainError;
+  return isEmittingTopLevelDomainError || !TopDomainHasErrorHandler(env);
 }
 
 

test/common.js

@@ -371,8 +371,10 @@ exports.nodeProcessAborted = function nodeProcessAborted(exitCode, signal) {
   // which corresponds to exit code 3221225477 (0xC0000005)
   if (process.platform === 'win32')
     expectedExitCodes = [3221225477];
-  else if (process.platform === 'os390')
-    expectedExitCodes = [132, 154, 131];
+  else if (process.platform === 'os390') {
+    expectedExitCodes = [137];
+    expectedSignals = ['SIGKILL'];
+  }
 
   // When using --abort-on-uncaught-exception, V8 will use
   // base::OS::Abort to terminate the process.

test/simple/test-domain-with-abort-on-uncaught-exception.js

@@ -21,6 +21,8 @@
 
 var assert = require('assert');
 
+var common = require('../common');
+
 /*
  * The goal of this test is to make sure that:
  *
@@ -139,62 +141,27 @@ if (process.argv[2] === 'child') {
       });
 
       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);
-
-            if (process.platform === 'os390')
-              expectedExitCode = [ 137 ];
-
-            // 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];
+            assert(common.nodeProcessAborted(exitCode, signal),
+                'process should have aborted, but did not');
+          } else {
+            // By default, uncaught exceptions make node exit with an exit
+            // code of 7.
+            assert.equal(exitCode, 7);
+            assert.equal(signal, null);
           }
-        }
-
-        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);
+          // 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
+          assert.equal(exitCode, 0);
+          assert.equal(signal, null);
         }
       });
     }