Revision: 6809
Author: [email protected]
Date: Wed Feb 16 03:40:48 2011
Log: 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
http://code.google.com/p/v8/source/detail?r=6809
Modified:
/branches/bleeding_edge/samples/shell.cc
/branches/bleeding_edge/src/d8.cc
/branches/bleeding_edge/src/top.cc
/branches/bleeding_edge/src/top.h
/branches/bleeding_edge/test/cctest/test-api.cc
=======================================
--- /branches/bleeding_edge/samples/shell.cc Wed Dec 8 02:42:32 2010
+++ /branches/bleeding_edge/samples/shell.cc Wed Feb 16 03:40:48 2011
@@ -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 @@
} 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.
=======================================
--- /branches/bleeding_edge/src/d8.cc Tue Jan 4 01:09:50 2011
+++ /branches/bleeding_edge/src/d8.cc Wed Feb 16 03:40:48 2011
@@ -127,11 +127,13 @@
} 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.
=======================================
--- /branches/bleeding_edge/src/top.cc Thu Feb 10 06:41:16 2011
+++ /branches/bleeding_edge/src/top.cc Wed Feb 16 03:40:48 2011
@@ -333,7 +333,7 @@
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::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 @@
}
-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 @@
// 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 @@
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 @@
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 @@
}
}
- 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
@@ -923,24 +928,65 @@
set_pending_exception(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 @@
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_;
}
=======================================
--- /branches/bleeding_edge/src/top.h Thu Feb 10 06:41:16 2011
+++ /branches/bleeding_edge/src/top.h Wed Feb 16 03:40:48 2011
@@ -249,12 +249,7 @@
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 @@
// 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 @@
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
=======================================
--- /branches/bleeding_edge/test/cctest/test-api.cc Tue Feb 15 02:39:22 2011
+++ /branches/bleeding_edge/test/cctest/test-api.cc Wed Feb 16 03:40:48 2011
@@ -2689,6 +2689,41 @@
Script::Compile(v8_str("var o = {}; with (o) { throw 42; }"))->Run();
CHECK(try_catch.HasCaught());
}
+
+
+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-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
