JDWP: more GC safety

Ensures GC safety when keeping references that may be moved by GC:
- SingleStepControl: stores ArtMethod* in a GcRoot
- ModBasket: stores references in a StackHandleScope

Bug: 18166750

(cherry picked from commit 261bc044a3575512869586593e99e97cd8b1c321)

Change-Id: I35971a901537956739d1f089d61cb4ea9dc6c93d

5 files changed, 103 insertions, 56 deletions

diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 4bc9f98dfe..852ba49cd2 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -411,7 +411,7 @@ void DebugInvokeReq::VisitRoots(RootVisitor* visitor, const RootInfo& root_info)
 }
 
 void SingleStepControl::VisitRoots(RootVisitor* visitor, const RootInfo& root_info) {
-  visitor->VisitRootIfNonNull(reinterpret_cast<mirror::Object**>(&method_), root_info);
+  method_.VisitRootIfNonNull(visitor, root_info);
 }
 
 void SingleStepControl::AddDexPc(uint32_t dex_pc) {
@@ -2929,10 +2929,11 @@ void Dbg::PostException(mirror::Throwable* exception_object) {
   if (!IsDebuggerActive()) {
     return;
   }
-  StackHandleScope<1> handle_scope(Thread::Current());
+  Thread* const self = Thread::Current();
+  StackHandleScope<1> handle_scope(self);
   Handle<mirror::Throwable> h_exception(handle_scope.NewHandle(exception_object));
   std::unique_ptr<Context> context(Context::Create());
-  CatchLocationFinder clf(Thread::Current(), h_exception, context.get());
+  CatchLocationFinder clf(self, h_exception, context.get());
   clf.WalkStack(/* include_transitions */ false);
   JDWP::EventLocation exception_throw_location;
   SetEventLocation(&exception_throw_location, clf.GetThrowMethod(), clf.GetThrowDexPc());
@@ -3981,7 +3982,7 @@ void Dbg::ExecuteMethod(DebugInvokeReq* pReq) {
   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.Get());
+  pReq->exception = gRegistry->Add(exception);
   if (pReq->exception != 0) {
     VLOG(jdwp) << "  JDWP invocation returning with exception=" << exception.Get()
                << " " << exception->Dump();
diff --git a/runtime/debugger.h b/runtime/debugger.h
index 789a0a4dca..811d345262 100644
--- a/runtime/debugger.h
+++ b/runtime/debugger.h
@@ -109,8 +109,8 @@ class SingleStepControl {
     return stack_depth_;
   }
 
-  mirror::ArtMethod* GetMethod() const {
-    return method_;
+  mirror::ArtMethod* GetMethod() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+    return method_.Read();
   }
 
   const std::set<uint32_t>& GetDexPcs() const {
@@ -138,7 +138,7 @@ class SingleStepControl {
   // set of DEX pcs associated to the source line number where the suspension occurred.
   // This is used to support SD_INTO and SD_OVER single-step depths so we detect when a single-step
   // causes the execution of an instruction in a different method or at a different line number.
-  mirror::ArtMethod* method_;
+  GcRoot<mirror::ArtMethod> method_;
   std::set<uint32_t> dex_pcs_;
 
   DISALLOW_COPY_AND_ASSIGN(SingleStepControl);
diff --git a/runtime/jdwp/jdwp_event.cc b/runtime/jdwp/jdwp_event.cc
index ab3f2e4ddd..ff75268daa 100644
--- a/runtime/jdwp/jdwp_event.cc
+++ b/runtime/jdwp/jdwp_event.cc
@@ -32,6 +32,8 @@
 #include "scoped_thread_state_change.h"
 #include "thread-inl.h"
 
+#include "handle_scope-inl.h"
+
 /*
 General notes:
 
@@ -108,20 +110,32 @@ namespace JDWP {
  * Stuff to compare against when deciding if a mod matches.  Only the
  * values for mods valid for the event being evaluated will be filled in.
  * The rest will be zeroed.
+ * Must be allocated on the stack only. This is enforced by removing the
+ * operator new.
  */
 struct ModBasket {
-  ModBasket() : pLoc(nullptr), thread(nullptr), locationClass(nullptr), exceptionClass(nullptr),
-                caught(false), field(nullptr), thisPtr(nullptr) { }
-
-  const EventLocation*  pLoc;             /* LocationOnly */
-  std::string           className;        /* ClassMatch/ClassExclude */
-  Thread*               thread;           /* ThreadOnly */
-  mirror::Class*        locationClass;    /* ClassOnly */
-  mirror::Class*        exceptionClass;   /* ExceptionOnly */
-  bool                  caught;           /* ExceptionOnly */
-  ArtField*             field;            /* FieldOnly */
-  mirror::Object*       thisPtr;          /* InstanceOnly */
+  explicit ModBasket(Thread* self)
+    : hs(self), pLoc(nullptr), thread(self),
+      locationClass(hs.NewHandle<mirror::Class>(nullptr)),
+      exceptionClass(hs.NewHandle<mirror::Class>(nullptr)),
+      caught(false),
+      field(nullptr),
+      thisPtr(hs.NewHandle<mirror::Object>(nullptr)) { }
+
+  StackHandleScope<3> hs;
+  const EventLocation*            pLoc;             /* LocationOnly */
+  std::string                     className;        /* ClassMatch/ClassExclude */
+  Thread* const                   thread;           /* ThreadOnly */
+  MutableHandle<mirror::Class>    locationClass;    /* ClassOnly */
+  MutableHandle<mirror::Class>    exceptionClass;   /* ExceptionOnly */
+  bool                            caught;           /* ExceptionOnly */
+  ArtField*                       field;            /* FieldOnly */
+  MutableHandle<mirror::Object>   thisPtr;          /* InstanceOnly */
   /* nothing for StepOnly -- handled differently */
+
+ private:
+  DISALLOW_ALLOCATION();  // forbids allocation on the heap.
+  DISALLOW_IMPLICIT_CONSTRUCTORS(ModBasket);
 };
 
 static bool NeedsFullDeoptimization(JdwpEventKind eventKind) {
@@ -457,7 +471,7 @@ static bool ModsMatch(JdwpEvent* pEvent, const ModBasket& basket)
       }
       break;
     case MK_CLASS_ONLY:
-      if (!Dbg::MatchType(basket.locationClass, pMod->classOnly.refTypeId)) {
+      if (!Dbg::MatchType(basket.locationClass.Get(), pMod->classOnly.refTypeId)) {
         return false;
       }
       break;
@@ -478,7 +492,7 @@ static bool ModsMatch(JdwpEvent* pEvent, const ModBasket& basket)
       break;
     case MK_EXCEPTION_ONLY:
       if (pMod->exceptionOnly.refTypeId != 0 &&
-          !Dbg::MatchType(basket.exceptionClass, pMod->exceptionOnly.refTypeId)) {
+          !Dbg::MatchType(basket.exceptionClass.Get(), pMod->exceptionOnly.refTypeId)) {
         return false;
       }
       if ((basket.caught && !pMod->exceptionOnly.caught) ||
@@ -497,7 +511,7 @@ static bool ModsMatch(JdwpEvent* pEvent, const ModBasket& basket)
       }
       break;
     case MK_INSTANCE_ONLY:
-      if (!Dbg::MatchInstance(pMod->instanceOnly.objectId, basket.thisPtr)) {
+      if (!Dbg::MatchInstance(pMod->instanceOnly.objectId, basket.thisPtr.Get())) {
         return false;
       }
       break;
@@ -825,12 +839,11 @@ void JdwpState::PostLocationEvent(const EventLocation* pLoc, mirror::Object* thi
   DCHECK(pLoc->method != nullptr);
   DCHECK_EQ(pLoc->method->IsStatic(), thisPtr == nullptr);
 
-  ModBasket basket;
+  ModBasket basket(Thread::Current());
   basket.pLoc = pLoc;
-  basket.locationClass = pLoc->method->GetDeclaringClass();
-  basket.thisPtr = thisPtr;
-  basket.thread = Thread::Current();
-  basket.className = Dbg::GetClassName(basket.locationClass);
+  basket.locationClass.Assign(pLoc->method->GetDeclaringClass());
+  basket.thisPtr.Assign(thisPtr);
+  basket.className = Dbg::GetClassName(basket.locationClass.Get());
 
   /*
    * On rare occasions we may need to execute interpreted code in the VM
@@ -924,16 +937,15 @@ void JdwpState::PostFieldEvent(const EventLocation* pLoc, ArtField* field,
   DCHECK_EQ(fieldValue != nullptr, is_modification);
   DCHECK_EQ(field->IsStatic(), this_object == nullptr);
 
-  ModBasket basket;
+  ModBasket basket(Thread::Current());
   basket.pLoc = pLoc;
-  basket.locationClass = pLoc->method->GetDeclaringClass();
-  basket.thisPtr = this_object;
-  basket.thread = Thread::Current();
-  basket.className = Dbg::GetClassName(basket.locationClass);
+  basket.locationClass.Assign(pLoc->method->GetDeclaringClass());
+  basket.thisPtr.Assign(this_object);
+  basket.className = Dbg::GetClassName(basket.locationClass.Get());
   basket.field = field;
 
   if (InvokeInProgress()) {
-    VLOG(jdwp) << "Not posting field event during invoke";
+    VLOG(jdwp) << "Not posting field event during invoke (" << basket.className << ")";
     return;
   }
 
@@ -975,7 +987,7 @@ void JdwpState::PostFieldEvent(const EventLocation* pLoc, ArtField* field,
   uint8_t tag;
   {
     ScopedObjectAccessUnchecked soa(Thread::Current());
-    tag = Dbg::TagFromObject(soa, basket.thisPtr);
+    tag = Dbg::TagFromObject(soa, basket.thisPtr.Get());
   }
 
   for (const JdwpEvent* pEvent : match_list) {
@@ -1028,8 +1040,7 @@ void JdwpState::PostThreadChange(Thread* thread, bool start) {
     return;
   }
 
-  ModBasket basket;
-  basket.thread = thread;
+  ModBasket basket(thread);
 
   std::vector<JdwpEvent*> match_list;
   const JdwpEventKind match_kind = (start) ? EK_THREAD_START : EK_THREAD_DEATH;
@@ -1106,18 +1117,15 @@ void JdwpState::PostException(const EventLocation* pThrowLoc, mirror::Throwable*
     VLOG(jdwp) << "Unexpected: exception event with empty throw location";
   }
 
-  ModBasket basket;
+  ModBasket basket(Thread::Current());
   basket.pLoc = pThrowLoc;
   if (pThrowLoc->method != nullptr) {
-    basket.locationClass = pThrowLoc->method->GetDeclaringClass();
-  } else {
-    basket.locationClass = nullptr;
+    basket.locationClass.Assign(pThrowLoc->method->GetDeclaringClass());
   }
-  basket.thread = Thread::Current();
-  basket.className = Dbg::GetClassName(basket.locationClass);
-  basket.exceptionClass = exception_object->GetClass();
+  basket.className = Dbg::GetClassName(basket.locationClass.Get());
+  basket.exceptionClass.Assign(exception_object->GetClass());
   basket.caught = (pCatchLoc->method != 0);
-  basket.thisPtr = thisPtr;
+  basket.thisPtr.Assign(thisPtr);
 
   /* don't try to post an exception caused by the debugger */
   if (InvokeInProgress()) {
@@ -1188,10 +1196,9 @@ void JdwpState::PostException(const EventLocation* pThrowLoc, mirror::Throwable*
 void JdwpState::PostClassPrepare(mirror::Class* klass) {
   DCHECK(klass != nullptr);
 
-  ModBasket basket;
-  basket.locationClass = klass;
-  basket.thread = Thread::Current();
-  basket.className = Dbg::GetClassName(basket.locationClass);
+  ModBasket basket(Thread::Current());
+  basket.locationClass.Assign(klass);
+  basket.className = Dbg::GetClassName(basket.locationClass.Get());
 
   /* suppress class prep caused by debugger */
   if (InvokeInProgress()) {
@@ -1214,7 +1221,7 @@ void JdwpState::PostClassPrepare(mirror::Class* klass) {
   // debuggers seem to like that.  There might be some advantage to honesty,
   // since the class may not yet be verified.
   int status = JDWP::CS_VERIFIED | JDWP::CS_PREPARED;
-  JDWP::JdwpTypeTag tag = Dbg::GetTypeTag(basket.locationClass);
+  JDWP::JdwpTypeTag tag = Dbg::GetTypeTag(basket.locationClass.Get());
   std::string temp;
   std::string signature(basket.locationClass->GetDescriptor(&temp));
 
diff --git a/runtime/jdwp/object_registry.cc b/runtime/jdwp/object_registry.cc
index a42a58f601..2b28f7df5a 100644
--- a/runtime/jdwp/object_registry.cc
+++ b/runtime/jdwp/object_registry.cc
@@ -36,17 +36,45 @@ ObjectRegistry::ObjectRegistry()
 }
 
 JDWP::RefTypeId ObjectRegistry::AddRefType(mirror::Class* c) {
-  return InternalAdd(c);
+  return Add(c);
 }
 
-JDWP::ObjectId ObjectRegistry::Add(mirror::Object* o) {
-  return InternalAdd(o);
+JDWP::RefTypeId ObjectRegistry::AddRefType(Handle<mirror::Class> c_h) {
+  return Add(c_h);
 }
 
-JDWP::ObjectId ObjectRegistry::InternalAdd(mirror::Object* o) {
+JDWP::ObjectId ObjectRegistry::Add(mirror::Object* o) {
   if (o == nullptr) {
     return 0;
   }
+  Thread* const self = Thread::Current();
+  StackHandleScope<1> hs(self);
+  return InternalAdd(hs.NewHandle(o));
+}
+
+// Template instantiations must be declared below.
+template<class T>
+JDWP::ObjectId ObjectRegistry::Add(Handle<T> obj_h) {
+  if (obj_h.Get() == nullptr) {
+    return 0;
+  }
+  return InternalAdd(obj_h);
+}
+
+// Explicit template instantiation.
+template
+SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+LOCKS_EXCLUDED(Locks::thread_list_lock_, Locks::thread_suspend_count_lock_)
+JDWP::ObjectId ObjectRegistry::Add(Handle<mirror::Object> obj_h);
+
+template
+SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+LOCKS_EXCLUDED(Locks::thread_list_lock_, Locks::thread_suspend_count_lock_)
+JDWP::ObjectId ObjectRegistry::Add(Handle<mirror::Throwable> obj_h);
+
+template<class T>
+JDWP::ObjectId ObjectRegistry::InternalAdd(Handle<T> obj_h) {
+  CHECK(obj_h.Get() != nullptr);
 
   Thread* const self = Thread::Current();
   self->AssertNoPendingException();
@@ -55,9 +83,6 @@ JDWP::ObjectId ObjectRegistry::InternalAdd(mirror::Object* o) {
   Locks::thread_list_lock_->AssertNotHeld(self);
   Locks::thread_suspend_count_lock_->AssertNotHeld(self);
 
-  StackHandleScope<1> hs(self);
-  Handle<mirror::Object> obj_h(hs.NewHandle(o));
-
   // Call IdentityHashCode here to avoid a lock level violation between lock_ and monitor_lock.
   int32_t identity_hash_code = obj_h->IdentityHashCode();
 
diff --git a/runtime/jdwp/object_registry.h b/runtime/jdwp/object_registry.h
index 27a4e55f41..4c149cdac7 100644
--- a/runtime/jdwp/object_registry.h
+++ b/runtime/jdwp/object_registry.h
@@ -23,6 +23,7 @@
 #include <map>
 
 #include "base/casts.h"
+#include "handle.h"
 #include "jdwp/jdwp.h"
 #include "safe_map.h"
 
@@ -65,11 +66,23 @@ class ObjectRegistry {
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       LOCKS_EXCLUDED(Locks::thread_list_lock_,
                      Locks::thread_suspend_count_lock_);
+
   JDWP::RefTypeId AddRefType(mirror::Class* c)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       LOCKS_EXCLUDED(Locks::thread_list_lock_,
                      Locks::thread_suspend_count_lock_);
 
+  template<class T>
+  JDWP::ObjectId Add(Handle<T> obj_h)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_,
+                     Locks::thread_suspend_count_lock_);
+
+  JDWP::RefTypeId AddRefType(Handle<mirror::Class> c_h)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_,
+                     Locks::thread_suspend_count_lock_);
+
   template<typename T> T Get(JDWP::ObjectId id, JDWP::JdwpError* error)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
     if (id == 0) {
@@ -98,7 +111,8 @@ class ObjectRegistry {
   jobject GetJObject(JDWP::ObjectId id) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
  private:
-  JDWP::ObjectId InternalAdd(mirror::Object* o)
+  template<class T>
+  JDWP::ObjectId InternalAdd(Handle<T> obj_h)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       LOCKS_EXCLUDED(lock_,
                      Locks::thread_list_lock_,