src: fix abort-on-uncaught-exception

This PR fixes 0af4c9e so that node
aborts at the right time when throwing an error and using
--abort-on-uncaught-exception.

Basically, it wraps most node internal callbacks with:

if (!domain || domain.emittingTopLevelError)
  runCallback();
else {
  try {
    runCallback();
  } catch (err) {
    process._fatalException(err);
  }
}

so that V8 can abort properly in Isolate::Throw if
--abort-on-uncaught-exception was passed on the command line, and domain
can handle the error if one is active and not already in the top level
domain's error handler.

It also reverts 921f2de partially:
node::FatalException does not abort anymore because at that time, it's
already too late.

It adds process._forceTickDone, which is really a hack to allow
test-next-tick-error-spin.js to pass. It's here to basically avoid an
infinite recursion when throwing in a domain from a nextTick callback,
and queuing the same callback on the next tick from the domain's error
handler.

This change is an alternative approach to nodejs#3036 for fixing nodejs#3035.

Fixes nodejs#3035.

Author: Julien Gilli

lib/domain.js

@@ -71,39 +71,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();
+
+  if (stack.length === 1) {
+    try {
+      this.emittingTopLevelError = true;
+
+      caught = this.emit('error', er);
+
+      stack.length = 0;
+      exports.active = process.domain = null;
+    } finally {
+      this.emittingTopLevelError = 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 = 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();
+      }
+      if (stack.length) {
+        exports.active = process.domain = stack[stack.length - 1];
+        caught = process._fatalException(er2);
+      } else {
+        caught = false;
+      }
+      return caught;
     }
-    return caught;
   }
+
   return caught;
 };
 
@@ -176,20 +191,29 @@ Domain.prototype.run = function(fn) {
   if (this._disposed)
     return;
 
-  var ret;
+  var ret, args;
 
   this.enter();
   if (arguments.length >= 2) {
     var len = arguments.length;
-    var args = new Array(len - 1);
+    args = new Array(len - 1);
 
     for (var i = 1; i < len; i++)
       args[i - 1] = arguments[i];
+  }
 
+  // Wrap this in a try/catch block so that
+  //
+  // domain.run(function() { throw new Error("foo"); })
+  //
+  // actually emits an error event on the domain.
+  try {
     ret = fn.apply(this, args);
-  } else {
-    ret = fn.call(this);
+  } catch (err) {
+    process._forceTickDone();
+    process._fatalException(err);
   }
+
   this.exit();
 
   return ret;

lib/repl.js

@@ -159,20 +159,10 @@ function REPLServer(prompt,
     regExMatcher.test(savedRegExMatches.join(sep));
 
     if (!err) {
-      try {
-        if (self.useGlobal) {
-          result = script.runInThisContext({ displayErrors: false });
-        } else {
-          result = script.runInContext(context, { displayErrors: false });
-        }
-      } catch (e) {
-        err = e;
-        if (err && process.domain) {
-          debug('not recoverable, send to domain');
-          process.domain.emit('error', err);
-          process.domain.exit();
-          return;
-        }
+      if (self.useGlobal) {
+        result = script.runInThisContext({ displayErrors: false });
+      } else {
+        result = script.runInContext(context, { displayErrors: false });
       }
     }
 

lib/timers.js

@@ -81,24 +81,30 @@ function listOnTimeout() {
       if (domain && domain._disposed)
         continue;
 
-      try {
-        if (domain)
-          domain.enter();
-        threw = true;
-        first._called = true;
-        first._onTimeout();
-        if (domain)
-          domain.exit();
-        threw = false;
-      } finally {
-        if (threw) {
-          // We need to continue processing after domain error handling
-          // is complete, but not by using whatever domain was left over
-          // when the timeout threw its exception.
-          var oldDomain = process.domain;
-          process.domain = null;
-          process.nextTick(listOnTimeoutNT, list);
-          process.domain = oldDomain;
+      // If there's no active domain or we're in the
+      // error handler for the domain at the top of the stack,
+      // don't use try/catch to allow --abort-on-uncaught-exception
+      // to abort if an error is thrown.
+      // Otherwise, use try/catch to get the chance to emit an
+      // error on the current domain
+      if (!domain || domain.emittingTopLevelError) {
+        try {
+          fireTimer(first);
+          threw = false;
+        } finally {
+          if (threw) {
+            postFireTimer();
+          }
+        }
+      } else {
+        try {
+          fireTimer(first);
+          threw = false;
+        } catch (err) {
+          process._fatalException(err);
+          if (threw) {
+            postFireTimer();
+          }
         }
       }
     }
@@ -108,6 +114,26 @@ function listOnTimeout() {
   assert(L.isEmpty(list));
   list.close();
   delete lists[msecs];
+
+  function fireTimer(timer) {
+    if (domain)
+      domain.enter();
+    threw = true;
+    first._called = true;
+    first._onTimeout();
+    if (domain)
+      domain.exit();
+  }
+
+  function postFireTimer() {
+    // We need to continue processing after domain error handling
+    // is complete, but not by using whatever domain was left over
+    // when the timeout threw its exception.
+    var oldDomain = process.domain;
+    process.domain = null;
+    process.nextTick(listOnTimeoutNT, list);
+    process.domain = oldDomain;
+  }
 }
 
 
@@ -370,20 +396,28 @@ function processImmediate() {
       domain.enter();
 
     var threw = true;
-    try {
-      immediate._onImmediate();
-      threw = false;
-    } finally {
-      if (threw) {
-        if (!L.isEmpty(queue)) {
-          // Handle any remaining on next tick, assuming we're still
-          // alive to do so.
-          while (!L.isEmpty(immediateQueue)) {
-            L.append(queue, L.shift(immediateQueue));
-          }
-          immediateQueue = queue;
-          process.nextTick(processImmediate);
-        }
+
+    // If there's no active domain or we're in the
+    // error handler for the domain at the top of the stack,
+    // don't use try/catch to allow --abort-on-uncaught-exception
+    // to abort if an error is thrown.
+    // Otherwise, use try/catch to get the chance to emit an
+    // error on the current domain
+    if (!domain || domain.emittingTopLevelError) {
+      try {
+        immediate._onImmediate();
+        threw = false;
+      } finally {
+        postImmediate(threw);
+      }
+    } else {
+      try {
+        immediate._onImmediate();
+        threw = false;
+      } catch (err) {
+        process._fatalException(err);
+
+        postImmediate(threw);
       }
     }
 
@@ -397,6 +431,20 @@ function processImmediate() {
   if (L.isEmpty(immediateQueue)) {
     process._needImmediateCallback = false;
   }
+
+  function postImmediate(threw) {
+    if (threw) {
+      if (!L.isEmpty(queue)) {
+        // Handle any remaining on next tick, assuming we're still
+        // alive to do so.
+        while (!L.isEmpty(immediateQueue)) {
+          L.append(queue, L.shift(immediateQueue));
+        }
+        immediateQueue = queue;
+        process.nextTick(processImmediate);
+      }
+    }
+  }
 }
 
 

src/node.cc

@@ -2180,11 +2180,7 @@ void FatalException(Isolate* isolate,
 
   if (false == caught->BooleanValue()) {
     ReportException(env, error, message);
-    if (abort_on_uncaught_exception) {
-      ABORT();
-    } else {
-      exit(1);
-    }
+    exit(1);
   }
 }
 
@@ -3201,6 +3197,10 @@ static void ParseArgs(int* argc,
     } else if (strcmp(arg, "--abort-on-uncaught-exception") == 0 ||
                strcmp(arg, "--abort_on_uncaught_exception") == 0) {
       abort_on_uncaught_exception = true;
+      // Pass this option to V8 anyway, because V8 uses it to determine
+      // if it needs to abort on an uncaught exception
+      new_v8_argv[new_v8_argc] = arg;
+      new_v8_argc += 1;
     } else if (strcmp(arg, "--v8-options") == 0) {
       new_v8_argv[new_v8_argc] = "--help";
       new_v8_argc += 1;