src: fix --abort-on-uncaught-exception

Revert 0af4c9ea7434e4f505dbe071357e4bc3b4ab2a8a, parts of
921f2de6cf999b5a4663615e37967b4269d755fe and port
https://github.com/nodejs/node-v0.x-archive/pull/25835 from v0.12 to
master so that node aborts at the right time when an error is thrown
and --abort-on-uncaught-exception is used.

Fixes #3035.

PR: #3036
PR-URL: https://github.com/nodejs/node/pull/3036
Reviewed-By: Ben Noordhuis <ben@strongloop.com>

lib/domain.js

@@ -59,6 +59,20 @@ Domain.prototype._disposed = undefined;
 // Called by process._fatalException in case an error was thrown.
 Domain.prototype._errorHandler = function errorHandler(er) {
   var caught = false;
+  var self = this;
+
+  function emitError() {
+    var handled = self.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.
+    stack.length = 0;
+    exports.active = process.domain = null;
+
+    return handled;
+  }
+
   // ignore errors on disposed domains.
   //
   // XXX This is a bit stupid.  We should probably get rid of
@@ -71,38 +85,54 @@ Domain.prototype._errorHandler = function errorHandler(er) {
     er.domain = this;
     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 = this.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.
-    stack.length = 0;
-    exports.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 (this === exports.active) {
-      stack.pop();
+  // 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 (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 (stack.length) {
-      exports.active = process.domain = stack[stack.length - 1];
-      caught = process._fatalException(er2);
-    } else {
-      caught = 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 = emitError();
+    } 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 (this === exports.active) {
+        stack.pop();
+      }
+      if (stack.length) {
+        exports.active = process.domain = stack[stack.length - 1];
+        caught = process._fatalException(er2);
+      } else {
+        caught = false;
+      }
+      return caught;
     }
-    return caught;
   }
   return caught;
 };

src/async-wrap.cc

@@ -168,7 +168,6 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   Local<Object> process = env()->process_object();
   Local<Object> domain;
   bool has_domain = false;
-  bool has_abort_on_uncaught_and_domains = false;
 
   if (env()->using_domains()) {
     Local<Value> domain_v = context->Get(env()->domain_string());
@@ -177,7 +176,6 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
       domain = domain_v.As<Object>();
       if (domain->Get(env()->disposed_string())->IsTrue())
         return Undefined(env()->isolate());
-      has_abort_on_uncaught_and_domains = env()->using_abort_on_uncaught_exc();
     }
   }
 
@@ -201,21 +199,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
     try_catch.SetVerbose(true);
   }
 
-  Local<Value> ret;
-
-  if (has_abort_on_uncaught_and_domains) {
-    Local<Value> fn = process->Get(env()->domain_abort_uncaught_exc_string());
-    if (fn->IsFunction()) {
-      Local<Array> special_context = Array::New(env()->isolate(), 2);
-      special_context->Set(0, context);
-      special_context->Set(1, cb);
-      ret = fn.As<Function>()->Call(special_context, argc, argv);
-    } else {
-      ret = cb->Call(context, argc, argv);
-    }
-  } else {
-    ret = cb->Call(context, argc, argv);
-  }
+  Local<Value> ret = cb->Call(context, argc, argv);
 
   if (try_catch.HasCaught()) {
     return Undefined(env()->isolate());

src/env-inl.h

@@ -208,7 +208,6 @@ inline Environment::Environment(v8::Local<v8::Context> context,
       isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)),
       timer_base_(uv_now(loop)),
       using_domains_(false),
-      using_abort_on_uncaught_exc_(false),
       using_asyncwrap_(false),
       printed_error_(false),
       trace_sync_io_(false),
@@ -341,14 +340,6 @@ inline uint64_t Environment::timer_base() const {
   return timer_base_;
 }
 
-inline bool Environment::using_abort_on_uncaught_exc() const {
-  return using_abort_on_uncaught_exc_;
-}
-
-inline void Environment::set_using_abort_on_uncaught_exc(bool value) {
-  using_abort_on_uncaught_exc_ = value;
-}
-
 inline bool Environment::using_domains() const {
   return using_domains_;
 }

src/env.h

@@ -69,7 +69,7 @@ namespace node {
   V(dev_string, "dev")                                                        \
   V(disposed_string, "_disposed")                                             \
   V(domain_string, "domain")                                                  \
-  V(domain_abort_uncaught_exc_string, "_makeCallbackAbortOnUncaught")         \
+  V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError")   \
   V(exchange_string, "exchange")                                              \
   V(idle_string, "idle")                                                      \
   V(irq_string, "irq")                                                        \
@@ -432,9 +432,6 @@ class Environment {
   inline ares_channel* cares_channel_ptr();
   inline ares_task_list* cares_task_list();
 
-  inline bool using_abort_on_uncaught_exc() const;
-  inline void set_using_abort_on_uncaught_exc(bool value);
-
   inline bool using_domains() const;
   inline void set_using_domains(bool value);
 
@@ -540,7 +537,6 @@ class Environment {
   ares_channel cares_channel_;
   ares_task_list cares_task_list_;
   bool using_domains_;
-  bool using_abort_on_uncaught_exc_;
   bool using_asyncwrap_;
   bool printed_error_;
   bool trace_sync_io_;

src/node.cc

@@ -124,7 +124,6 @@ static bool force_repl = false;
 static bool syntax_check_only = false;
 static bool trace_deprecation = false;
 static bool throw_deprecation = false;
-static bool abort_on_uncaught_exception = false;
 static bool trace_sync_io = false;
 static bool track_heap_objects = false;
 static const char* eval_string = nullptr;
@@ -907,6 +906,33 @@ void* ArrayBufferAllocator::Allocate(size_t size) {
 }
 
 
+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)
+    return false;
+
+  Local<Value> domain_v = domain_array->Get(0);
+  return !domain_v->IsNull();
+}
+
+
+static bool ShouldAbortOnUncaughtException(Isolate* isolate) {
+  HandleScope scope(isolate);
+
+  Environment* env = Environment::GetCurrent(isolate);
+  Local<Object> process_object = env->process_object();
+  Local<String> emitting_top_level_domain_error_key =
+    env->emitting_top_level_domain_error_string();
+  bool isEmittingTopLevelDomainError =
+      process_object->Get(emitting_top_level_domain_error_key)->BooleanValue();
+
+  return !IsDomainActive(env) || isEmittingTopLevelDomainError;
+}
+
+
 void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -2185,11 +2211,7 @@ void FatalException(Isolate* isolate,
 
   if (false == caught->BooleanValue()) {
     ReportException(env, error, message);
-    if (abort_on_uncaught_exception) {
-      ABORT();
-    } else {
-      exit(1);
-    }
+    exit(1);
   }
 }
 
@@ -3217,9 +3239,6 @@ static void ParseArgs(int* argc,
       track_heap_objects = true;
     } else if (strcmp(arg, "--throw-deprecation") == 0) {
       throw_deprecation = true;
-    } else if (strcmp(arg, "--abort-on-uncaught-exception") == 0 ||
-               strcmp(arg, "--abort_on_uncaught_exception") == 0) {
-      abort_on_uncaught_exception = true;
     } else if (strcmp(arg, "--v8-options") == 0) {
       new_v8_argv[new_v8_argc] = "--help";
       new_v8_argc += 1;
@@ -3923,8 +3942,10 @@ static void StartNodeInstance(void* arg) {
     Environment* env = CreateEnvironment(isolate, context, instance_data);
     array_buffer_allocator->set_env(env);
     Context::Scope context_scope(context);
-    if (instance_data->is_main())
-      env->set_using_abort_on_uncaught_exc(abort_on_uncaught_exception);
+
+    node_isolate->SetAbortOnUncaughtExceptionCallback(
+        ShouldAbortOnUncaughtException);
+
     // Start debug agent when argv has --debug
     if (instance_data->use_debug_agent())
       StartDebug(env, debug_wait_connect);

src/node.js

@@ -210,13 +210,6 @@
   };
 
   startup.processFatal = function() {
-    process._makeCallbackAbortOnUncaught = function() {
-      try {
-        return this[1].apply(this[0], arguments);
-      } catch (err) {
-        process._fatalException(err);
-      }
-    };
 
     process._fatalException = function(er) {
       var caught;

test/parallel/test-domain-top-level-error-handler-throw.js

@@ -0,0 +1,50 @@
+'use strict';
+
+/*
+ * 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.
+ */
+
+const common = require('../common');
+
+const domainErrHandlerExMessage = 'exception from domain error handler';
+const 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');
+
+  var child = fork(process.argv[1], ['child'], {silent:true});
+  var stderrOutput = '';
+  if (child) {
+    child.stderr.on('data', function onStderrData(data) {
+      stderrOutput += data.toString();
+    });
+
+    child.on('exit', function onChildExited(exitCode, signal) {
+      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/parallel/test-domain-with-abort-on-uncaught-exception.js

@@ -0,0 +1,220 @@
+'use strict';
+
+const assert = require('assert');
+const fs = require('fs');
+const common = require('../common');
+
+/*
+ * 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.
+ */
+
+const 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(err) {
+    // Swallowing the error on purpose if 'throwInDomainErrHandler' is not
+    // set
+    if (process.argv.indexOf('throwInDomainErrHandler') !== -1) {
+      // If useTryCatch is set, wrap the throw in a try/catch block.
+      // This is to make sure that a caught exception does not trigger
+      // an abort.
+      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');
+    });
+
+    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');
+    });
+
+    setTimeout(function onTimeout() {
+      throw new Error('Error from setTimeout callback');
+    }, 0);
+
+    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) {
+        var expectedExitCodes;
+        var expectedSignals;
+
+        // 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 (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.
+            // Depending on the compiler used, node will exit with either
+            // exit code 132 (SIGILL) or 134 (SIGABRT).
+            expectedExitCodes = [132, 134];
+
+            // On platforms using a non-GNU compiler, base::OS::Abort raises
+            // an illegal instruction signal.
+            // On platforms using a GNU compiler but 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.
+            expectedSignals = ['SIGABRT', 'SIGILL'];
+
+            // 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) {
+              assert.ok(expectedSignals.indexOf(signal) > -1);
+            } else {
+              assert.ok(expectedExitCodes.indexOf(exitCode) > -1);
+            }
+          } else {
+            // By default, uncaught exceptions make node exit with an exit
+            // code of 7.
+            assert.equal(exitCode, 7);
+            assert.equal(signal, null);
+          }
+        } else {
+          // 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);
+        }
+      });
+    }
+  }
+
+  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
+  });
+}