As long as all debugger messages are handled by a single static method we need...

As long as all debugger messages are handled by a single static method we need a way to identify request sender to route the response to the right handler. To accomplish this clients can send some additional data along with command text and debugger will later pass this data to the message handler along with the response text.

Review URL: http://codereview.chromium.org/67266

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

include/v8-debug.h

@@ -79,48 +79,60 @@ enum DebugEvent {
 };
 
 
-/**
- * Debug event callback function.
- *
- * \param event the type of the debug event that triggered the callback
- *   (enum DebugEvent)
- * \param exec_state execution state (JavaScript object)
- * \param event_data event specific data (JavaScript object)
- * \param data value passed by the user to SetDebugEventListener
- */
-typedef void (*DebugEventCallback)(DebugEvent event,
-                                   Handle<Object> exec_state,
-                                   Handle<Object> event_data,
-                                   Handle<Value> data);
-
-
-/**
- * Debug message callback function.
- *
- * \param message the debug message
- * \param length length of the message
- * \param data the data value passed when registering the message handler
- * A DebugMessageHandler does not take posession of the message string,
- * and must not rely on the data persisting after the handler returns.
- */
-typedef void (*DebugMessageHandler)(const uint16_t* message, int length,
-                                    void* data);
-
-/**
- * Debug host dispatch callback function.
- *
- * \param dispatch the dispatch value
- * \param data the data value passed when registering the dispatch handler
- */
-typedef void (*DebugHostDispatchHandler)(void* dispatch,
-                                         void* data);
-
-
-
 class EXPORT Debug {
  public:
+  /**
+   * A client object passed to the v8 debugger whose ownership will be taken by
+   * it. v8 is always responsible for deleting the object.
+   */
+  class ClientData {
+   public:
+    virtual ~ClientData() {}
+  };
+
+
+  /**
+   * Debug event callback function.
+   *
+   * \param event the type of the debug event that triggered the callback
+   *   (enum DebugEvent)
+   * \param exec_state execution state (JavaScript object)
+   * \param event_data event specific data (JavaScript object)
+   * \param data value passed by the user to SetDebugEventListener
+   */
+  typedef void (*EventCallback)(DebugEvent event,
+                                     Handle<Object> exec_state,
+                                     Handle<Object> event_data,
+                                     Handle<Value> data);
+
+
+  /**
+   * Debug message callback function.
+   *
+   * \param message the debug message
+   * \param length length of the message
+   * \param data the data value passed when registering the message handler
+   * \param client_data the data value passed into Debug::SendCommand along
+   *     with the request that led to the message or NULL if the message is an
+   *     asynchronous event. The debugger takes ownership of the data and will
+   *     delete it before dying even if there is no message handler.
+   * A MessageHandler does not take posession of the message string,
+   * and must not rely on the data persisting after the handler returns.
+   */
+  typedef void (*MessageHandler)(const uint16_t* message, int length,
+                                      ClientData* client_data);
+
+  /**
+   * Debug host dispatch callback function.
+   *
+   * \param dispatch the dispatch value
+   * \param data the data value passed when registering the dispatch handler
+   */
+  typedef void (*HostDispatchHandler)(ClientData* dispatch);
+
+
   // Set a C debug event listener.
-  static bool SetDebugEventListener(DebugEventCallback that,
+  static bool SetDebugEventListener(EventCallback that,
                                     Handle<Value> data = Handle<Value>());
 
   // Set a JavaScript debug event listener.
@@ -131,14 +143,14 @@ class EXPORT Debug {
   static void DebugBreak();
 
   // Message based interface. The message protocol is JSON.
-  static void SetMessageHandler(DebugMessageHandler handler, void* data = NULL,
+  static void SetMessageHandler(MessageHandler handler,
                                 bool message_handler_thread = true);
-  static void SendCommand(const uint16_t* command, int length);
+  static void SendCommand(const uint16_t* command, int length,
+                          ClientData* client_data = NULL);
 
   // Dispatch interface.
-  static void SetHostDispatchHandler(DebugHostDispatchHandler handler,
-                                     void* data = NULL);
-  static void SendHostDispatch(void* dispatch);
+  static void SetHostDispatchHandler(HostDispatchHandler handler);
+  static void SendHostDispatch(ClientData* dispatch);
 
  /**
   * Run a JavaScript function in the debugger.

src/api.cc

@@ -3230,7 +3230,7 @@ Local<Value> Exception::Error(v8::Handle<v8::String> raw_message) {
 // --- D e b u g   S u p p o r t ---
 
 #ifdef ENABLE_DEBUGGER_SUPPORT
-bool Debug::SetDebugEventListener(DebugEventCallback that, Handle<Value> data) {
+bool Debug::SetDebugEventListener(EventCallback that, Handle<Value> data) {
   EnsureInitialized("v8::Debug::SetDebugEventListener()");
   ON_BAILOUT("v8::Debug::SetDebugEventListener()", return false);
   ENTER_V8;
@@ -3260,29 +3260,30 @@ void Debug::DebugBreak() {
 }
 
 
-void Debug::SetMessageHandler(v8::DebugMessageHandler handler, void* data,
+void Debug::SetMessageHandler(v8::Debug::MessageHandler handler,
                               bool message_handler_thread) {
   EnsureInitialized("v8::Debug::SetMessageHandler");
   ENTER_V8;
-  i::Debugger::SetMessageHandler(handler, data, message_handler_thread);
+  i::Debugger::SetMessageHandler(handler, message_handler_thread);
 }
 
 
-void Debug::SendCommand(const uint16_t* command, int length) {
+void Debug::SendCommand(const uint16_t* command, int length,
+                        ClientData* client_data) {
   if (!i::V8::HasBeenSetup()) return;
-  i::Debugger::ProcessCommand(i::Vector<const uint16_t>(command, length));
+  i::Debugger::ProcessCommand(i::Vector<const uint16_t>(command, length),
+                              client_data);
 }
 
 
-void Debug::SetHostDispatchHandler(v8::DebugHostDispatchHandler handler,
-                                   void* data) {
+void Debug::SetHostDispatchHandler(HostDispatchHandler handler) {
   EnsureInitialized("v8::Debug::SetHostDispatchHandler");
   ENTER_V8;
-  i::Debugger::SetHostDispatchHandler(handler, data);
+  i::Debugger::SetHostDispatchHandler(handler);
 }
 
 
-void Debug::SendHostDispatch(void* dispatch) {
+void Debug::SendHostDispatch(ClientData* dispatch) {
   if (!i::V8::HasBeenSetup()) return;
   i::Debugger::ProcessHostDispatch(dispatch);
 }

src/debug-agent.cc

@@ -35,10 +35,12 @@ namespace v8 { namespace internal {
 // Public V8 debugger API message handler function. This function just delegates
 // to the debugger agent through it's data parameter.
 void DebuggerAgentMessageHandler(const uint16_t* message, int length,
-                                 void *data) {
-  reinterpret_cast<DebuggerAgent*>(data)->DebuggerMessage(message, length);
+                                 v8::Debug::ClientData* client_data) {
+  DebuggerAgent::instance_->DebuggerMessage(message, length);
 }
 
+// static
+DebuggerAgent* DebuggerAgent::instance_ = NULL;
 
 // Debugger agent main thread.
 void DebuggerAgent::Run() {

src/debug-agent.h

@@ -46,8 +46,14 @@ class DebuggerAgent: public Thread {
       : name_(StrDup(name)), port_(port),
         server_(OS::CreateSocket()), terminate_(false),
         session_access_(OS::CreateMutex()), session_(NULL),
-        terminate_now_(OS::CreateSemaphore(0)) {}
-  ~DebuggerAgent() { delete server_; }
+        terminate_now_(OS::CreateSemaphore(0)) {
+    ASSERT(instance_ == NULL);
+    instance_ = this;
+  }
+  ~DebuggerAgent() {
+     instance_ = NULL;
+     delete server_;
+  }
 
   void Shutdown();
 
@@ -66,9 +72,11 @@ class DebuggerAgent: public Thread {
   DebuggerAgentSession* session_;  // Current active session if any.
   Semaphore* terminate_now_;  // Semaphore to signal termination.
 
+  static DebuggerAgent* instance_;
+
   friend class DebuggerAgentSession;
   friend void DebuggerAgentMessageHandler(const uint16_t* message, int length,
-                                          void *data);
+                                          v8::Debug::ClientData* client_data);
 
   DISALLOW_COPY_AND_ASSIGN(DebuggerAgent);
 };

src/debug.h

@@ -397,6 +397,36 @@ class Debug {
 };
 
 
+// Message send by user to v8 debugger or debugger output message.
+// In addition to command text it may contain a pointer to some user data
+// which are expected to be passed along with the command reponse to message
+// handler.
+class Message {
+ public:
+  static Message NewCommand(const Vector<uint16_t>& command,
+                            v8::Debug::ClientData* data);
+  static Message NewHostDispatch(v8::Debug::ClientData* dispatch);
+  static Message NewOutput(v8::Handle<v8::String> output,
+                           v8::Debug::ClientData* data);
+  static Message NewEmptyMessage();
+  Message();
+  ~Message();
+
+  // Deletes user data and disposes of the text.
+  void Dispose();
+  bool IsHostDispatch() const;
+  Vector<uint16_t> text() const { return text_; }
+  v8::Debug::ClientData* client_data() const { return client_data_; }
+ private:
+  Message(const Vector<uint16_t>& text,
+          v8::Debug::ClientData* data,
+          bool is_host_dispatch);
+
+  Vector<uint16_t> text_;
+  v8::Debug::ClientData* client_data_;
+  bool is_host_dispatch_;
+};
+
 // A Queue of Vector<uint16_t> objects.  A thread-safe version is
 // LockingMessageQueue, based on this class.
 class MessageQueue BASE_EMBEDDED {
@@ -404,14 +434,14 @@ class MessageQueue BASE_EMBEDDED {
   explicit MessageQueue(int size);
   ~MessageQueue();
   bool IsEmpty() const { return start_ == end_; }
-  Vector<uint16_t> Get();
-  void Put(const Vector<uint16_t>& message);
+  Message Get();
+  void Put(const Message& message);
   void Clear() { start_ = end_ = 0; }  // Queue is empty after Clear().
  private:
   // Doubles the size of the message queue, and copies the messages.
   void Expand();
 
-  Vector<uint16_t>* messages_;
+  Message* messages_;
   int start_;
   int end_;
   int size_;  // The size of the queue buffer.  Queue can hold size-1 messages.
@@ -427,8 +457,8 @@ class LockingMessageQueue BASE_EMBEDDED {
   explicit LockingMessageQueue(int size);
   ~LockingMessageQueue();
   bool IsEmpty() const;
-  Vector<uint16_t> Get();
-  void Put(const Vector<uint16_t>& message);
+  Message Get();
+  void Put(const Message& message);
   void Clear();
  private:
   MessageQueue queue_;
@@ -473,29 +503,29 @@ class Debugger {
                                    Handle<Object> event_data,
                                    bool auto_continue);
   static void SetEventListener(Handle<Object> callback, Handle<Object> data);
-  static void SetMessageHandler(v8::DebugMessageHandler handler, void* data,
+  static void SetMessageHandler(v8::Debug::MessageHandler handler,
                                 bool message_handler_thread);
   static void TearDown();
-  static void SetHostDispatchHandler(v8::DebugHostDispatchHandler handler,
-                                     void* data);
+  static void SetHostDispatchHandler(v8::Debug::HostDispatchHandler handler);
 
   // Invoke the message handler function.
-  static void InvokeMessageHandler(Vector< uint16_t> message);
+  static void InvokeMessageHandler(Message message);
 
   // Send a message to the message handler eiher through the message thread or
   // directly.
-  static void SendMessage(Vector<uint16_t> message);
+  static void SendMessage(Message message);
 
   // Send the JSON message for a debug event.
   static bool SendEventMessage(Handle<Object> event_data);
 
   // Add a debugger command to the command queue.
-  static void ProcessCommand(Vector<const uint16_t> command);
+  static void ProcessCommand(Vector<const uint16_t> command,
+                             v8::Debug::ClientData* client_data = NULL);
 
   // Check whether there are commands in the command queue.
   static bool HasCommands();
 
-  static void ProcessHostDispatch(void* dispatch);
+  static void ProcessHostDispatch(v8::Debug::ClientData* dispatch);
   static Handle<Object> Call(Handle<JSFunction> fun,
                              Handle<Object> data,
                              bool* pending_exception);
@@ -539,11 +569,9 @@ class Debugger {
   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 v8::Debug::MessageHandler 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_;
+  static v8::Debug::HostDispatchHandler host_dispatch_handler_;
 
   static DebuggerAgent* agent_;
 

test/cctest/test-debug.cc

@@ -45,10 +45,13 @@ using ::v8::internal::JSGlobalProxy;
 using ::v8::internal::Code;
 using ::v8::internal::Debug;
 using ::v8::internal::Debugger;
+using ::v8::internal::Message;
+using ::v8::internal::MessageQueue;
 using ::v8::internal::StepAction;
 using ::v8::internal::StepIn;  // From StepAction enum
 using ::v8::internal::StepNext;  // From StepAction enum
 using ::v8::internal::StepOut;  // From StepAction enum
+using ::v8::internal::Vector;
 
 
 // Size of temp buffer for formatting small strings.
@@ -3375,7 +3378,8 @@ class MessageQueueDebuggerThread : public v8::internal::Thread {
   void Run();
 };
 
-static void MessageHandler(const uint16_t* message, int length, void *data) {
+static void MessageHandler(const uint16_t* message, int length,
+                           v8::Debug::ClientData* client_data) {
   static char print_buffer[1000];
   Utf16ToAscii(message, length, print_buffer);
   if (IsBreakEventMessage(print_buffer)) {
@@ -3390,7 +3394,6 @@ static void MessageHandler(const uint16_t* message, int length, void *data) {
   fflush(stdout);
 }
 
-
 void MessageQueueDebuggerThread::Run() {
   const int kBufferSize = 1000;
   uint16_t buffer_1[kBufferSize];
@@ -3481,6 +3484,114 @@ TEST(MessageQueues) {
   fflush(stdout);
 }
 
+
+class TestClientData : public v8::Debug::ClientData {
+ public:
+  TestClientData() {
+    constructor_call_counter++;
+  }
+  virtual ~TestClientData() {
+    destructor_call_counter++;
+  }
+
+  static void ResetCounters() {
+    constructor_call_counter = 0;
+    destructor_call_counter = 0;
+  }
+
+  static int constructor_call_counter;
+  static int destructor_call_counter;
+};
+
+int TestClientData::constructor_call_counter = 0;
+int TestClientData::destructor_call_counter = 0;
+
+
+// Tests that MessageQueue doesn't destroy client data when expands and
+// does destroy when it dies.
+TEST(MessageQueueExpandAndDestroy) {
+  TestClientData::ResetCounters();
+  { // Create a scope for the queue.
+    MessageQueue queue(1);
+    queue.Put(Message::NewCommand(Vector<uint16_t>::empty(),
+                                  new TestClientData()));
+    queue.Put(Message::NewCommand(Vector<uint16_t>::empty(),
+                                  new TestClientData()));
+    queue.Put(Message::NewHostDispatch(new TestClientData()));
+    ASSERT_EQ(0, TestClientData::destructor_call_counter);
+    queue.Get().Dispose();
+    ASSERT_EQ(1, TestClientData::destructor_call_counter);
+    queue.Put(Message::NewHostDispatch(new TestClientData()));
+    queue.Put(Message::NewHostDispatch(new TestClientData()));
+    queue.Put(Message::NewHostDispatch(new TestClientData()));
+    queue.Put(Message::NewOutput(v8::Handle<v8::String>(),
+                                 new TestClientData()));
+    queue.Put(Message::NewEmptyMessage());
+    ASSERT_EQ(1, TestClientData::destructor_call_counter);
+    queue.Get().Dispose();
+    ASSERT_EQ(2, TestClientData::destructor_call_counter);
+  }
+  // All the client data should be destroyed when the queue is destroyed.
+  ASSERT_EQ(TestClientData::destructor_call_counter,
+            TestClientData::destructor_call_counter);
+}
+
+
+static int handled_client_data_instances_count = 0;
+static void MessageHandlerCountingClientData(
+    const uint16_t* message,
+    int length,
+    v8::Debug::ClientData* client_data) {
+  if (client_data) {
+    handled_client_data_instances_count++;
+  }
+}
+
+
+// Tests that all client data passed to the debugger are sent to the handler.
+TEST(SendClientDataToHandler) {
+  // Create a V8 environment
+  v8::HandleScope scope;
+  DebugLocalContext env;
+  TestClientData::ResetCounters();
+  handled_client_data_instances_count = 0;
+  v8::Debug::SetMessageHandler(MessageHandlerCountingClientData,
+                               false /* message_handler_thread */);
+  const char* source_1 = "a = 3; b = 4; c = new Object(); c.d = 5; debugger;";
+  const int kBufferSize = 1000;
+  uint16_t buffer[kBufferSize];
+  const char* command_1 =
+      "{\"seq\":117,"
+       "\"type\":\"request\","
+       "\"command\":\"evaluate\","
+       "\"arguments\":{\"expression\":\"1+2\"}}";
+  const char* command_2 =
+    "{\"seq\":118,"
+     "\"type\":\"request\","
+     "\"command\":\"evaluate\","
+     "\"arguments\":{\"expression\":\"1+a\"}}";
+  const char* command_continue =
+    "{\"seq\":106,"
+     "\"type\":\"request\","
+     "\"command\":\"continue\"}";
+
+  v8::Debug::SendCommand(buffer, AsciiToUtf16(command_1, buffer),
+                         new TestClientData());
+  v8::Debug::SendCommand(buffer, AsciiToUtf16(command_2, buffer), NULL);
+  v8::Debug::SendCommand(buffer, AsciiToUtf16(command_2, buffer),
+                         new TestClientData());
+  v8::Debug::SendCommand(buffer, AsciiToUtf16(command_2, buffer),
+                         new TestClientData());
+  v8::Debug::SendCommand(buffer, AsciiToUtf16(command_continue, buffer));
+  CompileRun(source_1);
+  ASSERT_EQ(3, TestClientData::constructor_call_counter);
+  ASSERT_EQ(TestClientData::constructor_call_counter,
+            handled_client_data_instances_count);
+  ASSERT_EQ(TestClientData::constructor_call_counter,
+            TestClientData::destructor_call_counter);
+}
+
+
 /* Test ThreadedDebugging */
 /* This test interrupts a running infinite loop that is
  * occupying the v8 thread by a break command from the
@@ -3508,7 +3619,7 @@ static v8::Handle<v8::Value> ThreadedAtBarrier1(const v8::Arguments& args) {
 
 
 static void ThreadedMessageHandler(const uint16_t* message, int length,
-                                   void *data) {
+                                   v8::Debug::ClientData* client_data) {
   static char print_buffer[1000];
   Utf16ToAscii(message, length, print_buffer);
   if (IsBreakEventMessage(print_buffer)) {
@@ -3606,7 +3717,7 @@ Barriers* breakpoints_barriers;
 
 static void BreakpointsMessageHandler(const uint16_t* message,
                                       int length,
-                                      void *data) {
+                                      v8::Debug::ClientData* client_data) {
   static char print_buffer[1000];
   Utf16ToAscii(message, length, print_buffer);
   printf("%s\n", print_buffer);
@@ -3752,7 +3863,8 @@ TEST(SetDebugEventListenerOnUninitializedVM) {
 
 
 static void DummyMessageHandler(const uint16_t* message,
-                                int length, void *data) {
+                                int length,
+                                v8::Debug::ClientData* client_data) {
 }
 
 
@@ -3978,7 +4090,8 @@ 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) {
+                                   int length,
+                                   v8::Debug::ClientData* client_data) {
   message_handler_hit_count++;
 
   const int kBufferSize = 1000;
@@ -4026,9 +4139,10 @@ TEST(DebuggerClearMessageHandler) {
 
 
 // Debugger message handler which clears the message handler while active.
-static void MessageHandlerClearingMessageHandler(const uint16_t* message,
-                                                 int length,
-                                                 void* data) {
+static void MessageHandlerClearingMessageHandler(
+    const uint16_t* message,
+    int length,
+    v8::Debug::ClientData* client_data) {
   message_handler_hit_count++;
 
   // Clear debug message handler.
@@ -4059,7 +4173,7 @@ TEST(DebuggerClearMessageHandlerWhileActive) {
 
 
 int host_dispatch_hit_count = 0;
-static void HostDispatchHandlerHitCount(void* dispatch, void *data) {
+static void HostDispatchHandlerHitCount(v8::Debug::ClientData* dispatch) {
   host_dispatch_hit_count++;
 }
 
@@ -4085,8 +4199,7 @@ TEST(DebuggerHostDispatch) {
 
   // Setup message and host dispatch handlers.
   v8::Debug::SetMessageHandler(DummyMessageHandler);
-  v8::Debug::SetHostDispatchHandler(HostDispatchHandlerHitCount,
-                                    NULL);
+  v8::Debug::SetHostDispatchHandler(HostDispatchHandlerHitCount);
 
   // Send a host dispatch by itself.
   v8::Debug::SendHostDispatch(NULL);