Avoid interleaving JDWP requests and events.

To avoid bad synchronization between the debugger and the debuggee, we should
not send any events while processing a request (before sending its reply).

This particularly prevents from sending the VM_DEATH event (and consequently
closing the JDWP connection) after receiving the VM_Resume command but before
sending its reply.

Bug: 12581527
Change-Id: I197cc54e980a983faae4b545f25dfe7fe812e841

4 files changed, 59 insertions, 0 deletions

diff --git a/runtime/jdwp/jdwp.h b/runtime/jdwp/jdwp.h
index 334dca4..e45cb6e 100644
--- a/runtime/jdwp/jdwp.h
+++ b/runtime/jdwp/jdwp.h
@@ -295,6 +295,10 @@ struct JdwpState {
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   void SendBufferedRequest(uint32_t type, const std::vector<iovec>& iov);
 
+  void StartProcessingRequest() LOCKS_EXCLUDED(process_request_lock_);
+  void EndProcessingRequest() LOCKS_EXCLUDED(process_request_lock_);
+  void WaitForProcessingRequest() LOCKS_EXCLUDED(process_request_lock_);
+
  public:  // TODO: fix privacy
   const JdwpOptions* options_;
 
@@ -340,6 +344,12 @@ struct JdwpState {
   ConditionVariable event_thread_cond_ GUARDED_BY(event_thread_lock_);
   ObjectId event_thread_id_;
 
+  // Used to synchronize request processing and event sending (to avoid sending an event before
+  // sending the reply of a command being processed).
+  Mutex process_request_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
+  ConditionVariable process_request_cond_ GUARDED_BY(process_request_lock_);
+  bool processing_request_ GUARDED_BY(process_request_lock_);
+
   bool ddm_is_active_;
 
   bool should_exit_;
diff --git a/runtime/jdwp/jdwp_event.cc b/runtime/jdwp/jdwp_event.cc
index e372c26..ede81d2 100644
--- a/runtime/jdwp/jdwp_event.cc
+++ b/runtime/jdwp/jdwp_event.cc
@@ -697,6 +697,11 @@ void JdwpState::EventFinish(ExpandBuf* pReq) {
   Set1(buf+9, kJdwpEventCommandSet);
   Set1(buf+10, kJdwpCompositeCommand);
 
+  // Prevents from interleaving commands and events. Otherwise we could end up in sending an event
+  // before sending the reply of the command being processed and would lead to bad synchronization
+  // between the debugger and the debuggee.
+  WaitForProcessingRequest();
+
   SendRequest(pReq);
 
   expandBufFree(pReq);
diff --git a/runtime/jdwp/jdwp_handler.cc b/runtime/jdwp/jdwp_handler.cc
index a514e69..0ff78d0 100644
--- a/runtime/jdwp/jdwp_handler.cc
+++ b/runtime/jdwp/jdwp_handler.cc
@@ -1747,6 +1747,44 @@ void JdwpState::ProcessRequest(Request& request, ExpandBuf* pReply) {
   self->TransitionFromRunnableToSuspended(old_state);
 }
 
+/*
+ * Indicates a request is about to be processed. If a thread wants to send an event in the meantime,
+ * it will need to wait until we processed this request (see EndProcessingRequest).
+ */
+void JdwpState::StartProcessingRequest() {
+  Thread* self = Thread::Current();
+  CHECK_EQ(self, GetDebugThread()) << "Requests are only processed by debug thread";
+  MutexLock mu(self, process_request_lock_);
+  CHECK_EQ(processing_request_, false);
+  processing_request_ = true;
+}
+
+/*
+ * Indicates a request has been processed (and we sent its reply). All threads waiting for us (see
+ * WaitForProcessingRequest) are waken up so they can send events again.
+ */
+void JdwpState::EndProcessingRequest() {
+  Thread* self = Thread::Current();
+  CHECK_EQ(self, GetDebugThread()) << "Requests are only processed by debug thread";
+  MutexLock mu(self, process_request_lock_);
+  CHECK_EQ(processing_request_, true);
+  processing_request_ = false;
+  process_request_cond_.Broadcast(self);
+}
+
+/*
+ * Waits for any request being processed so we do not send an event in the meantime.
+ */
+void JdwpState::WaitForProcessingRequest() {
+  Thread* self = Thread::Current();
+  CHECK_NE(self, GetDebugThread()) << "Events should not be posted by debug thread";
+  MutexLock mu(self, process_request_lock_);
+  while (processing_request_) {
+    process_request_cond_.Wait(self);
+  }
+  CHECK_EQ(processing_request_, false);
+}
+
 }  // namespace JDWP
 
 }  // namespace art
diff --git a/runtime/jdwp/jdwp_main.cc b/runtime/jdwp/jdwp_main.cc
index 928f53d..ba49c45 100644
--- a/runtime/jdwp/jdwp_main.cc
+++ b/runtime/jdwp/jdwp_main.cc
@@ -218,6 +218,9 @@ JdwpState::JdwpState(const JdwpOptions* options)
       event_thread_lock_("JDWP event thread lock"),
       event_thread_cond_("JDWP event thread condition variable", event_thread_lock_),
       event_thread_id_(0),
+      process_request_lock_("JDWP process request lock"),
+      process_request_cond_("JDWP process request condition variable", process_request_lock_),
+      processing_request_(false),
       ddm_is_active_(false),
       should_exit_(false),
       exit_status_(0) {
@@ -383,9 +386,12 @@ bool JdwpState::HandlePacket() {
   JdwpNetStateBase* netStateBase = reinterpret_cast<JdwpNetStateBase*>(netState);
   JDWP::Request request(netStateBase->input_buffer_, netStateBase->input_count_);
 
+  StartProcessingRequest();
   ExpandBuf* pReply = expandBufAlloc();
   ProcessRequest(request, pReply);
   ssize_t cc = netStateBase->WritePacket(pReply);
+  EndProcessingRequest();
+
   if (cc != (ssize_t) expandBufGetLength(pReply)) {
     PLOG(ERROR) << "Failed sending reply to debugger";
     expandBufFree(pReply);