When message handler is set to NULL and there is no debugger listener the debugger...

Without the change the debugger may crash as Debugger::EventActive(v8::Break) called from OnDebugBreak may clear current debugger context.

Also when compilation cache was enabled debugger could fail on second attach for the same reason(see AfterCompileMessageWhenMessageHandlerIsReset).

BUG=12404
Review URL: http://codereview.chromium.org/115709

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

diff --git a/src/debug.cc b/src/debug.cc
index 7de6579..26c6647 100644 (file)
--- a/src/debug.cc
+++ b/src/debug.cc
@@ -1589,7 +1589,7 @@ bool Debugger::compiling_natives_ = false;
 bool Debugger::is_loading_debugger_ = false;
 bool Debugger::never_unload_debugger_ = false;
 v8::Debug::MessageHandler2 Debugger::message_handler_ = NULL;
-bool Debugger::message_handler_cleared_ = false;
+bool Debugger::debugger_unload_pending_ = false;
 v8::Debug::HostDispatchHandler Debugger::host_dispatch_handler_ = NULL;
 int Debugger::host_dispatch_micros_ = 100 * 1000;
 DebuggerAgent* Debugger::agent_ = NULL;
@@ -1981,8 +1981,8 @@ void Debugger::UnloadDebugger() {
     Debug::Unload();
   }
 
-  // Clear the flag indicating that the message handler was recently cleared.
-  message_handler_cleared_ = false;
+  // Clear the flag indicating that the debugger should be unloaded.
+  debugger_unload_pending_ = false;
 }
 
 
@@ -2169,17 +2169,7 @@ void Debugger::SetEventListener(Handle<Object> callback,
     event_listener_data_ = Handle<Object>::cast(GlobalHandles::Create(*data));
   }
 
-  // Unload the debugger if event listener cleared.
-  if (callback->IsUndefined()) {
-    UnloadDebugger();
-  }
-
-  // Disable the compilation cache when the debugger is active.
-  if (IsDebuggerActive()) {
-    CompilationCache::Disable();
-  } else {
-    CompilationCache::Enable();
-  }
+  ListenersChanged();
 }
 
 
@@ -2187,22 +2177,32 @@ void Debugger::SetMessageHandler(v8::Debug::MessageHandler2 handler) {
   ScopedLock with(debugger_access_);
 
   message_handler_ = handler;
+  ListenersChanged();
   if (handler == NULL) {
-    // Indicate that the message handler was recently cleared.
-    message_handler_cleared_ = true;
-
     // Send an empty command to the debugger if in a break to make JavaScript
     // run again if the debugger is closed.
     if (Debug::InDebugger()) {
       ProcessCommand(Vector<const uint16_t>::empty());
     }
   }
+}
 
-  // Disable the compilation cache when the debugger is active.
+
+void Debugger::ListenersChanged() {
   if (IsDebuggerActive()) {
+    // Disable the compilation cache when the debugger is active.
     CompilationCache::Disable();
   } else {
     CompilationCache::Enable();
+
+    // Unload the debugger if event listener and message handler cleared.
+    if (Debug::InDebugger()) {
+      // If we are in debugger set the flag to unload the debugger when last
+      // EnterDebugger on the current stack is destroyed.
+      debugger_unload_pending_ = true;
+    } else {
+      UnloadDebugger();
+    }
   }
 }
 

diff --git a/src/debug.h b/src/debug.h
index 3bc27b0..aee54ca 100644 (file)
--- a/src/debug.h
+++ b/src/debug.h
@@ -629,7 +629,7 @@ class Debugger {
     ScopedLock with(debugger_access_);
 
     // Check whether the message handler was been cleared.
-    if (message_handler_cleared_) {
+    if (debugger_unload_pending_) {
       UnloadDebugger();
     }
 
@@ -646,6 +646,7 @@ class Debugger {
 
  private:
   static bool IsDebuggerActive();
+  static void ListenersChanged();
 
   static Mutex* debugger_access_;  // Mutex guarding debugger variables.
   static Handle<Object> event_listener_;  // Global handle to listener.
@@ -654,7 +655,7 @@ class Debugger {
   static bool is_loading_debugger_;  // Are we loading the debugger?
   static bool never_unload_debugger_;  // Can we unload the debugger?
   static v8::Debug::MessageHandler2 message_handler_;
-  static bool message_handler_cleared_;  // Was message handler cleared?
+  static bool debugger_unload_pending_;  // Was message handler cleared?
   static v8::Debug::HostDispatchHandler host_dispatch_handler_;
   static int host_dispatch_micros_;
 

diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc
index 4afcc34..eff02fb 100644 (file)
--- a/test/cctest/test-debug.cc
+++ b/test/cctest/test-debug.cc
@@ -4693,6 +4693,9 @@ TEST(ContextData) {
 
   // Two times compile event and two times break event.
   CHECK_GT(message_handler_hit_count, 4);
+
+  v8::Debug::SetMessageHandler2(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -4739,11 +4742,14 @@ TEST(EvalContextData) {
 
   // One time compile event and one time break event.
   CHECK_GT(message_handler_hit_count, 2);
+  v8::Debug::SetMessageHandler2(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
 static bool sent_eval = false;
 static int break_count = 0;
+static int continue_command_send_count = 0;
 // Check that the expected context is the one generating the debug event
 // including the case of nested break event.
 static void DebugEvalContextCheckMessageHandler(
@@ -4773,11 +4779,13 @@ static void DebugEvalContextCheckMessageHandler(
     } else {
       // It's a break event caused by the evaluation request above.
       SendContinueCommand();
+      continue_command_send_count++;
     }
-  } else if (message.IsResponse()) {
+  } else if (message.IsResponse() && continue_command_send_count < 2) {
     // Response to the evaluation request. We're still on the breakpoint so
     // send continue.
     SendContinueCommand();
+    continue_command_send_count++;
   }
 }
 
@@ -4786,6 +4794,8 @@ static void DebugEvalContextCheckMessageHandler(
 // in 'evaluate' debugger request.
 TEST(NestedBreakEventContextData) {
   v8::HandleScope scope;
+  break_count = 0;
+  message_handler_hit_count = 0;
   v8::Debug::SetMessageHandler2(DebugEvalContextCheckMessageHandler);
 
   ExecuteScriptForContextCheck();
@@ -4795,6 +4805,8 @@ TEST(NestedBreakEventContextData) {
 
   // One break from the source and another from the evaluate request.
   CHECK_EQ(break_count, 2);
+  v8::Debug::SetMessageHandler2(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -4814,6 +4826,7 @@ static void DebugEventScriptCollectedEvent(v8::DebugEvent event,
 // Test that scripts collected are reported through the debug event listener.
 TEST(ScriptCollectedEvent) {
   break_point_hit_count = 0;
+  script_collected_count = 0;
   v8::HandleScope scope;
   DebugLocalContext env;
 
@@ -4858,7 +4871,7 @@ static void ScriptCollectedMessageHandler(const v8::Debug::Message& message) {
 // Test that GetEventContext doesn't fail and return empty handle for
 // ScriptCollected events.
 TEST(ScriptCollectedEventContext) {
-  break_point_hit_count = 0;
+  script_collected_message_count = 0;
   v8::HandleScope scope;
 
   { // Scope for the DebugLocalContext.
@@ -4871,7 +4884,6 @@ TEST(ScriptCollectedEventContext) {
     // collected afterwards.
     Heap::CollectAllGarbage();
 
-    script_collected_count = 0;
     v8::Debug::SetMessageHandler2(ScriptCollectedMessageHandler);
     {
       v8::Script::Compile(v8::String::New("eval('a=1')"))->Run();
@@ -4887,3 +4899,101 @@ TEST(ScriptCollectedEventContext) {
 
   v8::Debug::SetMessageHandler2(NULL);
 }
+
+
+// Debug event listener which counts the after compile events.
+int after_compile_message_count = 0;
+static void AfterCompileMessageHandler(const v8::Debug::Message& message) {
+  // Count the number of scripts collected.
+  if (message.IsEvent()) {
+    if (message.GetEvent() == v8::AfterCompile) {
+      after_compile_message_count++;
+    } else if (message.GetEvent() == v8::Break) {
+      SendContinueCommand();
+    }
+  }
+}
+
+
+// Tests that after compile event is sent as many times as there are scripts
+// compiled.
+TEST(AfterCompileMessageWhenMessageHandlerIsReset) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+  after_compile_message_count = 0;
+  const char* script = "var a=1";
+
+  v8::Debug::SetMessageHandler2(AfterCompileMessageHandler);
+  v8::Script::Compile(v8::String::New(script))->Run();
+  v8::Debug::SetMessageHandler2(NULL);
+
+  v8::Debug::SetMessageHandler2(AfterCompileMessageHandler);
+  v8::Debug::DebugBreak();
+  v8::Script::Compile(v8::String::New(script))->Run();
+
+  // Setting listener to NULL should cause debugger unload.
+  v8::Debug::SetMessageHandler2(NULL);
+  CheckDebuggerUnloaded();
+
+  // Compilation cache should be disabled when debugger is active.
+  CHECK_EQ(2, after_compile_message_count);
+}
+
+
+// Tests that break event is sent when message handler is reset.
+TEST(BreakMessageWhenMessageHandlerIsReset) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+  after_compile_message_count = 0;
+  const char* script = "function f() {};";
+
+  v8::Debug::SetMessageHandler2(AfterCompileMessageHandler);
+  v8::Script::Compile(v8::String::New(script))->Run();
+  v8::Debug::SetMessageHandler2(NULL);
+
+  v8::Debug::SetMessageHandler2(AfterCompileMessageHandler);
+  v8::Debug::DebugBreak();
+  v8::Local<v8::Function> f =
+      v8::Local<v8::Function>::Cast(env->Global()->Get(v8::String::New("f")));
+  f->Call(env->Global(), 0, NULL);
+
+  // Setting message handler to NULL should cause debugger unload.
+  v8::Debug::SetMessageHandler2(NULL);
+  CheckDebuggerUnloaded();
+
+  // Compilation cache should be disabled when debugger is active.
+  CHECK_EQ(1, after_compile_message_count);
+}
+
+
+static int exception_event_count = 0;
+static void ExceptionMessageHandler(const v8::Debug::Message& message) {
+  if (message.IsEvent() && message.GetEvent() == v8::Exception) {
+    exception_event_count++;
+    SendContinueCommand();
+  }
+}
+
+
+// Tests that exception event is sent when message handler is reset.
+TEST(ExceptionMessageWhenMessageHandlerIsReset) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+  exception_event_count = 0;
+  const char* script = "function f() {throw new Error()};";
+
+  v8::Debug::SetMessageHandler2(AfterCompileMessageHandler);
+  v8::Script::Compile(v8::String::New(script))->Run();
+  v8::Debug::SetMessageHandler2(NULL);
+
+  v8::Debug::SetMessageHandler2(ExceptionMessageHandler);
+  v8::Local<v8::Function> f =
+      v8::Local<v8::Function>::Cast(env->Global()->Get(v8::String::New("f")));
+  f->Call(env->Global(), 0, NULL);
+
+  // Setting message handler to NULL should cause debugger unload.
+  v8::Debug::SetMessageHandler2(NULL);
+  CheckDebuggerUnloaded();
+
+  CHECK_EQ(1, exception_event_count);
+}