domains: port fix abort on uncaught to v0.12jenkins-accept-commit-temp

Port fbff705 and caeb677 from v0.10 to v0.12, original commit messages:

  fbff705
  Add v8::Isolate::SetAbortOnUncaughtException() so the user can be
  notified when an uncaught exception has bubbled.

  caeb677
  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: #8877

Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
PR-URL: https://github.com/joyent/node/pull/25835

9 files changed, 448 insertions, 41 deletions

diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h
index ef0bda63f..8ae075797 100644
--- a/deps/v8/include/v8.h
+++ b/deps/v8/include/v8.h
@@ -4187,6 +4187,17 @@ class V8_EXPORT 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)(Isolate*);
+  void SetAbortOnUncaughtException(abort_on_uncaught_exception_t callback);
+
+  /**
    * Methods below this point require holding a lock (using Locker) in
    * a multi-threaded environment.
    */
diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc
index 3d9e9d831..e6597246e 100644
--- a/deps/v8/src/api.cc
+++ b/deps/v8/src/api.cc
@@ -6556,6 +6556,13 @@ void 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);
   isolate->Exit();
diff --git a/deps/v8/src/isolate.cc b/deps/v8/src/isolate.cc
index 215296d73..2905a0828 100644
--- a/deps/v8/src/isolate.cc
+++ b/deps/v8/src/isolate.cc
@@ -1090,19 +1090,30 @@ 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 &&
+          (FLAG_abort_on_uncaught_exception) &&
           (report_exception || can_be_caught_externally)) {
-        fatal_exception_depth++;
-        PrintF(stderr,
-               "%s\n\nFROM\n",
-               MessageHandler::GetLocalizedMessage(this, message_obj).get());
-        PrintCurrentStackTrace(stderr);
-        base::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_(
+                                 reinterpret_cast<v8::Isolate*>(this)
+                             );
+
+        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).
+          PrintF(stderr,
+                 "%s\n\nFROM\n",
+                 MessageHandler::GetLocalizedMessage(this, message_obj).get());
+          PrintCurrentStackTrace(stderr);
+          base::OS::Abort();
+        }
       }
     } else if (location != NULL && !location->script().is_null()) {
       // We are bootstrapping and caught an error where the location is set
@@ -1299,6 +1310,12 @@ void Isolate::SetCaptureStackTraceForUncaughtExceptions(
 }
 
 
+void Isolate::SetAbortOnUncaughtException(
+      v8::Isolate::abort_on_uncaught_exception_t callback) {
+  abort_on_uncaught_exception_callback_ = callback;
+}
+
+
 Handle<Context> Isolate::native_context() {
   return handle(context()->native_context());
 }
@@ -1474,7 +1491,8 @@ Isolate::Isolate()
       num_sweeper_threads_(0),
       stress_deopt_count_(0),
       next_optimization_id_(0),
-      use_counter_callback_(NULL) {
+      use_counter_callback_(NULL),
+      abort_on_uncaught_exception_callback_(NULL) {
   id_ = base::NoBarrier_AtomicIncrement(&isolate_counter_, 1);
   TRACE_ISOLATE(constructor);
 
diff --git a/deps/v8/src/isolate.h b/deps/v8/src/isolate.h
index eae5cad28..c621fcc2f 100644
--- a/deps/v8/src/isolate.h
+++ b/deps/v8/src/isolate.h
@@ -707,6 +707,9 @@ class Isolate {
       int frame_limit,
       StackTrace::StackTraceOptions options);
 
+  typedef bool (*abort_on_uncaught_exception_t)(v8::Isolate*);
+  void SetAbortOnUncaughtException(abort_on_uncaught_exception_t callback);
+
   void PrintCurrentStackTrace(FILE* out);
   void PrintStack(StringStream* accumulator);
   void PrintStack(FILE* out);
@@ -1331,6 +1334,8 @@ class Isolate {
 
   v8::Isolate::UseCounterCallback use_counter_callback_;
 
+  abort_on_uncaught_exception_t abort_on_uncaught_exception_callback_;
+
   friend class ExecutionAccess;
   friend class HandleScopeImplementer;
   friend class IsolateInitializer;
diff --git a/lib/domain.js b/lib/domain.js
index 5924b44eb..87a47076d 100644
--- a/lib/domain.js
+++ b/lib/domain.js
@@ -77,6 +77,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
@@ -89,38 +103,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;
 };
diff --git a/src/env.h b/src/env.h
index fb243b0eb..2768388f3 100644
--- a/src/env.h
+++ b/src/env.h
@@ -86,6 +86,7 @@ namespace node {
   V(dev_string, "dev")                                                        \
   V(disposed_string, "_disposed")                                             \
   V(domain_string, "domain")                                                  \
+  V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError")   \
   V(exchange_string, "exchange")                                              \
   V(idle_string, "idle")                                                      \
   V(irq_string, "irq")                                                        \
diff --git a/src/node.cc b/src/node.cc
index 1ad02e993..45193498d 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -909,6 +909,33 @@ Local<Value> WinapiErrnoException(Isolate* isolate,
 #endif
 
 
+static bool IsDomainActive(const Environment* env) {
+  if (!env->using_domains()) {
+    return false;
+  }
+
+  Local<Array> domain_array = env->domain_array().As<Array>();
+  uint32_t domainsArrayLength = domain_array->Length();
+  if (domainsArrayLength == 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();
+  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.GetIsolate());
 
@@ -2772,6 +2799,9 @@ void SetupProcessObject(Environment* env,
 
   // pre-set _events object for faster emit checks
   process->Set(env->events_string(), Object::New(env->isolate()));
+
+  process->Set(env->emitting_top_level_domain_error_string(),
+               False(env->isolate()));
 }
 
 
@@ -3453,6 +3483,8 @@ void Init(int* argc,
   node_isolate = Isolate::New();
   Isolate::Scope isolate_scope(node_isolate);
 
+  node_isolate->SetAbortOnUncaughtException(ShouldAbortOnUncaughtException);
+
 #ifdef __POSIX__
   // Raise the open file descriptor limit.
   {  // NOLINT (whitespace/braces)
diff --git a/test/simple/test-domain-top-level-error-handler-throw.js b/test/simple/test-domain-top-level-error-handler-throw.js
new file mode 100644
index 000000000..6d4b69c19
--- /dev/null
+++ b/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();
+}
diff --git a/test/simple/test-domain-with-abort-on-uncaught-exception.js b/test/simple/test-domain-with-abort-on-uncaught-exception.js
new file mode 100644
index 000000000..d1ac09bf5
--- /dev/null
+++ b/test/simple/test-domain-with-abort-on-uncaught-exception.js
@@ -0,0 +1,229 @@
+// 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(err) {
+    // 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) {
+        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);
+
+            // 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];
+          }
+        }
+
+        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);
+        }
+      });
+    }
+  }
+
+  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
+  });
+}