From c16963b977b3cede4cef476c275226a513831407 Mon Sep 17 00:00:00 2001
From: Miroslav Bajtos <miro.bajtos@gmail.com>
Date: Mon, 17 Jun 2013 21:19:59 +0200
Subject: [PATCH] src: break on uncaught exception

Most TryCatch blocks have SetVerbose flag on, this tells V8 to report
uncaught exceptions to debugger.

FatalException handler is called from V8 Message listener instead from
the place where TryCatch was used. Otherwise uncaught exceptions are
logged twice.

See comment in `deps/v8/include/v8.h` for explanation of SetVerbose
flag:

>  By default, exceptions that are caught by an external exception
>  handler are not reported.  Call SetVerbose with true on an
>  external exception handler to have exceptions caught by the
>  handler reported as if they were not caught.

The flag is used by `Isolate::ShouldReportException()`, which is called
by `Isolate::DoThrow()` to decide whether an exception is considered
uncaught.
---
 src/node.cc                                   |  90 ++++++++-----
 src/node.h                                    |   2 +-
 src/node_script.cc                            |   8 +-
 test/fixtures/uncaught-exceptions/domain.js   |  12 ++
 test/fixtures/uncaught-exceptions/global.js   |   2 +
 .../uncaught-exceptions/parse-error-mod.js    |   2 +
 .../uncaught-exceptions/parse-error.js        |   2 +
 test/fixtures/uncaught-exceptions/timeout.js  |   3 +
 test/simple/test-debug-break-on-uncaught.js   | 124 ++++++++++++++++++
 9 files changed, 211 insertions(+), 34 deletions(-)
 create mode 100644 test/fixtures/uncaught-exceptions/domain.js
 create mode 100644 test/fixtures/uncaught-exceptions/global.js
 create mode 100644 test/fixtures/uncaught-exceptions/parse-error-mod.js
 create mode 100644 test/fixtures/uncaught-exceptions/parse-error.js
 create mode 100644 test/fixtures/uncaught-exceptions/timeout.js
 create mode 100644 test/simple/test-debug-break-on-uncaught.js

diff --git a/src/node.cc b/src/node.cc
index 6aa0eda69ddb4a..38ecbb836c78f8 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -908,6 +908,7 @@ MakeDomainCallback(const Handle<Object> object,
   Local<Function> exit;
 
   TryCatch try_catch;
+  try_catch.SetVerbose(true);
 
   bool has_domain = domain_v->IsObject();
   if (has_domain) {
@@ -922,7 +923,6 @@ MakeDomainCallback(const Handle<Object> object,
     enter->Call(domain, 0, NULL);
 
     if (try_catch.HasCaught()) {
-      FatalException(try_catch);
       return Undefined(node_isolate);
     }
   }
@@ -930,7 +930,6 @@ MakeDomainCallback(const Handle<Object> object,
   Local<Value> ret = callback->Call(object, argc, argv);
 
   if (try_catch.HasCaught()) {
-    FatalException(try_catch);
     return Undefined(node_isolate);
   }
 
@@ -940,7 +939,6 @@ MakeDomainCallback(const Handle<Object> object,
     exit->Call(domain, 0, NULL);
 
     if (try_catch.HasCaught()) {
-      FatalException(try_catch);
       return Undefined(node_isolate);
     }
   }
@@ -954,7 +952,6 @@ MakeDomainCallback(const Handle<Object> object,
   process_tickCallback->Call(process, 0, NULL);
 
   if (try_catch.HasCaught()) {
-    FatalException(try_catch);
     return Undefined(node_isolate);
   }
 
@@ -981,11 +978,11 @@ MakeCallback(const Handle<Object> object,
   }
 
   TryCatch try_catch;
+  try_catch.SetVerbose(true);
 
   Local<Value> ret = callback->Call(object, argc, argv);
 
   if (try_catch.HasCaught()) {
-    FatalException(try_catch);
     return Undefined(node_isolate);
   }
 
@@ -998,7 +995,6 @@ MakeCallback(const Handle<Object> object,
   process_tickCallback->Call(process, 0, NULL);
 
   if (try_catch.HasCaught()) {
-    FatalException(try_catch);
     return Undefined(node_isolate);
   }
 
@@ -1131,7 +1127,7 @@ ssize_t DecodeWrite(char *buf,
   return StringBytes::Write(buf, buflen, val, encoding, NULL);
 }
 
-void DisplayExceptionLine (TryCatch &try_catch) {
+void DisplayExceptionLine(Handle<Message> message) {
   // Prevent re-entry into this function.  For example, if there is
   // a throw from a program in vm.runInThisContext(code, filename, true),
   // then we want to show the original failure, not the secondary one.
@@ -1140,10 +1136,6 @@ void DisplayExceptionLine (TryCatch &try_catch) {
   if (displayed_error) return;
   displayed_error = true;
 
-  HandleScope scope(node_isolate);
-
-  Handle<Message> message = try_catch.Message();
-
   uv_tty_reset_mode();
 
   fprintf(stderr, "\n");
@@ -1197,21 +1189,21 @@ void DisplayExceptionLine (TryCatch &try_catch) {
 }
 
 
-static void ReportException(TryCatch &try_catch, bool show_line) {
+static void ReportException(Handle<Value> er, Handle<Message> message) {
   HandleScope scope(node_isolate);
 
-  if (show_line) DisplayExceptionLine(try_catch);
+  DisplayExceptionLine(message);
 
-  String::Utf8Value trace(try_catch.StackTrace());
+  Local<Value> trace_value(er->ToObject()->Get(String::New("stack")));
+  String::Utf8Value trace(trace_value);
 
   // range errors have a trace member set to undefined
-  if (trace.length() > 0 && !try_catch.StackTrace()->IsUndefined()) {
+  if (trace.length() > 0 && !trace_value->IsUndefined()) {
     fprintf(stderr, "%s\n", *trace);
   } else {
     // this really only happens for RangeErrors, since they're the only
     // kind that won't have all this info in the trace, or when non-Error
     // objects are thrown manually.
-    Local<Value> er = try_catch.Exception();
     bool isErrorObject = er->IsObject() &&
       !(er->ToObject()->Get(String::New("message"))->IsUndefined()) &&
       !(er->ToObject()->Get(String::New("name"))->IsUndefined());
@@ -1229,20 +1221,30 @@ static void ReportException(TryCatch &try_catch, bool show_line) {
   fflush(stderr);
 }
 
+
+static void ReportException(TryCatch& try_catch) {
+  ReportException(try_catch.Exception(), try_catch.Message());
+}
+
+
 // Executes a str within the current v8 context.
 Local<Value> ExecuteString(Handle<String> source, Handle<Value> filename) {
   HandleScope scope(node_isolate);
   TryCatch try_catch;
 
+  // try_catch must be nonverbose to disable FatalException() handler,
+  // we will handle exceptions ourself.
+  try_catch.SetVerbose(false);
+
   Local<v8::Script> script = v8::Script::Compile(source, filename);
   if (script.IsEmpty()) {
-    ReportException(try_catch, true);
+    ReportException(try_catch);
     exit(3);
   }
 
   Local<Value> result = script->Run();
   if (result.IsEmpty()) {
-    ReportException(try_catch, true);
+    ReportException(try_catch);
     exit(4);
   }
 
@@ -1869,7 +1871,8 @@ static void OnFatalError(const char* location, const char* message) {
   exit(5);
 }
 
-void FatalException(TryCatch &try_catch) {
+
+void FatalException(Handle<Value> error, Handle<Message> message) {
   HandleScope scope(node_isolate);
 
   if (fatal_exception_symbol.IsEmpty())
@@ -1880,33 +1883,48 @@ void FatalException(TryCatch &try_catch) {
   if (!fatal_v->IsFunction()) {
     // failed before the process._fatalException function was added!
     // this is probably pretty bad.  Nothing to do but report and exit.
-    ReportException(try_catch, true);
+    ReportException(error, message);
     exit(6);
   }
 
   Local<Function> fatal_f = Local<Function>::Cast(fatal_v);
 
-  Local<Value> error = try_catch.Exception();
-  Local<Value> argv[] = { error };
-
   TryCatch fatal_try_catch;
 
+  // Do not call FatalException when _fatalException handler throws
+  fatal_try_catch.SetVerbose(false);
+
   // this will return true if the JS layer handled it, false otherwise
-  Local<Value> caught = fatal_f->Call(process, ARRAY_SIZE(argv), argv);
+  Local<Value> caught = fatal_f->Call(process, 1, &error);
 
   if (fatal_try_catch.HasCaught()) {
     // the fatal exception function threw, so we must exit
-    ReportException(fatal_try_catch, true);
+    ReportException(fatal_try_catch);
     exit(7);
   }
 
   if (false == caught->BooleanValue()) {
-    ReportException(try_catch, true);
+    ReportException(error, message);
     exit(8);
   }
 }
 
 
+void FatalException(TryCatch& try_catch) {
+  HandleScope scope(node_isolate);
+  // TODO do not call FatalException if try_catch is verbose
+  // (requires V8 API to expose getter for try_catch.is_verbose_)
+  FatalException(try_catch.Exception(), try_catch.Message());
+}
+
+
+void OnMessage(Handle<Message> message, Handle<Value> error) {
+  // The current version of V8 sends messages for errors only
+  // (thus `error` is always set).
+  FatalException(error, message);
+}
+
+
 Persistent<Object> binding_cache;
 Persistent<Array> module_load_list;
 
@@ -2416,10 +2434,15 @@ void Load(Handle<Object> process_l) {
 
   TryCatch try_catch;
 
+  // Disable verbose mode to stop FatalException() handler from trying
+  // to handle the exception. Errors this early in the start-up phase
+  // are not safe to ignore.
+  try_catch.SetVerbose(false);
+
   Local<Value> f_value = ExecuteString(MainSource(),
                                        IMMUTABLE_STRING("node.js"));
   if (try_catch.HasCaught())  {
-    ReportException(try_catch, true);
+    ReportException(try_catch);
     exit(10);
   }
   assert(f_value->IsFunction());
@@ -2445,11 +2468,15 @@ void Load(Handle<Object> process_l) {
   InitPerfCounters(global);
 #endif
 
-  f->Call(global, 1, args);
+  // Enable handling of uncaught exceptions
+  // (FatalException(), break on uncaught exception in debugger)
+  //
+  // This is not strictly necessary since it's almost impossible
+  // to attach the debugger fast enought to break on exception
+  // thrown during process startup.
+  try_catch.SetVerbose(true);
 
-  if (try_catch.HasCaught())  {
-    FatalException(try_catch);
-  }
+  f->Call(global, 1, args);
 }
 
 static void PrintHelp();
@@ -2936,6 +2963,7 @@ char** Init(int argc, char *argv[]) {
   uv_idle_init(uv_default_loop(), &idle_immediate_dummy);
 
   V8::SetFatalErrorHandler(node::OnFatalError);
+  V8::AddMessageListener(OnMessage);
 
   // If the --debug flag was specified then initialize the debug thread.
   if (use_debug_agent) {
diff --git a/src/node.h b/src/node.h
index 49f342fae360b4..59d03a355aef81 100644
--- a/src/node.h
+++ b/src/node.h
@@ -133,7 +133,7 @@ enum encoding {ASCII, UTF8, BASE64, UCS2, BINARY, HEX, BUFFER};
 enum encoding ParseEncoding(v8::Handle<v8::Value> encoding_v,
                             enum encoding _default = BINARY);
 NODE_EXTERN void FatalException(v8::TryCatch &try_catch);
-void DisplayExceptionLine(v8::TryCatch &try_catch); // hack
+void DisplayExceptionLine(v8::Handle<v8::Message> message);
 
 NODE_EXTERN v8::Local<v8::Value> Encode(const void *buf, size_t len,
                                         enum encoding encoding = BINARY);
diff --git a/src/node_script.cc b/src/node_script.cc
index 639f41ab01ee81..4766cd05a69863 100644
--- a/src/node_script.cc
+++ b/src/node_script.cc
@@ -401,6 +401,10 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
   // Catch errors
   TryCatch try_catch;
 
+  // TryCatch must not be verbose to prevent duplicate logging
+  // of uncaught exceptions (we are rethrowing them)
+  try_catch.SetVerbose(false);
+
   Handle<Value> result;
   Handle<Script> script;
 
@@ -411,7 +415,7 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
                                          : Script::New(code, filename);
     if (script.IsEmpty()) {
       // FIXME UGLY HACK TO DISPLAY SYNTAX ERRORS.
-      if (display_error) DisplayExceptionLine(try_catch);
+      if (display_error) DisplayExceptionLine(try_catch.Message());
 
       // Hack because I can't get a proper stacktrace on SyntaxError
       return try_catch.ReThrow();
@@ -444,7 +448,7 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
             String::New("Script execution timed out.")));
     }
     if (result.IsEmpty()) {
-      if (display_error) DisplayExceptionLine(try_catch);
+      if (display_error) DisplayExceptionLine(try_catch.Message());
       return try_catch.ReThrow();
     }
   } else {
diff --git a/test/fixtures/uncaught-exceptions/domain.js b/test/fixtures/uncaught-exceptions/domain.js
new file mode 100644
index 00000000000000..c00b22dbb9c86d
--- /dev/null
+++ b/test/fixtures/uncaught-exceptions/domain.js
@@ -0,0 +1,12 @@
+var domain = require('domain');
+
+var d = domain.create();
+d.on('error', function(err) {
+  console.log('[ignored]', err.stack);
+});
+
+d.run(function() {
+  setImmediate(function() {
+    throw new Error('in domain');
+  });
+});
diff --git a/test/fixtures/uncaught-exceptions/global.js b/test/fixtures/uncaught-exceptions/global.js
new file mode 100644
index 00000000000000..df14e88c47d8e8
--- /dev/null
+++ b/test/fixtures/uncaught-exceptions/global.js
@@ -0,0 +1,2 @@
+console.log('going to throw an error');
+throw new Error('global');
diff --git a/test/fixtures/uncaught-exceptions/parse-error-mod.js b/test/fixtures/uncaught-exceptions/parse-error-mod.js
new file mode 100644
index 00000000000000..05dbf29f0026c8
--- /dev/null
+++ b/test/fixtures/uncaught-exceptions/parse-error-mod.js
@@ -0,0 +1,2 @@
+console.log('parse error on next line');
+var a = ';
diff --git a/test/fixtures/uncaught-exceptions/parse-error.js b/test/fixtures/uncaught-exceptions/parse-error.js
new file mode 100644
index 00000000000000..8ec4d4ec85e18c
--- /dev/null
+++ b/test/fixtures/uncaught-exceptions/parse-error.js
@@ -0,0 +1,2 @@
+console.log('require fails on next line');
+require('./parse-error-mod.js');
diff --git a/test/fixtures/uncaught-exceptions/timeout.js b/test/fixtures/uncaught-exceptions/timeout.js
new file mode 100644
index 00000000000000..1d8491ff62e6d6
--- /dev/null
+++ b/test/fixtures/uncaught-exceptions/timeout.js
@@ -0,0 +1,3 @@
+setTimeout(function() {
+  throw new Error('timeout');
+}, 10);
diff --git a/test/simple/test-debug-break-on-uncaught.js b/test/simple/test-debug-break-on-uncaught.js
new file mode 100644
index 00000000000000..5bb5b06a29b3c0
--- /dev/null
+++ b/test/simple/test-debug-break-on-uncaught.js
@@ -0,0 +1,124 @@
+// 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 path = require('path');
+var assert = require('assert');
+var spawn = require('child_process').spawn;
+var common = require('../common');
+var debug = require('_debugger');
+
+addScenario('global.js', null, 2);
+addScenario('timeout.js', null, 2);
+addScenario('domain.js', null, 10);
+
+// Exception is thrown from module.js (internal file)
+//   var compiledWrapper = runInThisContext(wrapper, filename, 0, true);
+addScenario('parse-error.js', 'module.js', null);
+
+run();
+
+/***************** IMPLEMENTATION *****************/
+
+var scenarios;
+function addScenario(scriptName, throwsInFile, throwsOnLine) {
+  if (!scenarios) scenarios = [];
+  scenarios.push(
+    runScenario.bind(null, scriptName, throwsInFile, throwsOnLine, run)
+  );
+}
+
+function run() {
+  var next = scenarios.shift();
+  if (next) next();
+}
+
+function runScenario(scriptName, throwsInFile, throwsOnLine, next) {
+  console.log('**[ %s ]**', scriptName);
+  var asserted = false;
+  var port = common.PORT + 1337;
+
+  var testScript = path.join(
+    common.fixturesDir,
+    'uncaught-exceptions',
+    scriptName
+  );
+
+  var child = spawn(process.execPath, [ '--debug-brk=' + port, testScript ]);
+  child.on('close', function() {
+    assert(asserted, 'debugger did not pause on exception');
+    if (next) next();
+  })
+
+  var exceptions = [];
+
+  setTimeout(setupClient.bind(null, runTest), 200);
+
+  function setupClient(callback) {
+    var client = new debug.Client();
+
+    client.once('ready', callback.bind(null, client));
+
+    client.on('unhandledResponse', function(body) {
+      console.error('unhandled response: %j', body);
+    });
+
+    client.on('error', function(err) {
+      if (asserted) return;
+      assert.ifError(err);
+    });
+
+    client.connect(port);
+  }
+
+  function runTest(client) {
+    client.req(
+      {
+        command: 'setexceptionbreak',
+        arguments: {
+          type: 'uncaught',
+          enabled: true
+        }
+      },
+      function(error, result) {
+        assert.ifError(error);
+
+        client.on('exception', function(event) {
+          exceptions.push(event.body);
+        });
+
+        client.reqContinue(function(error, result) {
+          assert.ifError(error);
+          setTimeout(assertHasPaused.bind(null, client), 100);
+        });
+      }
+    );
+  }
+
+  function assertHasPaused(client) {
+    assert.equal(exceptions.length, 1, 'debugger did not pause on exception');
+    assert.equal(exceptions[0].uncaught, true);
+    assert.equal(exceptions[0].script.name, throwsInFile || testScript);
+    if (throwsOnLine != null)
+      assert.equal(exceptions[0].sourceLine + 1, throwsOnLine);
+    asserted = true;
+    client.reqContinue(assert.ifError);
+  }
+}