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 nodejs#8631. Also fixes nodejs#8630.

deps/v8/include/v8.h

@@ -2845,6 +2845,17 @@ class V8EXPORT Isolate {
    */
   static Isolate* GetCurrent();
 
+  /**
+   * Custom callback used by embedders to help V8 determine if it should abort
+   * when it throws and no internal handler can catch the exception.
+   * If FLAG_abort_on_uncaught_exception is true, then V8 will abort if either:
+   * - no custom callback is set.
+   * - the custom callback set returns true.
+   * Otherwise it won't abort.
+   */
+  typedef bool (*abort_on_uncaught_exception_t)();
+  void SetAbortOnUncaughtException(abort_on_uncaught_exception_t callback);
+
   /**
    * Methods below this point require holding a lock (using Locker) in
    * a multi-threaded environment.

deps/v8/src/api.cc

@@ -5565,6 +5565,11 @@ void Isolate::Enter() {
   isolate->Enter();
 }
 
+void Isolate::SetAbortOnUncaughtException(
+      abort_on_uncaught_exception_t callback) {
+  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
+  isolate->SetAbortOnUncaughtException(callback);
+}
 
 void Isolate::Exit() {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);

deps/v8/src/isolate.cc

@@ -1152,18 +1152,26 @@ void Isolate::DoThrow(Object* exception, MessageLocation* location) {
         thread_local_top()->pending_message_end_pos_ = location->end_pos();
       }
 
-      // If the abort-on-uncaught-exception flag is specified, abort on any
-      // exception not caught by JavaScript, even when an external handler is
-      // present.  This flag is intended for use by JavaScript developers, so
-      // print a user-friendly stack trace (not an internal one).
+      // If the abort-on-uncaught-exception flag is specified, and if the
+      // exception is not caught by JavaScript (even when an external handler is
+      // present).
       if (fatal_exception_depth == 0 &&
           FLAG_abort_on_uncaught_exception &&
           (report_exception || can_be_caught_externally)) {
-        fatal_exception_depth++;
-        fprintf(stderr, "%s\n\nFROM\n",
-          *MessageHandler::GetLocalizedMessage(message_obj));
-        PrintCurrentStackTrace(stderr);
-        OS::Abort();
+        // If the embedder didn't specify a custom uncaught exception callback,
+        // or if the custom callback determined that V8 should abort, then
+        // abort
+        bool should_abort = !abort_on_uncaught_exception_callback_ ||
+                             abort_on_uncaught_exception_callback_();
+        if (should_abort) {
+          fatal_exception_depth++;
+          // This flag is intended for use by JavaScript developers, so
+          // print a user-friendly stack trace (not an internal one).
+          fprintf(stderr, "%s\n\nFROM\n",
+            *MessageHandler::GetLocalizedMessage(message_obj));
+          PrintCurrentStackTrace(stderr);
+          OS::Abort();
+        }
       }
     } else if (location != NULL && !location->script().is_null()) {
       // We are bootstrapping and caught an error where the location is set
@@ -1339,6 +1347,10 @@ void Isolate::SetCaptureStackTraceForUncaughtExceptions(
   stack_trace_for_uncaught_exceptions_options_ = options;
 }
 
+void Isolate::SetAbortOnUncaughtException(
+      v8::Isolate::abort_on_uncaught_exception_t callback) {
+  abort_on_uncaught_exception_callback_ = callback;
+}
 
 bool Isolate::is_out_of_memory() {
   if (has_pending_exception()) {
@@ -1534,7 +1546,8 @@ Isolate::Isolate()
       date_cache_(NULL),
       context_exit_happened_(false),
       deferred_handles_head_(NULL),
-      optimizing_compiler_thread_(this) {
+      optimizing_compiler_thread_(this),
+      abort_on_uncaught_exception_callback_(0) {
   TRACE_ISOLATE(constructor);
 
   memset(isolate_addresses_, 0,

deps/v8/src/isolate.h

@@ -692,6 +692,9 @@ class Isolate {
       int frame_limit,
       StackTrace::StackTraceOptions options);
 
+  typedef bool (*abort_on_uncaught_exception_t)();
+  void SetAbortOnUncaughtException(abort_on_uncaught_exception_t callback);
+
   // Tells whether the current context has experienced an out of memory
   // exception.
   bool is_out_of_memory();
@@ -1292,6 +1295,8 @@ class Isolate {
   DeferredHandles* deferred_handles_head_;
   OptimizingCompilerThread optimizing_compiler_thread_;
 
+  abort_on_uncaught_exception_t abort_on_uncaught_exception_callback_;
+
   friend class ExecutionAccess;
   friend class HandleScopeImplementer;
   friend class IsolateInitializer;

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,29 @@ 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);
+
+  bool has_domain = domain_v->IsObject();
+  if (has_domain) {
+    Local<Object> domain = domain_v->ToObject();
+    assert(!domain.IsEmpty());
+    if (!domain->IsNull()) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+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 +2461,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 +2988,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();
+}