JDWP: asynchronous invoke command handling

The JDWP thread used to wait for the result of a method invocation
running in an event thread. But doing that prevents the JDWP thread
from processing incoming commands from the debugger if the event
thread gets suspended by a debug event occurring in another thread.
In Android Studio (or another IDE), this leads to the debugger being
blocked (with the famous message "Waiting until last debugger command
completes" of Android Studio / IntelliJ) because it is actually
waiting for the reply of its latest command while the JDWP thread
cannot process it.

This CL changes the way invoke commands (ClassType.InvokeCommand,
ClassType.NewInstance and ObjectReference.InvokeCommand) are handled
in the ART runtime.
The JDWP thread no longer waits for the event thread to complete the
method invocation. It now simply waits for the next JDWP command to
process. This means it does not send any reply for invoke commands,
except if the information given by the debugger is wrong. In this
case, it still sends a reply with the appropriate error code.
The event thread is now responsible for sending the reply (containing
the result and the exception object of the invoked method) before
going back to the suspended state.

In other words, we add special handling for invoke commands so they
are handled asynchronously while other commands remained handled
synchronously. In the future, we may want to handle all commands
asynchronously (using a queue of reply/event for instance) to remove
the special handling code this CL is adding.

Now the JDWP thread can process commands while a thread is invoking
a method, it is possible for the debugger to detach (by sending a
VirtualMachine.Dispose command) before the invocation completes. In
that situation, we must not suspend threads again (including the
event thread that executed the method) because they would all remain
suspended forever.

Also minor cleanup of the use of JDWP constants and update comments.

Bug: 21515842
Bug: 18899981

(cherry picked from commit cbc5064ff05179b97b416f00ca579c55e38cd7d9)

Change-Id: I8d31006043468913ee8453212e6d16e11fcfe4ea

11 files changed, 351 insertions, 250 deletions

diff --git a/runtime/base/mutex.h b/runtime/base/mutex.h
index f2be85e..0ab148e 100644
--- a/runtime/base/mutex.h
+++ b/runtime/base/mutex.h
@@ -94,7 +94,6 @@ enum LockLevel {
   kMonitorListLock,
   kJniLoadLibraryLock,
   kThreadListLock,
-  kBreakpointInvokeLock,
   kAllocTrackerLock,
   kDeoptimizationLock,
   kProfilerLock,
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 24615e2..3c6f98a 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -32,6 +32,7 @@
 #include "gc/space/large_object_space.h"
 #include "gc/space/space-inl.h"
 #include "handle_scope.h"
+#include "jdwp/jdwp_priv.h"
 #include "jdwp/object_registry.h"
 #include "mirror/class.h"
 #include "mirror/class-inl.h"
@@ -3763,17 +3764,16 @@ static char JdwpTagToShortyChar(JDWP::JdwpTag tag) {
   }
 }
 
-JDWP::JdwpError Dbg::InvokeMethod(JDWP::ObjectId thread_id, JDWP::ObjectId object_id,
-                                  JDWP::RefTypeId class_id, JDWP::MethodId method_id,
-                                  uint32_t arg_count, uint64_t* arg_values,
-                                  JDWP::JdwpTag* arg_types, uint32_t options,
-                                  JDWP::JdwpTag* pResultTag, uint64_t* pResultValue,
-                                  JDWP::ObjectId* pExceptionId) {
-  ThreadList* thread_list = Runtime::Current()->GetThreadList();
+JDWP::JdwpError Dbg::PrepareInvokeMethod(uint32_t request_id, JDWP::ObjectId thread_id,
+                                         JDWP::ObjectId object_id, JDWP::RefTypeId class_id,
+                                         JDWP::MethodId method_id, uint32_t arg_count,
+                                         uint64_t arg_values[], JDWP::JdwpTag* arg_types,
+                                         uint32_t options) {
+  Thread* const self = Thread::Current();
+  CHECK_EQ(self, GetDebugThread()) << "This must be called by the JDWP thread";
 
+  ThreadList* thread_list = Runtime::Current()->GetThreadList();
   Thread* targetThread = nullptr;
-  std::unique_ptr<DebugInvokeReq> req;
-  Thread* self = Thread::Current();
   {
     ScopedObjectAccessUnchecked soa(self);
     JDWP::JdwpError error;
@@ -3883,99 +3883,82 @@ JDWP::JdwpError Dbg::InvokeMethod(JDWP::ObjectId thread_id, JDWP::ObjectId objec
     }
 
     // Allocates a DebugInvokeReq.
-    req.reset(new (std::nothrow) DebugInvokeReq(receiver, c, m, options, arg_values, arg_count));
-    if (req.get() == nullptr) {
+    DebugInvokeReq* req = new (std::nothrow) DebugInvokeReq(request_id, thread_id, receiver, c, m,
+                                                            options, arg_values, arg_count);
+    if (req == nullptr) {
       LOG(ERROR) << "Failed to allocate DebugInvokeReq";
       return JDWP::ERR_OUT_OF_MEMORY;
     }
 
-    // Attach the DebugInvokeReq to the target thread so it executes the method when
-    // it is resumed. Once the invocation completes, it will detach it and signal us
-    // before suspending itself.
-    targetThread->SetDebugInvokeReq(req.get());
+    // Attaches the DebugInvokeReq to the target thread so it executes the method when
+    // it is resumed. Once the invocation completes, the target thread will delete it before
+    // suspending itself (see ThreadList::SuspendSelfForDebugger).
+    targetThread->SetDebugInvokeReq(req);
   }
 
   // The fact that we've released the thread list lock is a bit risky --- if the thread goes
-  // away we're sitting high and dry -- but we must release this before the ResumeAllThreads
-  // call, and it's unwise to hold it during WaitForSuspend.
-
-  {
-    /*
-     * We change our (JDWP thread) status, which should be THREAD_RUNNING,
-     * so we can suspend for a GC if the invoke request causes us to
-     * run out of memory.  It's also a good idea to change it before locking
-     * the invokeReq mutex, although that should never be held for long.
-     */
-    self->TransitionFromRunnableToSuspended(kWaitingForDebuggerSend);
-
-    VLOG(jdwp) << "    Transferring control to event thread";
-    {
-      MutexLock mu(self, req->lock);
-
-      if ((options & JDWP::INVOKE_SINGLE_THREADED) == 0) {
-        VLOG(jdwp) << "      Resuming all threads";
-        thread_list->UndoDebuggerSuspensions();
-      } else {
-        VLOG(jdwp) << "      Resuming event thread only";
-        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 (targetThread->GetInvokeReq() != nullptr) {
-        req->cond.Wait(self);
-      }
-    }
-    VLOG(jdwp) << "    Control has returned from event thread";
-
-    /* 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();
+  // away we're sitting high and dry -- but we must release this before the UndoDebuggerSuspensions
+  // call.
 
-    self->TransitionFromSuspendedToRunnable();
-  }
-
-  /*
-   * Suspend the threads.  We waited for the target thread to suspend
-   * itself, so all we need to do is suspend the others.
-   *
-   * 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) {
-    self->TransitionFromRunnableToSuspended(kWaitingForDebuggerSuspension);
-    VLOG(jdwp) << "      Suspending all threads";
-    thread_list->SuspendAllForDebugger();
-    self->TransitionFromSuspendedToRunnable();
-    VLOG(jdwp) << "      Resuming event thread to balance the count";
+    VLOG(jdwp) << "      Resuming all threads";
+    thread_list->UndoDebuggerSuspensions();
+  } else {
+    VLOG(jdwp) << "      Resuming event thread only";
     thread_list->Resume(targetThread, true);
   }
 
-  // Copy the result.
-  *pResultTag = req->result_tag;
-  *pResultValue = req->result_value;
-  *pExceptionId = req->exception;
-  return req->error;
+  return JDWP::ERR_NONE;
 }
 
 void Dbg::ExecuteMethod(DebugInvokeReq* pReq) {
-  ScopedObjectAccess soa(Thread::Current());
+  Thread* const self = Thread::Current();
+  CHECK_NE(self, GetDebugThread()) << "This must be called by the event thread";
+
+  ScopedObjectAccess soa(self);
 
   // We can be called while an exception is pending. We need
   // to preserve that across the method invocation.
-  StackHandleScope<3> hs(soa.Self());
-  auto old_exception = hs.NewHandle<mirror::Throwable>(soa.Self()->GetException());
+  StackHandleScope<1> hs(soa.Self());
+  Handle<mirror::Throwable> old_exception = hs.NewHandle(soa.Self()->GetException());
   soa.Self()->ClearException();
 
+  // Execute the method then sends reply to the debugger.
+  ExecuteMethodWithoutPendingException(soa, pReq);
+
+  // If an exception was pending before the invoke, restore it now.
+  if (old_exception.Get() != nullptr) {
+    soa.Self()->SetException(old_exception.Get());
+  }
+}
+
+// Helper function: write a variable-width value into the output input buffer.
+static void WriteValue(JDWP::ExpandBuf* pReply, int width, uint64_t value) {
+  switch (width) {
+    case 1:
+      expandBufAdd1(pReply, value);
+      break;
+    case 2:
+      expandBufAdd2BE(pReply, value);
+      break;
+    case 4:
+      expandBufAdd4BE(pReply, value);
+      break;
+    case 8:
+      expandBufAdd8BE(pReply, value);
+      break;
+    default:
+      LOG(FATAL) << width;
+      UNREACHABLE();
+  }
+}
+
+void Dbg::ExecuteMethodWithoutPendingException(ScopedObjectAccess& soa, DebugInvokeReq* pReq) {
+  soa.Self()->AssertNoPendingException();
+
   // Translate the method through the vtable, unless the debugger wants to suppress it.
-  auto* m = pReq->method;
-  auto image_pointer_size = Runtime::Current()->GetClassLinker()->GetImagePointerSize();
+  ArtMethod* m = pReq->method;
+  size_t image_pointer_size = Runtime::Current()->GetClassLinker()->GetImagePointerSize();
   if ((pReq->options & JDWP::INVOKE_NONVIRTUAL) == 0 && pReq->receiver.Read() != nullptr) {
     ArtMethod* actual_method =
         pReq->klass.Read()->FindVirtualMethodForVirtualOrInterface(m, image_pointer_size);
@@ -3992,39 +3975,133 @@ void Dbg::ExecuteMethod(DebugInvokeReq* pReq) {
 
   CHECK_EQ(sizeof(jvalue), sizeof(uint64_t));
 
+  // Invoke the method.
   ScopedLocalRef<jobject> ref(soa.Env(), soa.AddLocalReference<jobject>(pReq->receiver.Read()));
   JValue result = InvokeWithJValues(soa, ref.get(), soa.EncodeMethod(m),
-                                    reinterpret_cast<jvalue*>(pReq->arg_values));
+                                    reinterpret_cast<jvalue*>(pReq->arg_values.get()));
 
-  pReq->result_tag = BasicTagFromDescriptor(m->GetShorty());
-  const bool is_object_result = (pReq->result_tag == JDWP::JT_OBJECT);
+  // Prepare JDWP ids for the reply.
+  JDWP::JdwpTag result_tag = BasicTagFromDescriptor(m->GetShorty());
+  const bool is_object_result = (result_tag == JDWP::JT_OBJECT);
+  StackHandleScope<2> hs(soa.Self());
   Handle<mirror::Object> object_result = hs.NewHandle(is_object_result ? result.GetL() : nullptr);
   Handle<mirror::Throwable> exception = hs.NewHandle(soa.Self()->GetException());
   soa.Self()->ClearException();
-  pReq->exception = gRegistry->Add(exception);
-  if (pReq->exception != 0) {
+
+  if (!IsDebuggerActive()) {
+    // The debugger detached: we must not re-suspend threads. We also don't need to fill the reply
+    // because it won't be sent either.
+    return;
+  }
+
+  JDWP::ObjectId exceptionObjectId = gRegistry->Add(exception);
+  uint64_t result_value = 0;
+  if (exceptionObjectId != 0) {
     VLOG(jdwp) << "  JDWP invocation returning with exception=" << exception.Get()
                << " " << exception->Dump();
-    pReq->result_value = 0;
+    result_value = 0;
   } else if (is_object_result) {
-    /* if no exception thrown, examine object result more closely */
+    /* if no exception was thrown, examine object result more closely */
     JDWP::JdwpTag new_tag = TagFromObject(soa, object_result.Get());
-    if (new_tag != pReq->result_tag) {
-      VLOG(jdwp) << "  JDWP promoted result from " << pReq->result_tag << " to " << new_tag;
-      pReq->result_tag = new_tag;
+    if (new_tag != result_tag) {
+      VLOG(jdwp) << "  JDWP promoted result from " << result_tag << " to " << new_tag;
+      result_tag = new_tag;
     }
 
     // Register the object in the registry and reference its ObjectId. This ensures
     // GC safety and prevents from accessing stale reference if the object is moved.
-    pReq->result_value = gRegistry->Add(object_result.Get());
+    result_value = gRegistry->Add(object_result.Get());
   } else {
     // Primitive result.
-    DCHECK(IsPrimitiveTag(pReq->result_tag));
-    pReq->result_value = result.GetJ();
+    DCHECK(IsPrimitiveTag(result_tag));
+    result_value = result.GetJ();
+  }
+  const bool is_constructor = m->IsConstructor() && !m->IsStatic();
+  if (is_constructor) {
+    // If we invoked a constructor (which actually returns void), return the receiver,
+    // unless we threw, in which case we return null.
+    result_tag = JDWP::JT_OBJECT;
+    if (exceptionObjectId == 0) {
+      // TODO we could keep the receiver ObjectId in the DebugInvokeReq to avoid looking into the
+      // object registry.
+      result_value = GetObjectRegistry()->Add(pReq->receiver.Read());
+    } else {
+      result_value = 0;
+    }
   }
 
-  if (old_exception.Get() != nullptr) {
-    soa.Self()->SetException(old_exception.Get());
+  // Suspend other threads if the invoke is not single-threaded.
+  if ((pReq->options & JDWP::INVOKE_SINGLE_THREADED) == 0) {
+    soa.Self()->TransitionFromRunnableToSuspended(kWaitingForDebuggerSuspension);
+    VLOG(jdwp) << "      Suspending all threads";
+    Runtime::Current()->GetThreadList()->SuspendAllForDebugger();
+    soa.Self()->TransitionFromSuspendedToRunnable();
+  }
+
+  VLOG(jdwp) << "  --> returned " << result_tag
+             << StringPrintf(" %#" PRIx64 " (except=%#" PRIx64 ")", result_value,
+                             exceptionObjectId);
+
+  // Show detailed debug output.
+  if (result_tag == JDWP::JT_STRING && exceptionObjectId == 0) {
+    if (result_value != 0) {
+      if (VLOG_IS_ON(jdwp)) {
+        std::string result_string;
+        JDWP::JdwpError error = Dbg::StringToUtf8(result_value, &result_string);
+        CHECK_EQ(error, JDWP::ERR_NONE);
+        VLOG(jdwp) << "      string '" << result_string << "'";
+      }
+    } else {
+      VLOG(jdwp) << "      string (null)";
+    }
+  }
+
+  // Attach the reply to DebugInvokeReq so it can be sent to the debugger when the event thread
+  // is ready to suspend.
+  BuildInvokeReply(pReq->reply, pReq->request_id, result_tag, result_value, exceptionObjectId);
+}
+
+void Dbg::BuildInvokeReply(JDWP::ExpandBuf* pReply, uint32_t request_id, JDWP::JdwpTag result_tag,
+                           uint64_t result_value, JDWP::ObjectId exception) {
+  // Make room for the JDWP header since we do not know the size of the reply yet.
+  JDWP::expandBufAddSpace(pReply, kJDWPHeaderLen);
+
+  size_t width = GetTagWidth(result_tag);
+  JDWP::expandBufAdd1(pReply, result_tag);
+  if (width != 0) {
+    WriteValue(pReply, width, result_value);
+  }
+  JDWP::expandBufAdd1(pReply, JDWP::JT_OBJECT);
+  JDWP::expandBufAddObjectId(pReply, exception);
+
+  // Now we know the size, we can complete the JDWP header.
+  uint8_t* buf = expandBufGetBuffer(pReply);
+  JDWP::Set4BE(buf + kJDWPHeaderSizeOffset, expandBufGetLength(pReply));
+  JDWP::Set4BE(buf + kJDWPHeaderIdOffset, request_id);
+  JDWP::Set1(buf + kJDWPHeaderFlagsOffset, kJDWPFlagReply);  // flags
+  JDWP::Set2BE(buf + kJDWPHeaderErrorCodeOffset, JDWP::ERR_NONE);
+}
+
+void Dbg::FinishInvokeMethod(DebugInvokeReq* pReq) {
+  CHECK_NE(Thread::Current(), GetDebugThread()) << "This must be called by the event thread";
+
+  JDWP::ExpandBuf* const pReply = pReq->reply;
+  CHECK(pReply != nullptr) << "No reply attached to DebugInvokeReq";
+
+  // We need to prevent other threads (including JDWP thread) from interacting with the debugger
+  // while we send the reply but are not yet suspended. The JDWP token will be released just before
+  // we suspend ourself again (see ThreadList::SuspendSelfForDebugger).
+  gJdwpState->AcquireJdwpTokenForEvent(pReq->thread_id);
+
+  // Send the reply unless the debugger detached before the completion of the method.
+  if (IsDebuggerActive()) {
+    const size_t replyDataLength = expandBufGetLength(pReply) - kJDWPHeaderLen;
+    VLOG(jdwp) << StringPrintf("REPLY INVOKE id=0x%06x (length=%zu)",
+                               pReq->request_id, replyDataLength);
+
+    gJdwpState->SendRequest(pReply);
+  } else {
+    VLOG(jdwp) << "Not sending invoke reply because debugger detached";
   }
 }
 
diff --git a/runtime/debugger.h b/runtime/debugger.h
index 7c586a4..e40f10f 100644
--- a/runtime/debugger.h
+++ b/runtime/debugger.h
@@ -45,6 +45,7 @@ class AllocRecord;
 class ArtField;
 class ArtMethod;
 class ObjectRegistry;
+class ScopedObjectAccess;
 class ScopedObjectAccessUnchecked;
 class StackVisitor;
 class Thread;
@@ -53,33 +54,32 @@ class Thread;
  * Invoke-during-breakpoint support.
  */
 struct DebugInvokeReq {
-  DebugInvokeReq(mirror::Object* invoke_receiver, mirror::Class* invoke_class,
+  DebugInvokeReq(uint32_t invoke_request_id, JDWP::ObjectId invoke_thread_id,
+                 mirror::Object* invoke_receiver, mirror::Class* invoke_class,
                  ArtMethod* invoke_method, uint32_t invoke_options,
-                 uint64_t* args, uint32_t args_count)
-      : receiver(invoke_receiver), klass(invoke_class), method(invoke_method),
-        arg_count(args_count), arg_values(args), options(invoke_options),
-        error(JDWP::ERR_NONE), result_tag(JDWP::JT_VOID), result_value(0), exception(0),
-        lock("a DebugInvokeReq lock", kBreakpointInvokeLock),
-        cond("a DebugInvokeReq condition variable", lock) {
+                 uint64_t args[], uint32_t args_count)
+      : request_id(invoke_request_id), thread_id(invoke_thread_id), receiver(invoke_receiver),
+        klass(invoke_class), method(invoke_method), arg_count(args_count), arg_values(args),
+        options(invoke_options), reply(JDWP::expandBufAlloc()) {
   }
 
-  /* request */
-  GcRoot<mirror::Object> receiver;      // not used for ClassType.InvokeMethod
+  ~DebugInvokeReq() {
+    JDWP::expandBufFree(reply);
+  }
+
+  // Request
+  const uint32_t request_id;
+  const JDWP::ObjectId thread_id;
+  GcRoot<mirror::Object> receiver;      // not used for ClassType.InvokeMethod.
   GcRoot<mirror::Class> klass;
-  ArtMethod* method;
+  ArtMethod* const method;
   const uint32_t arg_count;
-  uint64_t* const arg_values;   // will be null if arg_count_ == 0
+  std::unique_ptr<uint64_t[]> arg_values;   // will be null if arg_count_ == 0. We take ownership
+                                            // of this array so we must delete it upon destruction.
   const uint32_t options;
 
-  /* result */
-  JDWP::JdwpError error;
-  JDWP::JdwpTag result_tag;
-  uint64_t result_value;        // either a primitive value or an ObjectId
-  JDWP::ObjectId exception;
-
-  /* condition variable to wait on while the method executes */
-  Mutex lock DEFAULT_MUTEX_ACQUIRED_AFTER;
-  ConditionVariable cond GUARDED_BY(lock);
+  // Reply
+  JDWP::ExpandBuf* const reply;
 
   void VisitRoots(RootVisitor* visitor, const RootInfo& root_info)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
@@ -621,19 +621,39 @@ class Dbg {
       LOCKS_EXCLUDED(Locks::thread_list_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
-  // Invoke support for commands ClassType.InvokeMethod, ClassType.NewInstance and
-  // ObjectReference.InvokeMethod.
-  static JDWP::JdwpError InvokeMethod(JDWP::ObjectId thread_id, JDWP::ObjectId object_id,
-                                      JDWP::RefTypeId class_id, JDWP::MethodId method_id,
-                                      uint32_t arg_count, uint64_t* arg_values,
-                                      JDWP::JdwpTag* arg_types, uint32_t options,
-                                      JDWP::JdwpTag* pResultTag, uint64_t* pResultValue,
-                                      JDWP::ObjectId* pExceptObj)
+  /*
+   * Invoke support
+   */
+
+  // Called by the JDWP thread to prepare invocation in the event thread (suspended on an event).
+  // If the information sent by the debugger is incorrect, it will send a reply with the
+  // appropriate error code. Otherwise, it will attach a DebugInvokeReq object to the event thread
+  // and resume it (and possibly other threads depending on the invoke options).
+  // Unlike other commands, the JDWP thread will not send the reply to the debugger (see
+  // JdwpState::ProcessRequest). The reply will be sent by the event thread itself after method
+  // invocation completes (see FinishInvokeMethod). This is required to allow the JDWP thread to
+  // process incoming commands from the debugger while the invocation is still in progress in the
+  // event thread, especially if it gets suspended by a debug event occurring in another thread.
+  static JDWP::JdwpError PrepareInvokeMethod(uint32_t request_id, JDWP::ObjectId thread_id,
+                                             JDWP::ObjectId object_id, JDWP::RefTypeId class_id,
+                                             JDWP::MethodId method_id, uint32_t arg_count,
+                                             uint64_t arg_values[], JDWP::JdwpTag* arg_types,
+                                             uint32_t options)
       LOCKS_EXCLUDED(Locks::thread_list_lock_,
                      Locks::thread_suspend_count_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
+  // Called by the event thread to execute a method prepared by the JDWP thread in the given
+  // DebugInvokeReq object. Once the invocation completes, the event thread attaches a reply
+  // to that DebugInvokeReq object so it can be sent to the debugger only when the event thread
+  // is ready to suspend (see FinishInvokeMethod).
   static void ExecuteMethod(DebugInvokeReq* pReq);
 
+  // Called by the event thread to send the reply of the invoke (created in ExecuteMethod)
+  // before suspending itself. This is to ensure the thread is ready to suspend before the
+  // debugger receives the reply.
+  static void FinishInvokeMethod(DebugInvokeReq* pReq);
+
   /*
    * DDM support.
    */
@@ -717,6 +737,14 @@ class Dbg {
   }
 
  private:
+  static void ExecuteMethodWithoutPendingException(ScopedObjectAccess& soa, DebugInvokeReq* pReq)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
+  static void BuildInvokeReply(JDWP::ExpandBuf* pReply, uint32_t request_id,
+                               JDWP::JdwpTag result_tag, uint64_t result_value,
+                               JDWP::ObjectId exception)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
   static JDWP::JdwpError GetLocalValue(const StackVisitor& visitor,
                                        ScopedObjectAccessUnchecked& soa, int slot,
                                        JDWP::JdwpTag tag, uint8_t* buf, size_t width)
diff --git a/runtime/jdwp/jdwp.h b/runtime/jdwp/jdwp.h
index e18d10f..7c48985 100644
--- a/runtime/jdwp/jdwp.h
+++ b/runtime/jdwp/jdwp.h
@@ -297,7 +297,7 @@ struct JdwpState {
 
  private:
   explicit JdwpState(const JdwpOptions* options);
-  size_t ProcessRequest(Request* request, ExpandBuf* pReply);
+  size_t ProcessRequest(Request* request, ExpandBuf* pReply, bool* skip_reply);
   bool InvokeInProgress();
   bool IsConnected();
   void SuspendByPolicy(JdwpSuspendPolicy suspend_policy, JDWP::ObjectId thread_self_id)
diff --git a/runtime/jdwp/jdwp_event.cc b/runtime/jdwp/jdwp_event.cc
index 612af8b..14f097f 100644
--- a/runtime/jdwp/jdwp_event.cc
+++ b/runtime/jdwp/jdwp_event.cc
@@ -99,10 +99,6 @@ put ourselves to sleep.  That way we don't interfere with anyone else and
 don't allow anyone else to interfere with us.
 */
 
-
-#define kJdwpEventCommandSet    64
-#define kJdwpCompositeCommand   100
-
 namespace art {
 
 namespace JDWP {
@@ -612,13 +608,10 @@ void JdwpState::SuspendByPolicy(JdwpSuspendPolicy suspend_policy, JDWP::ObjectId
      */
     DebugInvokeReq* const pReq = Dbg::GetInvokeReq();
     if (pReq == nullptr) {
-      /*LOGD("SuspendByPolicy: no invoke needed");*/
       break;
     }
 
-    /* grab this before posting/suspending again */
-    AcquireJdwpTokenForEvent(thread_self_id);
-
+    // Execute method.
     Dbg::ExecuteMethod(pReq);
   }
 }
@@ -749,11 +742,11 @@ static ExpandBuf* eventPrep() {
 void JdwpState::EventFinish(ExpandBuf* pReq) {
   uint8_t* buf = expandBufGetBuffer(pReq);
 
-  Set4BE(buf, expandBufGetLength(pReq));
-  Set4BE(buf + 4, NextRequestSerial());
-  Set1(buf + 8, 0);     /* flags */
-  Set1(buf + 9, kJdwpEventCommandSet);
-  Set1(buf + 10, kJdwpCompositeCommand);
+  Set4BE(buf + kJDWPHeaderSizeOffset, expandBufGetLength(pReq));
+  Set4BE(buf + kJDWPHeaderIdOffset, NextRequestSerial());
+  Set1(buf + kJDWPHeaderFlagsOffset, 0);     /* flags */
+  Set1(buf + kJDWPHeaderCmdSetOffset, kJDWPEventCmdSet);
+  Set1(buf + kJDWPHeaderCmdOffset, kJDWPEventCompositeCmd);
 
   SendRequest(pReq);
 
diff --git a/runtime/jdwp/jdwp_handler.cc b/runtime/jdwp/jdwp_handler.cc
index f7f70f6..d4e2656 100644
--- a/runtime/jdwp/jdwp_handler.cc
+++ b/runtime/jdwp/jdwp_handler.cc
@@ -52,17 +52,6 @@ std::string DescribeRefTypeId(const RefTypeId& ref_type_id) {
   return StringPrintf("%#" PRIx64 " (%s)", ref_type_id, signature.c_str());
 }
 
-// Helper function: write a variable-width value into the output input buffer.
-static void WriteValue(ExpandBuf* pReply, int width, uint64_t value) {
-  switch (width) {
-  case 1:     expandBufAdd1(pReply, value); break;
-  case 2:     expandBufAdd2BE(pReply, value); break;
-  case 4:     expandBufAdd4BE(pReply, value); break;
-  case 8:     expandBufAdd8BE(pReply, value); break;
-  default:    LOG(FATAL) << width; break;
-  }
-}
-
 static JdwpError WriteTaggedObject(ExpandBuf* reply, ObjectId object_id)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   uint8_t tag;
@@ -92,7 +81,7 @@ static JdwpError WriteTaggedObjectList(ExpandBuf* reply, const std::vector<Objec
  * If "is_constructor" is set, this returns "object_id" rather than the
  * expected-to-be-void return value of the called function.
  */
-static JdwpError RequestInvoke(JdwpState*, Request* request, ExpandBuf* pReply,
+static JdwpError RequestInvoke(JdwpState*, Request* request,
                                ObjectId thread_id, ObjectId object_id,
                                RefTypeId class_id, MethodId method_id, bool is_constructor)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
@@ -122,49 +111,15 @@ static JdwpError RequestInvoke(JdwpState*, Request* request, ExpandBuf* pReply,
                              (options & INVOKE_SINGLE_THREADED) ? " (SINGLE_THREADED)" : "",
                              (options & INVOKE_NONVIRTUAL) ? " (NONVIRTUAL)" : "");
 
-  JdwpTag resultTag;
-  uint64_t resultValue;
-  ObjectId exceptObjId;
-  JdwpError err = Dbg::InvokeMethod(thread_id, object_id, class_id, method_id, arg_count,
-                                    argValues.get(), argTypes.get(), options, &resultTag,
-                                    &resultValue, &exceptObjId);
-  if (err != ERR_NONE) {
-    return err;
-  }
-
-  if (is_constructor) {
-    // If we invoked a constructor (which actually returns void), return the receiver,
-    // unless we threw, in which case we return null.
-    resultTag = JT_OBJECT;
-    resultValue = (exceptObjId == 0) ? object_id : 0;
-  }
-
-  size_t width = Dbg::GetTagWidth(resultTag);
-  expandBufAdd1(pReply, resultTag);
-  if (width != 0) {
-    WriteValue(pReply, width, resultValue);
-  }
-  expandBufAdd1(pReply, JT_OBJECT);
-  expandBufAddObjectId(pReply, exceptObjId);
-
-  VLOG(jdwp) << "  --> returned " << resultTag
-      << StringPrintf(" %#" PRIx64 " (except=%#" PRIx64 ")", resultValue, exceptObjId);
-
-  /* show detailed debug output */
-  if (resultTag == JT_STRING && exceptObjId == 0) {
-    if (resultValue != 0) {
-      if (VLOG_IS_ON(jdwp)) {
-        std::string result_string;
-        JDWP::JdwpError error = Dbg::StringToUtf8(resultValue, &result_string);
-        CHECK_EQ(error, JDWP::ERR_NONE);
-        VLOG(jdwp) << "      string '" << result_string << "'";
-      }
-    } else {
-      VLOG(jdwp) << "      string (null)";
-    }
+  JDWP::JdwpError error =  Dbg::PrepareInvokeMethod(request->GetId(), thread_id, object_id,
+                                                    class_id, method_id, arg_count,
+                                                    argValues.get(), argTypes.get(), options);
+  if (error == JDWP::ERR_NONE) {
+    // We successfully requested the invoke. The event thread now owns the arguments array in its
+    // DebugInvokeReq mailbox.
+    argValues.release();
   }
-
-  return err;
+  return error;
 }
 
 static JdwpError VM_Version(JdwpState*, Request*, ExpandBuf* pReply)
@@ -684,13 +639,14 @@ static JdwpError CT_SetValues(JdwpState* , Request* request, ExpandBuf*)
  * Example: Eclipse sometimes uses java/lang/Class.forName(String s) on
  * values in the "variables" display.
  */
-static JdwpError CT_InvokeMethod(JdwpState* state, Request* request, ExpandBuf* pReply)
+static JdwpError CT_InvokeMethod(JdwpState* state, Request* request,
+                                 ExpandBuf* pReply ATTRIBUTE_UNUSED)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   RefTypeId class_id = request->ReadRefTypeId();
   ObjectId thread_id = request->ReadThreadId();
   MethodId method_id = request->ReadMethodId();
 
-  return RequestInvoke(state, request, pReply, thread_id, 0, class_id, method_id, false);
+  return RequestInvoke(state, request, thread_id, 0, class_id, method_id, false);
 }
 
 /*
@@ -700,7 +656,8 @@ static JdwpError CT_InvokeMethod(JdwpState* state, Request* request, ExpandBuf*
  * Example: in IntelliJ, create a watch on "new String(myByteArray)" to
  * see the contents of a byte[] as a string.
  */
-static JdwpError CT_NewInstance(JdwpState* state, Request* request, ExpandBuf* pReply)
+static JdwpError CT_NewInstance(JdwpState* state, Request* request,
+                                ExpandBuf* pReply ATTRIBUTE_UNUSED)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   RefTypeId class_id = request->ReadRefTypeId();
   ObjectId thread_id = request->ReadThreadId();
@@ -711,7 +668,7 @@ static JdwpError CT_NewInstance(JdwpState* state, Request* request, ExpandBuf* p
   if (status != ERR_NONE) {
     return status;
   }
-  return RequestInvoke(state, request, pReply, thread_id, object_id, class_id, method_id, true);
+  return RequestInvoke(state, request, thread_id, object_id, class_id, method_id, true);
 }
 
 /*
@@ -863,14 +820,15 @@ static JdwpError OR_MonitorInfo(JdwpState*, Request* request, ExpandBuf* reply)
  * object), it will try to invoke the object's toString() function.  This
  * feature becomes crucial when examining ArrayLists with Eclipse.
  */
-static JdwpError OR_InvokeMethod(JdwpState* state, Request* request, ExpandBuf* pReply)
+static JdwpError OR_InvokeMethod(JdwpState* state, Request* request,
+                                 ExpandBuf* pReply ATTRIBUTE_UNUSED)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   ObjectId object_id = request->ReadObjectId();
   ObjectId thread_id = request->ReadThreadId();
   RefTypeId class_id = request->ReadRefTypeId();
   MethodId method_id = request->ReadMethodId();
 
-  return RequestInvoke(state, request, pReply, thread_id, object_id, class_id, method_id, false);
+  return RequestInvoke(state, request, thread_id, object_id, class_id, method_id, false);
 }
 
 static JdwpError OR_DisableCollection(JdwpState*, Request* request, ExpandBuf*)
@@ -1602,13 +1560,27 @@ static std::string DescribeCommand(Request* request) {
   return result;
 }
 
+// Returns true if the given command_set and command identify an "invoke" command.
+static bool IsInvokeCommand(uint8_t command_set, uint8_t command) {
+  if (command_set == kJDWPClassTypeCmdSet) {
+    return command == kJDWPClassTypeInvokeMethodCmd || command == kJDWPClassTypeNewInstanceCmd;
+  } else if (command_set == kJDWPObjectReferenceCmdSet) {
+    return command == kJDWPObjectReferenceInvokeCmd;
+  } else {
+    return false;
+  }
+}
+
 /*
- * Process a request from the debugger.
+ * Process a request from the debugger. The skip_reply flag is set to true to indicate to the
+ * caller the reply must not be sent to the debugger. This is used for invoke commands where the
+ * reply is sent by the event thread after completing the invoke.
  *
  * On entry, the JDWP thread is in VMWAIT.
  */
-size_t JdwpState::ProcessRequest(Request* request, ExpandBuf* pReply) {
+size_t JdwpState::ProcessRequest(Request* request, ExpandBuf* pReply, bool* skip_reply) {
   JdwpError result = ERR_NONE;
+  *skip_reply = false;
 
   if (request->GetCommandSet() != kJDWPDdmCmdSet) {
     /*
@@ -1661,24 +1633,31 @@ size_t JdwpState::ProcessRequest(Request* request, ExpandBuf* pReply) {
     result = ERR_NOT_IMPLEMENTED;
   }
 
-  /*
-   * Set up the reply header.
-   *
-   * If we encountered an error, only send the header back.
-   */
-  uint8_t* replyBuf = expandBufGetBuffer(pReply);
-  size_t replyLength = (result == ERR_NONE) ? expandBufGetLength(pReply) : kJDWPHeaderLen;
-  Set4BE(replyBuf + 0, replyLength);
-  Set4BE(replyBuf + 4, request->GetId());
-  Set1(replyBuf + 8, kJDWPFlagReply);
-  Set2BE(replyBuf + 9, result);
-
-  CHECK_GT(expandBufGetLength(pReply), 0U) << GetCommandName(request) << " " << request->GetId();
-
-  size_t respLen = expandBufGetLength(pReply) - kJDWPHeaderLen;
-  VLOG(jdwp) << "REPLY: " << GetCommandName(request) << " " << result << " (length=" << respLen << ")";
-  if (false) {
-    VLOG(jdwp) << HexDump(expandBufGetBuffer(pReply) + kJDWPHeaderLen, respLen, false, "");
+  size_t replyLength = 0U;
+  if (result == ERR_NONE && IsInvokeCommand(request->GetCommandSet(), request->GetCommand())) {
+    // We successfully request an invoke in the event thread. It will send the reply once the
+    // invoke completes so we must not send it now.
+    *skip_reply = true;
+  } else {
+    /*
+     * Set up the reply header.
+     *
+     * If we encountered an error, only send the header back.
+     */
+    uint8_t* replyBuf = expandBufGetBuffer(pReply);
+    replyLength = (result == ERR_NONE) ? expandBufGetLength(pReply) : kJDWPHeaderLen;
+    Set4BE(replyBuf + kJDWPHeaderSizeOffset, replyLength);
+    Set4BE(replyBuf + kJDWPHeaderIdOffset, request->GetId());
+    Set1(replyBuf + kJDWPHeaderFlagsOffset, kJDWPFlagReply);
+    Set2BE(replyBuf + kJDWPHeaderErrorCodeOffset, result);
+
+    CHECK_GT(expandBufGetLength(pReply), 0U) << GetCommandName(request) << " " << request->GetId();
+
+    size_t respLen = expandBufGetLength(pReply) - kJDWPHeaderLen;
+    VLOG(jdwp) << "REPLY: " << GetCommandName(request) << " " << result << " (length=" << respLen << ")";
+    if (false) {
+      VLOG(jdwp) << HexDump(expandBufGetBuffer(pReply) + kJDWPHeaderLen, respLen, false, "");
+    }
   }
 
   VLOG(jdwp) << "----------";
diff --git a/runtime/jdwp/jdwp_main.cc b/runtime/jdwp/jdwp_main.cc
index e6b97a2..6bc5e27 100644
--- a/runtime/jdwp/jdwp_main.cc
+++ b/runtime/jdwp/jdwp_main.cc
@@ -395,8 +395,15 @@ bool JdwpState::HandlePacket() {
   JDWP::Request request(netStateBase->input_buffer_, netStateBase->input_count_);
 
   ExpandBuf* pReply = expandBufAlloc();
-  size_t replyLength = ProcessRequest(&request, pReply);
-  ssize_t cc = netStateBase->WritePacket(pReply, replyLength);
+  bool skip_reply = false;
+  size_t replyLength = ProcessRequest(&request, pReply, &skip_reply);
+  ssize_t cc = 0;
+  if (!skip_reply) {
+    cc = netStateBase->WritePacket(pReply, replyLength);
+  } else {
+    DCHECK_EQ(replyLength, 0U);
+  }
+  expandBufFree(pReply);
 
   /*
    * We processed this request and sent its reply so we can release the JDWP token.
@@ -405,10 +412,8 @@ bool JdwpState::HandlePacket() {
 
   if (cc != static_cast<ssize_t>(replyLength)) {
     PLOG(ERROR) << "Failed sending reply to debugger";
-    expandBufFree(pReply);
     return false;
   }
-  expandBufFree(pReply);
   netStateBase->ConsumeBytes(request.GetLength());
   {
     MutexLock mu(self, shutdown_lock_);
diff --git a/runtime/jdwp/jdwp_priv.h b/runtime/jdwp/jdwp_priv.h
index f290be0..d58467d 100644
--- a/runtime/jdwp/jdwp_priv.h
+++ b/runtime/jdwp/jdwp_priv.h
@@ -29,15 +29,32 @@
 /*
  * JDWP constants.
  */
-#define kJDWPHeaderLen  11
-#define kJDWPFlagReply  0x80
-
-#define kMagicHandshake     "JDWP-Handshake"
-#define kMagicHandshakeLen  (sizeof(kMagicHandshake)-1)
+static constexpr size_t kJDWPHeaderSizeOffset = 0U;
+static constexpr size_t kJDWPHeaderIdOffset = 4U;
+static constexpr size_t kJDWPHeaderFlagsOffset = 8U;
+static constexpr size_t kJDWPHeaderErrorCodeOffset = 9U;
+static constexpr size_t kJDWPHeaderCmdSetOffset = 9U;
+static constexpr size_t kJDWPHeaderCmdOffset = 10U;
+static constexpr size_t kJDWPHeaderLen = 11U;
+static constexpr uint8_t kJDWPFlagReply = 0x80;
+
+static constexpr const char kMagicHandshake[] = "JDWP-Handshake";
+static constexpr size_t kMagicHandshakeLen = sizeof(kMagicHandshake) - 1;
+
+/* Invoke commands */
+static constexpr uint8_t kJDWPClassTypeCmdSet = 3U;
+static constexpr uint8_t kJDWPClassTypeInvokeMethodCmd = 3U;
+static constexpr uint8_t kJDWPClassTypeNewInstanceCmd = 4U;
+static constexpr uint8_t kJDWPObjectReferenceCmdSet = 9U;
+static constexpr uint8_t kJDWPObjectReferenceInvokeCmd = 6U;
+
+/* Event command */
+static constexpr uint8_t kJDWPEventCmdSet = 64U;
+static constexpr uint8_t kJDWPEventCompositeCmd = 100U;
 
 /* DDM support */
-#define kJDWPDdmCmdSet  199     /* 0xc7, or 'G'+128 */
-#define kJDWPDdmCmd     1
+static constexpr uint8_t kJDWPDdmCmdSet = 199U;  // 0xc7, or 'G'+128
+static constexpr uint8_t kJDWPDdmCmd = 1U;
 
 namespace art {
 
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 845345a..29635a4 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -2517,12 +2517,11 @@ void Thread::SetDebugInvokeReq(DebugInvokeReq* req) {
 }
 
 void Thread::ClearDebugInvokeReq() {
-  CHECK(Dbg::IsDebuggerActive());
   CHECK(GetInvokeReq() != nullptr) << "Debug invoke req not active in thread " << *this;
   CHECK(Thread::Current() == this) << "Debug invoke must be finished by the thread itself";
-  // We do not own the DebugInvokeReq* so we must not delete it, it is the responsibility of
-  // the owner (the JDWP thread).
+  DebugInvokeReq* req = tlsPtr_.debug_invoke_req;
   tlsPtr_.debug_invoke_req = nullptr;
+  delete req;
 }
 
 void Thread::PushVerifier(verifier::MethodVerifier* verifier) {
diff --git a/runtime/thread.h b/runtime/thread.h
index 3f0d0a5..27f029e 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -774,15 +774,14 @@ class Thread {
   void DeactivateSingleStepControl();
 
   // Sets debug invoke request for debugging. When the thread is resumed,
-  // it executes the method described by this request then suspends itself.
-  // The thread does not take ownership of the given DebugInvokeReq*, it is
-  // owned by the JDWP thread which is waiting for the execution of the
-  // method.
+  // it executes the method described by this request then sends the reply
+  // before suspending itself. The thread takes the ownership of the given
+  // DebugInvokeReq*. It is deleted by a call to ClearDebugInvokeReq.
   void SetDebugInvokeReq(DebugInvokeReq* req);
 
   // Clears debug invoke request for debugging. When the thread completes
-  // method invocation, it clears its debug invoke request, signals the
-  // JDWP thread and suspends itself.
+  // method invocation, it deletes its debug invoke request and suspends
+  // itself.
   void ClearDebugInvokeReq();
 
   // Returns the fake exception used to activate deoptimization.
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index af9ba68..b697b43 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -875,31 +875,36 @@ void ThreadList::SuspendSelfForDebugger() {
 
   // The debugger thread must not suspend itself due to debugger activity!
   Thread* debug_thread = Dbg::GetDebugThread();
-  CHECK(debug_thread != nullptr);
   CHECK(self != debug_thread);
   CHECK_NE(self->GetState(), kRunnable);
   Locks::mutator_lock_->AssertNotHeld(self);
 
-  {
+  // The debugger may have detached while we were executing an invoke request. In that case, we
+  // must not suspend ourself.
+  DebugInvokeReq* pReq = self->GetInvokeReq();
+  const bool skip_thread_suspension = (pReq != nullptr && !Dbg::IsDebuggerActive());
+  if (!skip_thread_suspension) {
     // Collisions with other suspends aren't really interesting. We want
     // to ensure that we're the only one fiddling with the suspend count
     // though.
     MutexLock mu(self, *Locks::thread_suspend_count_lock_);
     self->ModifySuspendCount(self, +1, true);
     CHECK_GT(self->GetSuspendCount(), 0);
-  }
 
-  VLOG(threads) << *self << " self-suspending (debugger)";
+    VLOG(threads) << *self << " self-suspending (debugger)";
+  } else {
+    // We must no longer be subject to debugger suspension.
+    MutexLock mu(self, *Locks::thread_suspend_count_lock_);
+    CHECK_EQ(self->GetDebugSuspendCount(), 0) << "Debugger detached without resuming us";
 
-  // Tell JDWP we've completed invocation and are ready to suspend.
-  DebugInvokeReq* const pReq = self->GetInvokeReq();
+    VLOG(threads) << *self << " not self-suspending because debugger detached during invoke";
+  }
+
+  // If the debugger requested an invoke, we need to send the reply and clear the request.
   if (pReq != nullptr) {
-    // Clear debug invoke request before signaling.
+    Dbg::FinishInvokeMethod(pReq);
     self->ClearDebugInvokeReq();
-
-    VLOG(jdwp) << "invoke complete, signaling";
-    MutexLock mu(self, pReq->lock);
-    pReq->cond.Signal(self);
+    pReq = nullptr;  // object has been deleted, clear it for safety.
   }
 
   // Tell JDWP that we've completed suspension. The JDWP thread can't