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: https://github.com/joyent/node/issues/8631
Fixes: https://github.com/joyent/node/issues/8630
PR-URL: https://github.com/joyent/node/pull/8666
Reviewed-by: Trevor Norris <trev.norris@gmail.com>

src/node.cc

@@ -126,6 +126,7 @@ static Persistent<String> enter_symbol;
 static Persistent<String> exit_symbol;
 static Persistent<String> disposed_symbol;
 
+static Persistent<String> emitting_toplevel_domain_error_symbol;
 
 static bool print_eval = false;
 static bool force_repl = false;
@@ -904,6 +905,20 @@ 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");
+
+  Local<Value> domain_v = process->Get(domain_symbol);
+
+  return domain_v->IsObject();
+}
+
+bool ShouldAbortOnUncaughtException() {
+  Local<Value> emitting_toplevel_domain_error_v =
+    process->Get(emitting_toplevel_domain_error_symbol);
+  return !IsDomainActive() || emitting_toplevel_domain_error_v->BooleanValue();
+}
 
 Handle<Value> UsingDomains(const Arguments& args) {
   HandleScope scope;
@@ -2437,6 +2452,11 @@ Handle<Object> 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 +2979,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->SetAbortOnUncaughtException(ShouldAbortOnUncaughtException);
 
   // If the --debug flag was specified then initialize the debug thread.
   if (use_debug_agent) {

src/node.js

@@ -236,37 +236,60 @@
 
         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 {
+              caught = false;
+            }
+          }
         }
       } else {
         caught = process.emit('uncaughtException', er);

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();
+}

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

@@ -0,0 +1,215 @@
+// 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() {
+    // 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) {
+        // 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;
+        // 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 SIGABRT. For these platforms only,
+        // and when the test is expected to abort, check the actual signal
+        // with the expected signal instead of the exit code.
+        var expectedSignal;
+
+        // 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) {
+          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 = 134;
+
+            // On linux, v8 raises SIGTRAP when aborting because
+            // the "debug break" flag is on by default
+            if (process.platform === 'linux')
+              expectedExitCode = 133;
+
+            if (process.platform === 'sunos') {
+              expectedExitCode = null;
+              expectedSignal = 'SIGABRT';
+            }
+
+            // On Windows, v8's OS::Abort also triggers a debug breakpoint
+            // which makes the process exit with code -2147483645
+            if (process.platform === 'win32') {
+              expectedExitCode = -2147483645;
+            }
+          }
+        }
+
+        if (expectedSignal)
+          assert.equal(signal, expectedSignal)
+
+        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
+  });
+}