JDWP: update thread synchronization

This CL ensures only one thread can do JDWP stuff at a time: either
processing a command coming from the debugger (JDWP thread) or
sending an event (breakpoint, class prepare, etc) to the debugger
before suspending.

The JDWP thread now uses AcquireJdwpTokenForCommand and
ReleaseJdwpTokenForCommand, respectively acquiring and releasing the
token used for synchronization. On the other hand, the event threads
now use AcquireJdwpTokenForEvent and ReleaseJdwpTokenForEvent.

During an invoke, the target thread needs to take the JDWP token to
execute the method while it's already held by the JDWP handler thread
waiting for the invocation to complete. To avoid both threads from
waiting for each other (deadlock), the JDWP thread releases the token
and acquires it again after the invocation is complete, so the target
thread can run safely and prevents other threads from sending events.

Bug: 19120467
Change-Id: Ie3208fb940a60573769d494128cf22f0fa30fa61

5 files changed, 116 insertions, 138 deletions

diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 678b29a..8de0478 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -604,7 +604,7 @@ Thread* Dbg::GetDebugThread() {
 }
 
 void Dbg::ClearWaitForEventThread() {
-  gJdwpState->ClearWaitForEventThread();
+  gJdwpState->ReleaseJdwpTokenForEvent();
 }
 
 void Dbg::Connected() {
@@ -3637,6 +3637,11 @@ JDWP::JdwpError Dbg::InvokeMethod(JDWP::ObjectId thread_id, JDWP::ObjectId objec
         thread_list->Resume(targetThread, true);
       }
 
+      // The target thread is resumed but needs the JDWP token we're holding.
+      // We release it now and will acquire it again when the invocation is
+      // complete and the target thread suspends itself.
+      gJdwpState->ReleaseJdwpTokenForCommand();
+
       // Wait for the request to finish executing.
       while (req->invoke_needed) {
         req->cond.Wait(self);
@@ -3646,6 +3651,10 @@ JDWP::JdwpError Dbg::InvokeMethod(JDWP::ObjectId thread_id, JDWP::ObjectId objec
 
     /* wait for thread to re-suspend itself */
     SuspendThread(thread_id, false /* request_suspension */);
+
+    // Now the thread is suspended again, we can re-acquire the JDWP token.
+    gJdwpState->AcquireJdwpTokenForCommand();
+
     self->TransitionFromSuspendedToRunnable();
   }
 
@@ -3653,7 +3662,7 @@ JDWP::JdwpError Dbg::InvokeMethod(JDWP::ObjectId thread_id, JDWP::ObjectId objec
    * Suspend the threads.  We waited for the target thread to suspend
    * itself, so all we need to do is suspend the others.
    *
-   * The suspendAllThreads() call will double-suspend the event thread,
+   * The SuspendAllForDebugger() call will double-suspend the event thread,
    * so we want to resume the target thread once to keep the books straight.
    */
   if ((options & JDWP::INVOKE_SINGLE_THREADED) == 0) {
diff --git a/runtime/jdwp/jdwp.h b/runtime/jdwp/jdwp.h
index 9f37998..e16221c 100644
--- a/runtime/jdwp/jdwp.h
+++ b/runtime/jdwp/jdwp.h
@@ -149,29 +149,19 @@ struct JdwpState {
 
   void ExitAfterReplying(int exit_status);
 
-  /*
-   * When we hit a debugger event that requires suspension, it's important
-   * that we wait for the thread to suspend itself before processing any
-   * additional requests.  (Otherwise, if the debugger immediately sends a
-   * "resume thread" command, the resume might arrive before the thread has
-   * suspended itself.)
-   *
-   * The thread should call the "set" function before sending the event to
-   * the debugger.  The main JDWP handler loop calls "get" before processing
-   * an event, and will wait for thread suspension if it's set.  Once the
-   * thread has suspended itself, the JDWP handler calls "clear" and
-   * continues processing the current event.  This works in the suspend-all
-   * case because the event thread doesn't suspend itself until everything
-   * else has suspended.
-   *
-   * It's possible that multiple threads could encounter thread-suspending
-   * events at the same time, so we grab a mutex in the "set" call, and
-   * release it in the "clear" call.
-   */
-  // ObjectId GetWaitForEventThread();
-  void SetWaitForEventThread(ObjectId threadId)
-      LOCKS_EXCLUDED(event_thread_lock_, process_request_lock_);
-  void ClearWaitForEventThread() LOCKS_EXCLUDED(event_thread_lock_);
+  // Acquires/releases the JDWP synchronization token for the debugger
+  // thread (command handler) so no event thread posts an event while
+  // it processes a command. This must be called only from the debugger
+  // thread.
+  void AcquireJdwpTokenForCommand() LOCKS_EXCLUDED(jdwp_token_lock_);
+  void ReleaseJdwpTokenForCommand() LOCKS_EXCLUDED(jdwp_token_lock_);
+
+  // Acquires/releases the JDWP synchronization token for the event thread
+  // so no other thread (debugger thread or event thread) interleaves with
+  // it when posting an event. This must NOT be called from the debugger
+  // thread, only event thread.
+  void AcquireJdwpTokenForEvent(ObjectId threadId) LOCKS_EXCLUDED(jdwp_token_lock_);
+  void ReleaseJdwpTokenForEvent() LOCKS_EXCLUDED(jdwp_token_lock_);
 
   /*
    * These notify the debug code that something interesting has happened.  This
@@ -330,9 +320,37 @@ 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_);
+  /*
+   * When we hit a debugger event that requires suspension, it's important
+   * that we wait for the thread to suspend itself before processing any
+   * additional requests. Otherwise, if the debugger immediately sends a
+   * "resume thread" command, the resume might arrive before the thread has
+   * suspended itself.
+   *
+   * It's also important no event thread suspends while we process a command
+   * from the debugger. Otherwise we could post an event ("thread death")
+   * before sending the reply of the command being processed ("resume") and
+   * cause bad synchronization with the debugger.
+   *
+   * The thread wanting "exclusive" access to the JDWP world must call the
+   * SetWaitForJdwpToken method before processing a command from the
+   * debugger or sending an event to the debugger.
+   * Once the command is processed or the event thread has posted its event,
+   * it must call the ClearWaitForJdwpToken method to allow another thread
+   * to do JDWP stuff.
+   *
+   * Therefore the main JDWP handler loop will wait for the event thread
+   * suspension before processing the next command. Once the event thread
+   * has suspended itself and cleared the token, the JDWP handler continues
+   * processing commands. This works in the suspend-all case because the
+   * event thread doesn't suspend itself until everything else has suspended.
+   *
+   * It's possible that multiple threads could encounter thread-suspending
+   * events at the same time, so we grab a mutex in the SetWaitForJdwpToken
+   * call, and release it in the ClearWaitForJdwpToken call.
+   */
+  void SetWaitForJdwpToken(ObjectId threadId) LOCKS_EXCLUDED(jdwp_token_lock_);
+  void ClearWaitForJdwpToken() LOCKS_EXCLUDED(jdwp_token_lock_);
 
  public:  // TODO: fix privacy
   const JdwpOptions* options_;
@@ -368,24 +386,21 @@ struct JdwpState {
 
   // Linked list of events requested by the debugger (breakpoints, class prep, etc).
   Mutex event_list_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER ACQUIRED_BEFORE(Locks::breakpoint_lock_);
-
   JdwpEvent* event_list_ GUARDED_BY(event_list_lock_);
   size_t event_list_size_ GUARDED_BY(event_list_lock_);  // Number of elements in event_list_.
 
-  // Used to synchronize suspension of the event thread (to avoid receiving "resume"
-  // events before the thread has finished suspending itself).
-  Mutex event_thread_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
-  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_ ACQUIRED_AFTER(event_thread_lock_);
-  ConditionVariable process_request_cond_ GUARDED_BY(process_request_lock_);
-  bool processing_request_ GUARDED_BY(process_request_lock_);
+  // Used to synchronize JDWP command handler thread and event threads so only one
+  // thread does JDWP stuff at a time. This prevent from interleaving command handling
+  // and event notification. Otherwise we could receive a "resume" command for an
+  // event thread that is not suspended yet, or post a "thread death" or event "VM death"
+  // event before sending the reply of the "resume" command that caused it.
+  Mutex jdwp_token_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
+  ConditionVariable jdwp_token_cond_ GUARDED_BY(jdwp_token_lock_);
+  ObjectId jdwp_token_owner_thread_id_;
 
   bool ddm_is_active_;
 
+  // Used for VirtualMachine.Exit command handling.
   bool should_exit_;
   int exit_status_;
 };
diff --git a/runtime/jdwp/jdwp_event.cc b/runtime/jdwp/jdwp_event.cc
index a8eaa26..b71f6cd 100644
--- a/runtime/jdwp/jdwp_event.cc
+++ b/runtime/jdwp/jdwp_event.cc
@@ -612,7 +612,7 @@ void JdwpState::SuspendByPolicy(JdwpSuspendPolicy suspend_policy, JDWP::ObjectId
     }
 
     /* grab this before posting/suspending again */
-    SetWaitForEventThread(thread_self_id);
+    AcquireJdwpTokenForEvent(thread_self_id);
 
     /* leave pReq->invoke_needed_ raised so we can check reentrancy */
     Dbg::ExecuteMethod(pReq);
@@ -630,7 +630,7 @@ void JdwpState::SendRequestAndPossiblySuspend(ExpandBuf* pReq, JdwpSuspendPolicy
   JDWP::ObjectId thread_self_id = Dbg::GetThreadSelfId();
   self->TransitionFromRunnableToSuspended(kWaitingForDebuggerSend);
   if (suspend_policy != SP_NONE) {
-    SetWaitForEventThread(threadId);
+    AcquireJdwpTokenForEvent(threadId);
   }
   EventFinish(pReq);
   SuspendByPolicy(suspend_policy, thread_self_id);
@@ -649,63 +649,82 @@ bool JdwpState::InvokeInProgress() {
   return pReq->invoke_needed;
 }
 
+void JdwpState::AcquireJdwpTokenForCommand() {
+  CHECK_EQ(Thread::Current(), GetDebugThread()) << "Expected debugger thread";
+  SetWaitForJdwpToken(debug_thread_id_);
+}
+
+void JdwpState::ReleaseJdwpTokenForCommand() {
+  CHECK_EQ(Thread::Current(), GetDebugThread()) << "Expected debugger thread";
+  ClearWaitForJdwpToken();
+}
+
+void JdwpState::AcquireJdwpTokenForEvent(ObjectId threadId) {
+  CHECK_NE(Thread::Current(), GetDebugThread()) << "Expected event thread";
+  CHECK_NE(debug_thread_id_, threadId) << "Not expected debug thread";
+  SetWaitForJdwpToken(threadId);
+}
+
+void JdwpState::ReleaseJdwpTokenForEvent() {
+  CHECK_NE(Thread::Current(), GetDebugThread()) << "Expected event thread";
+  ClearWaitForJdwpToken();
+}
+
 /*
  * We need the JDWP thread to hold off on doing stuff while we post an
  * event and then suspend ourselves.
  *
- * Call this with a threadId of zero if you just want to wait for the
- * current thread operation to complete.
- *
  * This could go to sleep waiting for another thread, so it's important
  * that the thread be marked as VMWAIT before calling here.
  */
-void JdwpState::SetWaitForEventThread(ObjectId threadId) {
+void JdwpState::SetWaitForJdwpToken(ObjectId threadId) {
   bool waited = false;
+  Thread* const self = Thread::Current();
+  CHECK_NE(threadId, 0u);
+  CHECK_NE(self->GetState(), kRunnable);
+  Locks::mutator_lock_->AssertNotHeld(self);
 
   /* this is held for very brief periods; contention is unlikely */
-  Thread* self = Thread::Current();
-  MutexLock mu(self, event_thread_lock_);
+  MutexLock mu(self, jdwp_token_lock_);
+
+  CHECK_NE(jdwp_token_owner_thread_id_, threadId) << "Thread is already holding event thread lock";
 
   /*
    * If another thread is already doing stuff, wait for it.  This can
    * go to sleep indefinitely.
    */
-  while (event_thread_id_ != 0) {
+  while (jdwp_token_owner_thread_id_ != 0) {
     VLOG(jdwp) << StringPrintf("event in progress (%#" PRIx64 "), %#" PRIx64 " sleeping",
-                               event_thread_id_, threadId);
+                               jdwp_token_owner_thread_id_, threadId);
     waited = true;
-    event_thread_cond_.Wait(self);
+    jdwp_token_cond_.Wait(self);
   }
 
-  if (waited || threadId != 0) {
+  if (waited || threadId != debug_thread_id_) {
     VLOG(jdwp) << StringPrintf("event token grabbed (%#" PRIx64 ")", threadId);
   }
-  if (threadId != 0) {
-    event_thread_id_ = threadId;
-  }
+  jdwp_token_owner_thread_id_ = threadId;
 }
 
 /*
  * Clear the threadId and signal anybody waiting.
  */
-void JdwpState::ClearWaitForEventThread() {
+void JdwpState::ClearWaitForJdwpToken() {
   /*
    * Grab the mutex.  Don't try to go in/out of VMWAIT mode, as this
-   * function is called by dvmSuspendSelf(), and the transition back
+   * function is called by Dbg::SuspendSelf(), and the transition back
    * to RUNNING would confuse it.
    */
-  Thread* self = Thread::Current();
-  MutexLock mu(self, event_thread_lock_);
-
-  CHECK_NE(event_thread_id_, 0U);
-  VLOG(jdwp) << StringPrintf("cleared event token (%#" PRIx64 ")", event_thread_id_);
+  Thread* const self = Thread::Current();
+  MutexLock mu(self, jdwp_token_lock_);
 
-  event_thread_id_ = 0;
+  CHECK_NE(jdwp_token_owner_thread_id_, 0U);
+  VLOG(jdwp) << StringPrintf("cleared event token (%#" PRIx64 ")", jdwp_token_owner_thread_id_);
 
-  event_thread_cond_.Signal(self);
+  jdwp_token_owner_thread_id_ = 0;
+  jdwp_token_cond_.Signal(self);
 }
 
-
 /*
  * Prep an event.  Allocates storage for the message and leaves space for
  * the header.
@@ -730,11 +749,6 @@ 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 a1d2a6c..0ce4de7 100644
--- a/runtime/jdwp/jdwp_handler.cc
+++ b/runtime/jdwp/jdwp_handler.cc
@@ -1633,27 +1633,15 @@ size_t JdwpState::ProcessRequest(Request* request, ExpandBuf* pReply) {
 
   /*
    * If a debugger event has fired in another thread, wait until the
-   * initiating thread has suspended itself before processing messages
+   * initiating thread has suspended itself before processing commands
    * from the debugger.  Otherwise we (the JDWP thread) could be told to
    * resume the thread before it has suspended.
    *
-   * We call with an argument of zero to wait for the current event
-   * thread to finish, and then clear the block.  Depending on the thread
-   * suspend policy, this may allow events in other threads to fire,
-   * but those events have no bearing on what the debugger has sent us
-   * in the current request->
-   *
    * Note that we MUST clear the event token before waking the event
    * thread up, or risk waiting for the thread to suspend after we've
    * told it to resume.
    */
-  SetWaitForEventThread(0);
-
-  /*
-   * We do not want events to be sent while we process a request-> Indicate the JDWP thread starts
-   * to process a request so other threads wait for it to finish before sending an event.
-   */
-  StartProcessingRequest();
+  AcquireJdwpTokenForCommand();
 
   /*
    * Tell the VM that we're running and shouldn't be interrupted by GC.
@@ -1719,50 +1707,6 @@ size_t JdwpState::ProcessRequest(Request* request, ExpandBuf* pReply) {
   return replyLength;
 }
 
-/*
- * 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_);
-  bool waited = false;
-  while (processing_request_) {
-    VLOG(jdwp) << StringPrintf("wait for processing request");
-    waited = true;
-    process_request_cond_.Wait(self);
-  }
-  if (waited) {
-    VLOG(jdwp) << StringPrintf("finished waiting for processing request");
-  }
-  CHECK_EQ(processing_request_, false);
-}
-
 }  // namespace JDWP
 
 }  // namespace art
diff --git a/runtime/jdwp/jdwp_main.cc b/runtime/jdwp/jdwp_main.cc
index b04aa6e..b6fedd9 100644
--- a/runtime/jdwp/jdwp_main.cc
+++ b/runtime/jdwp/jdwp_main.cc
@@ -220,12 +220,9 @@ JdwpState::JdwpState(const JdwpOptions* options)
       event_list_lock_("JDWP event list lock", kJdwpEventListLock),
       event_list_(nullptr),
       event_list_size_(0),
-      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),
+      jdwp_token_lock_("JDWP token lock"),
+      jdwp_token_cond_("JDWP token condition variable", jdwp_token_lock_),
+      jdwp_token_owner_thread_id_(0),
       ddm_is_active_(false),
       should_exit_(false),
       exit_status_(0) {
@@ -331,7 +328,7 @@ void JdwpState::ResetState() {
    * Should not have one of these in progress.  If the debugger went away
    * mid-request, though, we could see this.
    */
-  if (event_thread_id_ != 0) {
+  if (jdwp_token_owner_thread_id_ != 0) {
     LOG(WARNING) << "Resetting state while event in progress";
     DCHECK(false);
   }
@@ -382,10 +379,9 @@ bool JdwpState::HandlePacket() {
   ssize_t cc = netStateBase->WritePacket(pReply, replyLength);
 
   /*
-   * We processed this request and sent its reply. Notify other threads waiting for us they can now
-   * send events.
+   * We processed this request and sent its reply so we can release the JDWP token.
    */
-  EndProcessingRequest();
+  ReleaseJdwpTokenForCommand();
 
   if (cc != static_cast<ssize_t>(replyLength)) {
     PLOG(ERROR) << "Failed sending reply to debugger";