Reviewers: Michael Starzinger, vsevik, Yang,
Message:
Yang and Michael, Could you please have a look on last patch set?
Description:
This patch continues https://codereview.chromium.org/306463002/
First patch set is last reviewed patch set from
https://codereview.chromium.org/306463002/
Second patch set have fixed bug with ASSERT(!handler->is_catch()) in
Isolate::IsFinallyOnTop(). It is essentially to not check IsFinallyOnTop in
all
case except has_external_try_catch is true and is_catchable_by_javascript is
true.
V8 can clear exception pending message, when should not do this.
The case:
v8::TryCatch try_catch;
CompileRun(try { CEvaluate('throw 1;'); } finally {});
CHECK(try_catch.HasCaught());
CHECK(!try_catch.Message().IsEmpty());
CEvaluate is native call. Last check is not passed without patch. Patch
contains
test TryCatchFinallyStoresMessageUsingTryCatchHandler with more details.
[email protected], [email protected], [email protected]
Please review this at https://codereview.chromium.org/321763002/
SVN Base: git://github.com/v8/v8.git@master
Affected files (+67, -21 lines):
M src/isolate.h
M src/isolate.cc
M test/cctest/test-api.cc
Index: src/isolate.cc
diff --git a/src/isolate.cc b/src/isolate.cc
index
3542b994ff3338ffd3860b763f49b4730dfe6d54..da956b6e25c84422c874ebdf248b0160d565c3a9
100644
--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -1144,20 +1144,15 @@ void Isolate::DoThrow(Object* exception,
MessageLocation* location) {
}
-bool Isolate::IsExternallyCaught() {
+bool Isolate::HasExternalTryCatch() {
ASSERT(has_pending_exception());
- if ((thread_local_top()->catcher_ == NULL) ||
- (try_catch_handler() != thread_local_top()->catcher_)) {
- // When throwing the exception, we found no v8::TryCatch
- // which should care about this exception.
- return false;
- }
+ return (thread_local_top()->catcher_ != NULL) &&
+ (try_catch_handler() == thread_local_top()->catcher_);
+}
- if (!is_catchable_by_javascript(pending_exception())) {
- return true;
- }
+bool Isolate::IsFinallyOnTop() {
// 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 =
@@ -1177,18 +1172,18 @@ bool Isolate::IsExternallyCaught() {
StackHandler::FromAddress(Isolate::handler(thread_local_top()));
while (handler != NULL && handler->address() < external_handler_address)
{
ASSERT(!handler->is_catch());
- if (handler->is_finally()) return false;
+ if (handler->is_finally()) return true;
handler = handler->next();
}
- return true;
+ return false;
}
void Isolate::ReportPendingMessages() {
ASSERT(has_pending_exception());
- PropagatePendingExceptionToExternalTryCatch();
+ bool can_clear_message = PropagatePendingExceptionToExternalTryCatch();
HandleScope scope(this);
if (thread_local_top_.pending_exception_ ==
@@ -1215,7 +1210,7 @@ void Isolate::ReportPendingMessages() {
}
}
}
- clear_pending_message();
+ if (can_clear_message) clear_pending_message();
}
@@ -1722,14 +1717,25 @@ void Isolate::InitializeThreadLocal() {
}
-void Isolate::PropagatePendingExceptionToExternalTryCatch() {
+bool Isolate::PropagatePendingExceptionToExternalTryCatch() {
ASSERT(has_pending_exception());
- bool external_caught = IsExternallyCaught();
- thread_local_top_.external_caught_exception_ = external_caught;
+ bool has_external_try_catch = HasExternalTryCatch();
+ if (!has_external_try_catch) {
+ thread_local_top_.external_caught_exception_ = false;
+ return true;
+ }
- if (!external_caught) return;
+ bool is_catchable_by_js =
is_catchable_by_javascript(pending_exception());
+ if (is_catchable_by_js) {
+ bool is_finally_on_top = IsFinallyOnTop();
+ if (is_finally_on_top) {
+ thread_local_top_.external_caught_exception_ = false;
+ return false;
+ }
+ }
+ thread_local_top_.external_caught_exception_ = true;
if (thread_local_top_.pending_exception_ ==
heap()->termination_exception()) {
try_catch_handler()->can_continue_ = false;
@@ -1745,13 +1751,14 @@ void
Isolate::PropagatePendingExceptionToExternalTryCatch() {
handler->has_terminated_ = false;
handler->exception_ = pending_exception();
// Propagate to the external try-catch only if we got an actual
message.
- if (thread_local_top_.pending_message_obj_->IsTheHole()) return;
+ if (thread_local_top_.pending_message_obj_->IsTheHole()) return true;
handler->message_obj_ = thread_local_top_.pending_message_obj_;
handler->message_script_ = thread_local_top_.pending_message_script_;
handler->message_start_pos_ =
thread_local_top_.pending_message_start_pos_;
handler->message_end_pos_ = thread_local_top_.pending_message_end_pos_;
}
+ return true;
}
Index: src/isolate.h
diff --git a/src/isolate.h b/src/isolate.h
index
bc7646bd4834bafc03bf333f53da14ee072ac451..e33849d01946694484dfde087bc4cc69ee983855
100644
--- a/src/isolate.h
+++ b/src/isolate.h
@@ -605,7 +605,8 @@ class Isolate {
thread_local_top_.scheduled_exception_ = heap_.the_hole_value();
}
- bool IsExternallyCaught();
+ bool HasExternalTryCatch();
+ bool IsFinallyOnTop();
bool is_catchable_by_javascript(Object* exception) {
return exception != heap()->termination_exception();
@@ -1178,7 +1179,10 @@ class Isolate {
void FillCache();
- void PropagatePendingExceptionToExternalTryCatch();
+ // Propagate pending exception message to the v8::TryCatch.
+ // If there is no external try-catch or message was successfully
propagated,
+ // then return true.
+ bool PropagatePendingExceptionToExternalTryCatch();
void InitializeDebugger();
Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index
5f82bd7f2e7ee184975588e4de3e6b7fcebbe325..e5707c7b578e49299323a80f14993410dfa3991c
100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -8377,6 +8377,41 @@ TEST(TryCatchFinallyUsingTryCatchHandler) {
}
+void CEvaluate(const v8::FunctionCallbackInfo<v8::Value>& args) {
+ v8::HandleScope scope(args.GetIsolate());
+ CompileRun(args[0]->ToString());
+}
+
+
+TEST(TryCatchFinallyStoresMessageUsingTryCatchHandler) {
+ v8::Isolate* isolate = CcTest::isolate();
+ v8::HandleScope scope(isolate);
+ Local<ObjectTemplate> templ = ObjectTemplate::New(isolate);
+ templ->Set(v8_str("CEvaluate"),
+ v8::FunctionTemplate::New(isolate, CEvaluate));
+ LocalContext context(0, templ);
+ v8::TryCatch try_catch;
+ CompileRun("try {"
+ " CEvaluate('throw 1;');"
+ "} finally {"
+ "}");
+ CHECK(try_catch.HasCaught());
+ CHECK(!try_catch.Message().IsEmpty());
+ String::Utf8Value exception_value(try_catch.Exception());
+ CHECK_EQ(*exception_value, "1");
+ try_catch.Reset();
+ CompileRun("try {"
+ " CEvaluate('throw 1;');"
+ "} finally {"
+ " throw 2;"
+ "}");
+ CHECK(try_catch.HasCaught());
+ CHECK(!try_catch.Message().IsEmpty());
+ String::Utf8Value finally_exception_value(try_catch.Exception());
+ CHECK_EQ(*finally_exception_value, "2");
+}
+
+
// For use within the TestSecurityHandler() test.
static bool g_security_callback_result = false;
static bool NamedSecurityTestCallback(Local<v8::Object> global,
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
