Author: [email protected]
Date: Tue Mar 31 04:24:59 2009
New Revision: 1648

Modified:
    branches/bleeding_edge/src/debug.cc
    branches/bleeding_edge/src/debug.h
    branches/bleeding_edge/test/cctest/test-debug.cc

Log:
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

Modified: branches/bleeding_edge/src/debug.cc
==============================================================================
--- branches/bleeding_edge/src/debug.cc (original)
+++ branches/bleeding_edge/src/debug.cc Tue Mar 31 04:24:59 2009
@@ -607,9 +607,6 @@
      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 @@
  }


+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 @@
    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::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 @@
      // 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 @@
      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 @@
  // 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::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()) {

Modified: branches/bleeding_edge/src/debug.h
==============================================================================
--- branches/bleeding_edge/src/debug.h  (original)
+++ branches/bleeding_edge/src/debug.h  Tue Mar 31 04:24:59 2009
@@ -495,7 +495,6 @@
    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 @@
    // 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 @@
    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 @@
    static Semaphore* command_received_;  // Signaled for each command  
received.
    static Semaphore* message_received_;  // Signalled for each message send.

+  friend class EnterDebugger;
    friend class DebugMessageThread;
  };

@@ -616,6 +627,13 @@
      // these commands are processed.
      if (prev_ == NULL && Debugger::HasCommands()) {
        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.

Modified: branches/bleeding_edge/test/cctest/test-debug.cc
==============================================================================
--- branches/bleeding_edge/test/cctest/test-debug.cc    (original)
+++ branches/bleeding_edge/test/cctest/test-debug.cc    Tue Mar 31 04:24:59  
2009
@@ -390,12 +390,6 @@
  }


-// 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 @@
  } }  // 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 @@
    // 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 @@
    // 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);
@@ -3783,6 +3767,89 @@

    // Remove the debug event listener without clearing breakpoints again.
    v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded(true);
+}
+
+
+// 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);
  }


--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---

Reply via email to