Fix crash in ScriptDebugServer::wrapCallFrames

test/cctest/test-debug.cc

Index: test/cctest/test-debug.cc
diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc
index 048de0b1c2c3ca2f9c8e917722bfe411bcf1a372..60b4348bab98e396021aab1c21d062d48d6684e4 100644
--- a/test/cctest/test-debug.cc
+++ b/test/cctest/test-debug.cc
@@ -669,6 +669,8 @@ static void DebugEventBreakPointHitCount(
 int exception_hit_count = 0;
 int uncaught_exception_hit_count = 0;
 int last_js_stack_height = -1;
+v8::Handle<v8::Function> debug_event_listener_callback;
+int debug_event_listener_callback_result;
 
 static void DebugEventCounterClear() {
   break_point_hit_count = 0;
@@ -709,9 +711,17 @@ static void DebugEventCounter(
     static const int kArgc = 1;
     v8::Handle<v8::Value> argv[kArgc] = { exec_state };
     // Using exec_state as receiver is just to have a receiver.
-    v8::Handle<v8::Value> result =  frame_count->Call(exec_state, kArgc, argv);
+    v8::Handle<v8::Value> result = frame_count->Call(exec_state, kArgc, argv);
     last_js_stack_height = result->Int32Value();
   }
+
+  // Run callback from DebugEventListener and check the result.
+  if (!debug_event_listener_callback.IsEmpty()) {
+    v8::Handle<v8::Value> result =
+        debug_event_listener_callback->Call(event_data, 0, NULL);
+    CHECK(!result.IsEmpty());
+    CHECK_EQ(debug_event_listener_callback_result, result->Int32Value());
+  }
 }
 
 
@@ -3967,6 +3977,52 @@ TEST(BreakOnException) {
 }
 
 
+TEST(EvalJSInDebugEventListenerOnNativeReThrownException) {
+  DebugLocalContext env;
+  v8::HandleScope scope(env->GetIsolate());
+  env.ExposeDebug();
+
+  // Create functions for testing break on exception.
+  v8::Local<v8::Function> reThrowJS = CompileFunction(
+      &env, "function reThrowJS(){try {throw 1;} finally {};}", "reThrowJS");
+  v8::Local<v8::Function> noThrowJS = CompileFunction(
+      &env, "function noThrowJS(){var a=[1]; a.push(2); return a.length;}",
+      "noThrowJS");
+
+  debug_event_listener_callback = noThrowJS;
+  debug_event_listener_callback_result = 2;
+
+  v8::V8::AddMessageListener(MessageCallbackCount);
+  v8::Debug::SetDebugEventListener(DebugEventCounter);
+  // Break on uncaught exception
+  ChangeBreakOnException(false, true);
+  DebugEventCounterClear();
+  MessageCallbackCountClear();
+
+  // ReThrow error from JavaScript
+  reThrowJS->Call(env->Global(), 0, NULL);
+  CHECK_EQ(1, exception_hit_count);
+  CHECK_EQ(1, uncaught_exception_hit_count);
+  CHECK_EQ(1, message_callback_count);
+  CHECK(!debug_event_listener_callback.IsEmpty());
+
+  // ReThrow native error
+  {
+    v8::TryCatch tryCatch;
+    env->GetIsolate()->ThrowException(v8::Exception::TypeError(
+        v8::String::NewFromUtf8(env->GetIsolate(), "Type error")));
+    CHECK(tryCatch.HasCaught());
+    tryCatch.ReThrow();
+  }
+  CHECK_EQ(2, exception_hit_count);
+  CHECK_EQ(2, uncaught_exception_hit_count);
+  CHECK_EQ(1, message_callback_count);  // FIXME: Should it be 2 ?

[Yang, 2014/09/10 14:46:13]

I think 1 is correct.

[aandrey, 2014/09/10 14:51:19]

hm... why? the native version of JS's try-finally should report a message as
well, right?

[Yang, 2014/09/10 14:56:29]

If I understood correctly, the exception thrown in reThrowJS is caught by the
TryCatch scope, so it's a caught exception, for which we don't trigger an event.
Only on ReThrow, the destructor of the TryCatch triggers an uncaught exception
event.

[aandrey, 2014/09/10 15:01:53]

Right, but destructor should be already executed by this moment.

[aandrey, 2014/09/11 05:48:29]

Note, that if I remove the v8::TryCatch and call just ThrowException(), the
message_callback_count is still 1.

+  CHECK(!debug_event_listener_callback.IsEmpty());
+
+  debug_event_listener_callback.Clear();
+}
+
+
 // Test break on exception from compiler errors. When compiling using
 // v8::Script::Compile there is no JavaScript stack whereas when compiling using
 // eval there are JavaScript frames.