Properly process try/finally blocks.

In some circumstances, try/finally block can actually catch the exception:

function f() {
  try {
    throw 42;
  } finally {
    return 0;
  }
}

Therefore when propagating exception to v8::TryCatch, we must be sure
there is no try/finally blocks as well.

When bulding the messages we should be more conservative and expect that
any v8::TryCatch with no JS try/catch in between can potentionally
be the right exception handler.

Plus various minor refactorings.

BUG=1147
TEST=cctest/test-api/TryCatchAndFinallyHidingException, cctest/test-api/TryCatchAndFinally

Review URL: http://codereview.chromium.org/6526016

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6809 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/samples/shell.cc b/samples/shell.cc
index 6b67df6..64f78f0 100644 (file)
--- a/samples/shell.cc
+++ b/samples/shell.cc
@@ -27,6 +27,7 @@
 
 #include <v8.h>
 #include <v8-testing.h>
+#include <assert.h>
 #include <fcntl.h>
 #include <string.h>
 #include <stdio.h>
@@ -290,11 +291,13 @@ bool ExecuteString(v8::Handle<v8::String> source,
   } else {
     v8::Handle<v8::Value> result = script->Run();
     if (result.IsEmpty()) {
+      assert(try_catch.HasCaught());
       // Print errors that happened during execution.
       if (report_exceptions)
         ReportException(&try_catch);
       return false;
     } else {
+      assert(!try_catch.HasCaught());
       if (print_result && !result->IsUndefined()) {
         // If all went well and the result wasn't undefined then print
         // the returned value.

diff --git a/src/d8.cc b/src/d8.cc
index f0da7ac..4dcc794 100644 (file)
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -127,11 +127,13 @@ bool Shell::ExecuteString(Handle<String> source,
   } else {
     Handle<Value> result = script->Run();
     if (result.IsEmpty()) {
+      ASSERT(try_catch.HasCaught());
       // Print errors that happened during execution.
       if (report_exceptions && !i::FLAG_debugger)
         ReportException(&try_catch);
       return false;
     } else {
+      ASSERT(!try_catch.HasCaught());
       if (print_result && !result->IsUndefined()) {
         // If all went well and the result wasn't undefined then print
         // the returned value.

diff --git a/src/top.cc b/src/top.cc
index 3364c0d..83d7de3 100644 (file)
--- a/src/top.cc
+++ b/src/top.cc
@@ -333,7 +333,7 @@ void Top::RegisterTryCatchHandler(v8::TryCatch* that) {
 
 
 void Top::UnregisterTryCatchHandler(v8::TryCatch* that) {
-  ASSERT(thread_local_.TryCatchHandler() == that);
+  ASSERT(try_catch_handler() == that);
   thread_local_.set_try_catch_handler_address(
       reinterpret_cast<Address>(that->next_));
   thread_local_.catcher_ = NULL;
@@ -732,6 +732,13 @@ Failure* Top::Throw(Object* exception, MessageLocation* location) {
 
 
 Failure* Top::ReThrow(MaybeObject* exception, MessageLocation* location) {
+  bool can_be_caught_externally = false;
+  ShouldReportException(&can_be_caught_externally,
+                        is_catchable_by_javascript(exception));
+  if (can_be_caught_externally) {
+    thread_local_.catcher_ = try_catch_handler();
+  }
+
   // Set the exception being re-thrown.
   set_pending_exception(exception);
   return Failure::Exception();
@@ -807,7 +814,7 @@ void Top::ComputeLocation(MessageLocation* target) {
 }
 
 
-bool Top::ShouldReportException(bool* is_caught_externally,
+bool Top::ShouldReportException(bool* can_be_caught_externally,
                                 bool catchable_by_javascript) {
   // Find the top-most try-catch handler.
   StackHandler* handler =
@@ -823,13 +830,13 @@ bool Top::ShouldReportException(bool* is_caught_externally,
   // The exception has been externally caught if and only if there is
   // an external handler which is on top of the top-most try-catch
   // handler.
-  *is_caught_externally = external_handler_address != NULL &&
+  *can_be_caught_externally = external_handler_address != NULL &&
       (handler == NULL || handler->address() > external_handler_address ||
        !catchable_by_javascript);
 
-  if (*is_caught_externally) {
+  if (*can_be_caught_externally) {
     // Only report the exception if the external handler is verbose.
-    return thread_local_.TryCatchHandler()->is_verbose_;
+    return try_catch_handler()->is_verbose_;
   } else {
     // Report the exception if it isn't caught by JavaScript code.
     return handler == NULL;
@@ -848,14 +855,12 @@ void Top::DoThrow(MaybeObject* exception,
   Handle<Object> exception_handle(exception_object);
 
   // Determine reporting and whether the exception is caught externally.
-  bool is_out_of_memory = exception == Failure::OutOfMemoryException();
-  bool is_termination_exception = exception == Heap::termination_exception();
-  bool catchable_by_javascript = !is_termination_exception && !is_out_of_memory;
+  bool catchable_by_javascript = is_catchable_by_javascript(exception);
   // Only real objects can be caught by JS.
   ASSERT(!catchable_by_javascript || is_object);
-  bool is_caught_externally = false;
+  bool can_be_caught_externally = false;
   bool should_report_exception =
-      ShouldReportException(&is_caught_externally, catchable_by_javascript);
+      ShouldReportException(&can_be_caught_externally, catchable_by_javascript);
   bool report_exception = catchable_by_javascript && should_report_exception;
 
 #ifdef ENABLE_DEBUGGER_SUPPORT
@@ -869,8 +874,8 @@ void Top::DoThrow(MaybeObject* exception,
   Handle<Object> message_obj;
   MessageLocation potential_computed_location;
   bool try_catch_needs_message =
-      is_caught_externally &&
-      thread_local_.TryCatchHandler()->capture_message_;
+      can_be_caught_externally &&
+      try_catch_handler()->capture_message_;
   if (report_exception || try_catch_needs_message) {
     if (location == NULL) {
       // If no location was specified we use a computed one instead
@@ -908,8 +913,8 @@ void Top::DoThrow(MaybeObject* exception,
     }
   }
 
-  if (is_caught_externally) {
-    thread_local_.catcher_ = thread_local_.TryCatchHandler();
+  if (can_be_caught_externally) {
+    thread_local_.catcher_ = try_catch_handler();
   }
 
   // NOTE: Notifying the debugger or generating the message
@@ -925,22 +930,63 @@ void Top::DoThrow(MaybeObject* exception,
 }
 
 
+bool Top::IsExternallyCaught() {
+  ASSERT(has_pending_exception());
+
+  if ((thread_local_.catcher_ == NULL) ||
+      (try_catch_handler() != thread_local_.catcher_)) {
+    // When throwing the exception, we found no v8::TryCatch
+    // which should care about this exception.
+    return false;
+  }
+
+  if (!is_catchable_by_javascript(pending_exception())) {
+    return true;
+  }
+
+  // Get the address of the external handler so we can compare the address to
+  // determine which one is closer to the top of the stack.
+  Address external_handler_address = thread_local_.try_catch_handler_address();
+  ASSERT(external_handler_address != NULL);
+
+  // The exception has been externally caught if and only if there is
+  // an external handler which is on top of the top-most try-finally
+  // handler.
+  // There should be no try-catch blocks as they would prohibit us from
+  // finding external catcher in the first place (see catcher_ check above).
+  //
+  // Note, that finally clause would rethrow an exception unless it's
+  // aborted by jumps in control flow like return, break, etc. and we'll
+  // have another chances to set proper v8::TryCatch.
+  StackHandler* handler =
+      StackHandler::FromAddress(Top::handler(Top::GetCurrentThread()));
+  while (handler != NULL && handler->address() < external_handler_address) {
+    ASSERT(!handler->is_try_catch());
+    if (handler->is_try_finally()) return false;
+
+    handler = handler->next();
+  }
+
+  return true;
+}
+
+
 void Top::ReportPendingMessages() {
   ASSERT(has_pending_exception());
-  setup_external_caught();
   // If the pending exception is OutOfMemoryException set out_of_memory in
   // the global context.  Note: We have to mark the global context here
   // since the GenerateThrowOutOfMemory stub cannot make a RuntimeCall to
   // set it.
-  bool external_caught = thread_local_.external_caught_exception_;
+  bool external_caught = IsExternallyCaught();
+  thread_local_.external_caught_exception_ = external_caught;
   HandleScope scope;
   if (thread_local_.pending_exception_ == Failure::OutOfMemoryException()) {
     context()->mark_out_of_memory();
   } else if (thread_local_.pending_exception_ ==
              Heap::termination_exception()) {
     if (external_caught) {
-      thread_local_.TryCatchHandler()->can_continue_ = false;
-      thread_local_.TryCatchHandler()->exception_ = Heap::null_value();
+      try_catch_handler()->can_continue_ = false;
+      try_catch_handler()->exception_ = Heap::null_value();
     }
   } else {
     // At this point all non-object (failure) exceptions have
@@ -949,9 +995,8 @@ void Top::ReportPendingMessages() {
     Handle<Object> exception(pending_exception_object);
     thread_local_.external_caught_exception_ = false;
     if (external_caught) {
-      thread_local_.TryCatchHandler()->can_continue_ = true;
-      thread_local_.TryCatchHandler()->exception_ =
-        thread_local_.pending_exception_;
+      try_catch_handler()->can_continue_ = true;
+      try_catch_handler()->exception_ = thread_local_.pending_exception_;
       if (!thread_local_.pending_message_obj_->IsTheHole()) {
         try_catch_handler()->message_ = thread_local_.pending_message_obj_;
       }

diff --git a/src/top.h b/src/top.h
index c052dad..26ae542 100644 (file)
--- a/src/top.h
+++ b/src/top.h
@@ -249,12 +249,7 @@ class Top {
     thread_local_.scheduled_exception_ = Heap::the_hole_value();
   }
 
-  static void setup_external_caught() {
-    thread_local_.external_caught_exception_ =
-        has_pending_exception() &&
-        (thread_local_.catcher_ != NULL) &&
-        (try_catch_handler() == thread_local_.catcher_);
-  }
+  static bool IsExternallyCaught();
 
   static void SetCaptureStackTraceForUncaughtExceptions(
       bool capture,
@@ -265,6 +260,11 @@ class Top {
   // exception.
   static bool is_out_of_memory();
 
+  static bool is_catchable_by_javascript(MaybeObject* exception) {
+    return (exception != Failure::OutOfMemoryException()) &&
+        (exception != Heap::termination_exception());
+  }
+
   // JS execution stack (see frames.h).
   static Address c_entry_fp(ThreadLocalTop* thread) {
     return thread->c_entry_fp_;
@@ -397,7 +397,7 @@ class Top {
                       const char* message);
   // Checks if exception should be reported and finds out if it's
   // caught externally.
-  static bool ShouldReportException(bool* is_caught_externally,
+  static bool ShouldReportException(bool* can_be_caught_externally,
                                     bool catchable_by_javascript);
 
   // Attempts to compute the current source location, storing the

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index ded42b4..3de5b92 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -2691,6 +2691,41 @@ THREADED_TEST(CatchExceptionFromWith) {
 }
 
 
+THREADED_TEST(TryCatchAndFinallyHidingException) {
+  v8::HandleScope scope;
+  LocalContext context;
+  v8::TryCatch try_catch;
+  CHECK(!try_catch.HasCaught());
+  CompileRun("function f(k) { try { this[k]; } finally { return 0; } };");
+  CompileRun("f({toString: function() { throw 42; }});");
+  CHECK(!try_catch.HasCaught());
+}
+
+
+v8::Handle<v8::Value> WithTryCatch(const v8::Arguments& args) {
+  v8::TryCatch try_catch;
+  return v8::Undefined();
+}
+
+
+THREADED_TEST(TryCatchAndFinally) {
+  v8::HandleScope scope;
+  LocalContext context;
+  context->Global()->Set(
+      v8_str("native_with_try_catch"),
+      v8::FunctionTemplate::New(WithTryCatch)->GetFunction());
+  v8::TryCatch try_catch;
+  CHECK(!try_catch.HasCaught());
+  CompileRun(
+      "try {\n"
+      "  throw new Error('a');\n"
+      "} finally {\n"
+      "  native_with_try_catch();\n"
+      "}\n");
+  CHECK(try_catch.HasCaught());
+}
+
+
 THREADED_TEST(Equality) {
   v8::HandleScope scope;
   LocalContext context;