Do not hold breakpoint lock when running the verifier

When setting a breakpoint, we need to know whether the method may be
inlined. We run the method verifier but that may cause thread
suspension. Therefore we must not hold any lock at this time. The
issue is we do hold the breakpoint lock so we fails a check in debug
mode.

This CL ensures we don't hold the breakpoint lock when running the
method verifier to detect inlining.

Bug: 17562442
Change-Id: Ia6b128fc8917ce00025b68ae4ac62fb2a1f154e6
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index aced954..57487ea 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -3125,6 +3125,8 @@
     // should never be null. We could just check we never encounter this case.
     return false;
   }
+  // Note: method verifier may cause thread suspension.
+  self->AssertThreadSuspensionIsAllowable();
   StackHandleScope<3> hs(self);
   mirror::Class* declaring_class = m->GetDeclaringClass();
   Handle<mirror::DexCache> dex_cache(hs.NewHandle(declaring_class->GetDexCache()));
@@ -3150,20 +3152,18 @@
 // Sanity checks all existing breakpoints on the same method.
 static void SanityCheckExistingBreakpoints(mirror::ArtMethod* m, bool need_full_deoptimization)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_, Locks::breakpoint_lock_) {
-  if (kIsDebugBuild) {
-    for (const Breakpoint& breakpoint : gBreakpoints) {
-      CHECK_EQ(need_full_deoptimization, breakpoint.NeedFullDeoptimization());
-    }
-    if (need_full_deoptimization) {
-      // We should have deoptimized everything but not "selectively" deoptimized this method.
-      CHECK(Runtime::Current()->GetInstrumentation()->AreAllMethodsDeoptimized());
-      CHECK(!Runtime::Current()->GetInstrumentation()->IsDeoptimized(m));
-    } else {
-      // We should have "selectively" deoptimized this method.
-      // Note: while we have not deoptimized everything for this method, we may have done it for
-      // another event.
-      CHECK(Runtime::Current()->GetInstrumentation()->IsDeoptimized(m));
-    }
+  for (const Breakpoint& breakpoint : gBreakpoints) {
+    CHECK_EQ(need_full_deoptimization, breakpoint.NeedFullDeoptimization());
+  }
+  if (need_full_deoptimization) {
+    // We should have deoptimized everything but not "selectively" deoptimized this method.
+    CHECK(Runtime::Current()->GetInstrumentation()->AreAllMethodsDeoptimized());
+    CHECK(!Runtime::Current()->GetInstrumentation()->IsDeoptimized(m));
+  } else {
+    // We should have "selectively" deoptimized this method.
+    // Note: while we have not deoptimized everything for this method, we may have done it for
+    // another event.
+    CHECK(Runtime::Current()->GetInstrumentation()->IsDeoptimized(m));
   }
 }
 
@@ -3174,12 +3174,17 @@
   mirror::ArtMethod* m = FromMethodId(location->method_id);
   DCHECK(m != nullptr) << "No method for method id " << location->method_id;
 
-  WriterMutexLock mu(self, *Locks::breakpoint_lock_);
-  const Breakpoint* const existing_breakpoint = FindFirstBreakpointForMethod(m);
+  const Breakpoint* existing_breakpoint;
+  {
+    ReaderMutexLock mu(self, *Locks::breakpoint_lock_);
+    existing_breakpoint = FindFirstBreakpointForMethod(m);
+  }
   bool need_full_deoptimization;
   if (existing_breakpoint == nullptr) {
     // There is no breakpoint on this method yet: we need to deoptimize. If this method may be
     // inlined, we deoptimize everything; otherwise we deoptimize only this method.
+    // Note: IsMethodPossiblyInlined goes into the method verifier and may cause thread suspension.
+    // Therefore we must not hold any lock when we call it.
     need_full_deoptimization = IsMethodPossiblyInlined(self, m);
     if (need_full_deoptimization) {
       req->SetKind(DeoptimizationRequest::kFullDeoptimization);
@@ -3194,12 +3199,18 @@
     req->SetMethod(nullptr);
 
     need_full_deoptimization = existing_breakpoint->NeedFullDeoptimization();
-    SanityCheckExistingBreakpoints(m, need_full_deoptimization);
+    if (kIsDebugBuild) {
+      ReaderMutexLock mu(self, *Locks::breakpoint_lock_);
+      SanityCheckExistingBreakpoints(m, need_full_deoptimization);
+    }
   }
 
-  gBreakpoints.push_back(Breakpoint(m, location->dex_pc, need_full_deoptimization));
-  VLOG(jdwp) << "Set breakpoint #" << (gBreakpoints.size() - 1) << ": "
-             << gBreakpoints[gBreakpoints.size() - 1];
+  {
+    WriterMutexLock mu(self, *Locks::breakpoint_lock_);
+    gBreakpoints.push_back(Breakpoint(m, location->dex_pc, need_full_deoptimization));
+    VLOG(jdwp) << "Set breakpoint #" << (gBreakpoints.size() - 1) << ": "
+               << gBreakpoints[gBreakpoints.size() - 1];
+  }
 }
 
 // Uninstalls a breakpoint at the specified location. Also indicates through the deoptimization
@@ -3234,7 +3245,9 @@
     // There is at least one breakpoint for this method: we don't need to undeoptimize.
     req->SetKind(DeoptimizationRequest::kNothing);
     req->SetMethod(nullptr);
-    SanityCheckExistingBreakpoints(m, need_full_deoptimization);
+    if (kIsDebugBuild) {
+      SanityCheckExistingBreakpoints(m, need_full_deoptimization);
+    }
   }
 }