From: antonm@chromium.org Date: Wed, 16 Feb 2011 11:40:48 +0000 (+0000) Subject: Properly process try/finally blocks. X-Git-Tag: upstream/4.7.83~20192 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=6b4ff18b5bc0c876f48c6e30d9385d0b20883ab0;p=platform%2Fupstream%2Fv8.git 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 --- a/samples/shell.cc +++ b/samples/shell.cc @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -290,11 +291,13 @@ bool ExecuteString(v8::Handle source, } else { v8::Handle 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 --- a/src/d8.cc +++ b/src/d8.cc @@ -127,11 +127,13 @@ bool Shell::ExecuteString(Handle source, } else { Handle 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 --- 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
(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 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 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 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 --- 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 --- 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 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;