Change handling of debugger unloading.

Add a semaphore for accessing debugger varaibles which can be changed from a different thread. This is mainly the debug message handler which can be set to NULL to disconnect the debugger.

Control the unloading of the debugger from the V8 thread. Before the debugger unload was called from the thread setting the debug message handler to NULL. This was not safe as this involves calling into V8. This change handles the unloading of the debugger either when entering a debugger event and the debugger was disconnected while the debugger was not active or when leaving the debugger and the debugger was disconnected while the debugger was active.

Add a flag to avoid unloading the debugger if debugger code is used by the application for other purposes than debugging.

Added tests for clearing the debug message handler.
Review URL: http://codereview.chromium.org/56102

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

src/debug.cc

@@ -607,9 +607,6 @@ void Debug::Unload() {
     return;
   }
 
-  // Get rid of all break points and related information.
-  ClearAllBreakPoints();
-
   // Clear debugger context global handle.
   GlobalHandles::Destroy(reinterpret_cast<Object**>(debug_context_.location()));
   debug_context_ = Handle<Context>();
@@ -1369,13 +1366,15 @@ void Debug::ClearMirrorCache() {
 }
 
 
+Mutex* Debugger::debugger_access_ = OS::CreateMutex();
 Handle<Object> Debugger::event_listener_ = Handle<Object>();
 Handle<Object> Debugger::event_listener_data_ = Handle<Object>();
-bool Debugger::debugger_active_ = false;
 bool Debugger::compiling_natives_ = false;
 bool Debugger::is_loading_debugger_ = false;
+bool Debugger::never_unload_debugger_ = false;
 DebugMessageThread* Debugger::message_thread_ = NULL;
 v8::DebugMessageHandler Debugger::message_handler_ = NULL;
+bool Debugger::message_handler_cleared_ = false;
 void* Debugger::message_handler_data_ = NULL;
 v8::DebugHostDispatchHandler Debugger::host_dispatch_handler_ = NULL;
 void* Debugger::host_dispatch_handler_data_ = NULL;
@@ -1581,7 +1580,7 @@ void Debugger::OnAfterCompile(Handle<Script> script, Handle<JSFunction> fun) {
   if (compiling_natives()) return;
 
   // No more to do if not debugging.
-  if (!debugger_active()) return;
+  if (!IsDebuggerActive()) return;
 
   // Store whether in debugger before entering debugger.
   bool in_debugger = Debug::InDebugger();
@@ -1710,6 +1709,20 @@ void Debugger::ProcessDebugEvent(v8::DebugEvent event,
 }
 
 
+void Debugger::UnloadDebugger() {
+  // Make sure that there are no breakpoints left.
+  Debug::ClearAllBreakPoints();
+
+  // Unload the debugger if feasible.
+  if (!never_unload_debugger_) {
+    Debug::Unload();
+  }
+
+  // Clear the flag indicating that the message handler was recently cleared.
+  message_handler_cleared_ = false;
+}
+
+
 void Debugger::NotifyMessageHandler(v8::DebugEvent event,
                                     Handle<Object> exec_state,
                                     Handle<Object> event_data,
@@ -1777,7 +1790,7 @@ void Debugger::NotifyMessageHandler(v8::DebugEvent event,
     // Get the command from the queue.
     Vector<uint16_t> command = command_queue_.Get();
     Logger::DebugTag("Got request from command queue, in interactive loop.");
-    if (!Debugger::debugger_active()) {
+    if (!Debugger::IsDebuggerActive()) {
       return;
     }
 
@@ -1880,19 +1893,34 @@ void Debugger::SetEventListener(Handle<Object> callback,
     event_listener_data_ = Handle<Object>::cast(GlobalHandles::Create(*data));
   }
 
-  UpdateActiveDebugger();
+  // Unload the debugger if event listener cleared.
+  if (callback->IsUndefined()) {
+    UnloadDebugger();
+  }
 }
 
 
 void Debugger::SetMessageHandler(v8::DebugMessageHandler handler, void* data,
                                  bool message_handler_thread) {
+  ScopedLock with(debugger_access_);
+
   message_handler_ = handler;
   message_handler_data_ = data;
-  if (!message_thread_ && message_handler_thread) {
-    message_thread_ = new DebugMessageThread();
-    message_thread_->Start();
+  if (handler != NULL) {
+    if (!message_thread_ && message_handler_thread) {
+      message_thread_ = new DebugMessageThread();
+      message_thread_->Start();
+    }
+  } else {
+    // 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());
+    }
   }
-  UpdateActiveDebugger();
 }
 
 
@@ -1908,6 +1936,8 @@ void Debugger::SetHostDispatchHandler(v8::DebugHostDispatchHandler handler,
 // are allocated in various places and deallocated by the calling function
 // sometime after this call.
 void Debugger::InvokeMessageHandler(Vector<uint16_t> message) {
+  ScopedLock with(debugger_access_);
+
   if (message_handler_ != NULL) {
     message_handler_(message.start(), message.length(), message_handler_data_);
   }
@@ -1999,22 +2029,19 @@ void Debugger::ProcessHostDispatch(void* dispatch) {
 }
 
 
-void Debugger::UpdateActiveDebugger() {
-  set_debugger_active(message_handler_ != NULL || !event_listener_.is_null());
-  if (!debugger_active() && message_thread_) {
-    // Send an empty command to the debugger if in a break to make JavaScript
-    // run again if the debugger is closed.
-    ProcessCommand(Vector<const uint16_t>::empty());
-  }
-  if (!debugger_active()) {
-    Debug::Unload();
-  }
+bool Debugger::IsDebuggerActive() {
+  ScopedLock with(debugger_access_);
+
+  return message_handler_ != NULL || !event_listener_.is_null();
 }
 
 
 Handle<Object> Debugger::Call(Handle<JSFunction> fun,
                               Handle<Object> data,
                               bool* pending_exception) {
+  // When calling functions in the debugger prevent it from beeing unloaded.
+  Debugger::never_unload_debugger_ = true;
+
   // Enter the debugger.
   EnterDebugger debugger;
   if (debugger.FailedToEnter() || !debugger.HasJavaScriptFrames()) {

src/debug.h

@@ -495,7 +495,6 @@ class Debugger {
   static bool HasCommands();
 
   static void ProcessHostDispatch(void* dispatch);
-  static void UpdateActiveDebugger();
   static Handle<Object> Call(Handle<JSFunction> fun,
                              Handle<Object> data,
                              bool* pending_exception);
@@ -506,15 +505,22 @@ class Debugger {
   // Stop the debugger agent.
   static void StopAgent();
 
+  // Unload the debugger if possible. Only called when no debugger is currently
+  // active.
+  static void UnloadDebugger();
+
   inline static bool EventActive(v8::DebugEvent event) {
+    ScopedLock with(debugger_access_);
+
+    // Check whether the message handler was been cleared.
+    if (message_handler_cleared_) {
+      UnloadDebugger();
+    }
+
     // Currently argument event is not used.
-    return !Debugger::compiling_natives_ && Debugger::debugger_active_;
+    return !compiling_natives_ && Debugger::IsDebuggerActive();
   }
 
-  static void set_debugger_active(bool debugger_active) {
-    Debugger::debugger_active_ = debugger_active;
-  }
-  static bool debugger_active() { return Debugger::debugger_active_; }
   static void set_compiling_natives(bool compiling_natives) {
     Debugger::compiling_natives_ = compiling_natives;
   }
@@ -523,13 +529,17 @@ class Debugger {
   static bool is_loading_debugger() { return Debugger::is_loading_debugger_; }
 
  private:
-  static Handle<Object> event_listener_;  // Global handle to listener
+  static bool IsDebuggerActive();
+
+  static Mutex* debugger_access_;  // Mutex guarding debugger variables.
+  static Handle<Object> event_listener_;  // Global handle to listener.
   static Handle<Object> event_listener_data_;
-  static bool debugger_active_;  // Are there any active debugger?
   static bool compiling_natives_;  // Are we compiling natives?
   static bool is_loading_debugger_;  // Are we loading the debugger?
+  static bool never_unload_debugger_;  // Can we unload the debugger?
   static DebugMessageThread* message_thread_;
   static v8::DebugMessageHandler message_handler_;
+  static bool message_handler_cleared_;  // Was message handler cleared?
   static void* message_handler_data_;
   static v8::DebugHostDispatchHandler host_dispatch_handler_;
   static void* host_dispatch_handler_data_;
@@ -542,6 +552,7 @@ class Debugger {
   static Semaphore* command_received_;  // Signaled for each command received.
   static Semaphore* message_received_;  // Signalled for each message send.
 
+  friend class EnterDebugger;
   friend class DebugMessageThread;
 };
 
@@ -618,6 +629,13 @@ class EnterDebugger BASE_EMBEDDED {
       StackGuard::DebugCommand();
     }
 
+    // If leaving the debugger with the debugger no longer active unload it.
+    if (prev_ == NULL) {
+      if (!Debugger::IsDebuggerActive()) {
+        Debugger::UnloadDebugger();
+      }
+    }
+
     // Leaving this debugger entry.
     Debug::set_debugger_entry(prev_);
   }

test/cctest/test-debug.cc

@@ -390,12 +390,6 @@ static Handle<Code> ComputeCallDebugBreak(int argc) {
 }
 
 
-// Check that the debugger is loaded.
-static void CheckDebuggerLoaded() {
-  CHECK(Debug::debug_context().is_null());
-}
-
-
 // Check that the debugger has been fully unloaded.
 void CheckDebuggerUnloaded(bool check_functions) {
   // Check that the debugger context is cleared and that there is no debug
@@ -437,12 +431,6 @@ void CheckDebuggerUnloaded(bool check_functions) {
 } }  // namespace v8::internal
 
 
-// Check that the debugger is loaded.
-static void CheckDebuggerLoaded() {
-  v8::internal::CheckDebuggerLoaded();
-}
-
-
 // Check that the debugger has been fully unloaded.
 static void CheckDebuggerUnloaded(bool check_functions = false) {
   v8::internal::CheckDebuggerUnloaded(check_functions);
@@ -3740,8 +3728,6 @@ TEST(DebuggerUnload) {
   // Add debug event listener.
   v8::Debug::SetDebugEventListener(DebugEventBreakPointHitCount,
                                    v8::Undefined());
-  CheckDebuggerLoaded();
-
   // Create a couple of functions for the test.
   v8::Local<v8::Function> foo =
       CompileFunction(&env, "function foo(){x=1}", "foo");
@@ -3768,8 +3754,6 @@ TEST(DebuggerUnload) {
   // Set a new debug event listener.
   v8::Debug::SetDebugEventListener(DebugEventBreakPointHitCount,
                                    v8::Undefined());
-  CheckDebuggerLoaded();
-
   // Check that the break points was actually cleared.
   break_point_hit_count = 0;
   foo->Call(env->Global(), 0, NULL);
@@ -3787,6 +3771,89 @@ TEST(DebuggerUnload) {
 }
 
 
+// Debugger message handler which counts the number of times it is called.
+static int message_handler_hit_count = 0;
+static void MessageHandlerHitCount(const uint16_t* message,
+                                   int length, void* data) {
+  message_handler_hit_count++;
+
+  const int kBufferSize = 1000;
+  uint16_t buffer[kBufferSize];
+  const char* command_continue =
+    "{\"seq\":0,"
+     "\"type\":\"request\","
+     "\"command\":\"continue\"}";
+
+  v8::Debug::SendCommand(buffer, AsciiToUtf16(command_continue, buffer));
+}
+
+
+// Test clearing the debug message handler.
+TEST(DebuggerClearMessageHandler) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+
+  // Check debugger is unloaded before it is used.
+  CheckDebuggerUnloaded();
+
+  // Set a debug message handler.
+  v8::Debug::SetMessageHandler(MessageHandlerHitCount);
+
+  // Run code to throw a unhandled exception. This should end up in the message
+  // handler.
+  CompileRun("throw 1");
+
+  // The message handler should be called.
+  CHECK_GT(message_handler_hit_count, 0);
+
+  // Clear debug message handler.
+  message_handler_hit_count = 0;
+  v8::Debug::SetMessageHandler(NULL);
+
+  // Run code to throw a unhandled exception. This should end up in the message
+  // handler.
+  CompileRun("throw 1");
+
+  // The message handler should not be called more.
+  CHECK_EQ(0, message_handler_hit_count);
+
+  CheckDebuggerUnloaded(true);
+}
+
+
+// Debugger message handler which clears the message handler while active.
+static void MessageHandlerClearingMessageHandler(const uint16_t* message,
+                                                 int length,
+                                                 void* data) {
+  message_handler_hit_count++;
+
+  // Clear debug message handler.
+  v8::Debug::SetMessageHandler(NULL);
+}
+
+
+// Test clearing the debug message handler while processing a debug event.
+TEST(DebuggerClearMessageHandlerWhileActive) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+
+  // Check debugger is unloaded before it is used.
+  CheckDebuggerUnloaded();
+
+  // Set a debug message handler.
+  v8::Debug::SetMessageHandler(MessageHandlerClearingMessageHandler);
+
+  // Run code to throw a unhandled exception. This should end up in the message
+  // handler.
+  CompileRun("throw 1");
+
+  // The message handler should be called.
+  CHECK_EQ(1, message_handler_hit_count);
+
+  CheckDebuggerUnloaded(true);
+}
+
+
 int host_dispatch_hit_count = 0;
 static void HostDispatchHandlerHitCount(void* dispatch, void *data) {
   host_dispatch_hit_count++;