Don't run thread callbacks for runtime threads and shutdown thread

libjdwp tests need an event handler lock to process any events. When
processing certain events like removing the last breakpoint we have to
update entrypoints of the methods while holding the event handler lock.
Deopt manager requires to enter GCCriticalSection when updating
entrypoints. This might prevent us from attaching any threads since
thread callbacks cannot finish. This is generally not an issue except
for heap worker threads and shutdown thread. In these cases we can
create a mutual wait:
DeoptManager blocked on ongoing GC while holding event handler lock
GC waiting on deoptmanager to release event handler lock to attach
threads.

This CL avoids calling thread callbacks for runtime threads (heap / JIT
worker threads) and shutdown thread. This is a temporary fix till we can
avoid using GCCriticalSection in deopt manager.

Bug: 251163712
Bug: 70838465
Test: ART_USE_READ_BARRIER=false run-libjdwp-test.py ObjectReference_DisableCollectionTest

Change-Id: I2f3cb7f0227f88924c299c7e983abb5dc253425d

8 files changed, 56 insertions, 38 deletions

diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 85249de36c..46429e8156 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -338,10 +338,16 @@ Runtime::~Runtime() {
     // In this case we will just try again without allocating a peer so that shutdown can continue.
     // Very few things are actually capable of distinguishing between the peer & peerless states so
     // this should be fine.
+    // Running callbacks is prone to deadlocks in libjdwp tests that need an event handler lock to
+    // process any event. We also need to enter a GCCriticalSection when processing certain events
+    // (for ex: removing the last breakpoint). These two restrictions together make the tear down
+    // of the jdwp tests deadlock prone if we fail to finish Thread::Attach callback.
+    // (TODO:b/251163712) Remove this once we update deopt manager to not use GCCriticalSection.
     bool thread_attached = AttachCurrentThread("Shutdown thread",
                                                /* as_daemon= */ false,
                                                GetSystemThreadGroup(),
-                                               /* create_peer= */ IsStarted());
+                                               /* create_peer= */ IsStarted(),
+                                               /* should_run_callbacks= */ false);
     if (UNLIKELY(!thread_attached)) {
       LOG(WARNING) << "Failed to attach shutdown thread. Trying again without a peer.";
       CHECK(AttachCurrentThread("Shutdown thread (no java peer)",
@@ -441,7 +447,7 @@ Runtime::~Runtime() {
   }
 
   if (attach_shutdown_thread) {
-    DetachCurrentThread();
+    DetachCurrentThread(/* should_run_callbacks= */ false);
     self = nullptr;
   }
 
@@ -1829,7 +1835,7 @@ bool Runtime::Init(RuntimeArgumentMap&& runtime_options_in) {
   // ClassLinker needs an attached thread, but we can't fully attach a thread without creating
   // objects. We can't supply a thread group yet; it will be fixed later. Since we are the main
   // thread, we do not get a java peer.
-  Thread* self = Thread::Attach("main", false, nullptr, false);
+  Thread* self = Thread::Attach("main", false, nullptr, false, /* should_run_callbacks= */ true);
   CHECK_EQ(self->GetThreadId(), ThreadList::kMainThreadId);
   CHECK(self != nullptr);
 
@@ -2430,9 +2436,13 @@ void Runtime::BlockSignals() {
 }
 
 bool Runtime::AttachCurrentThread(const char* thread_name, bool as_daemon, jobject thread_group,
-                                  bool create_peer) {
+                                  bool create_peer, bool should_run_callbacks) {
   ScopedTrace trace(__FUNCTION__);
-  Thread* self = Thread::Attach(thread_name, as_daemon, thread_group, create_peer);
+  Thread* self = Thread::Attach(thread_name,
+                                as_daemon,
+                                thread_group,
+                                create_peer,
+                                should_run_callbacks);
   // Run ThreadGroup.add to notify the group that this thread is now started.
   if (self != nullptr && create_peer && !IsAotCompiler()) {
     ScopedObjectAccess soa(self);
@@ -2441,7 +2451,7 @@ bool Runtime::AttachCurrentThread(const char* thread_name, bool as_daemon, jobje
   return self != nullptr;
 }
 
-void Runtime::DetachCurrentThread() {
+void Runtime::DetachCurrentThread(bool should_run_callbacks) {
   ScopedTrace trace(__FUNCTION__);
   Thread* self = Thread::Current();
   if (self == nullptr) {
@@ -2450,7 +2460,7 @@ void Runtime::DetachCurrentThread() {
   if (self->HasManagedStack()) {
     LOG(FATAL) << *Thread::Current() << " attempting to detach while still running code";
   }
-  thread_list_->Unregister(self);
+  thread_list_->Unregister(self, should_run_callbacks);
 }
 
 mirror::Throwable* Runtime::GetPreAllocatedOutOfMemoryErrorWhenThrowingException() {
diff --git a/runtime/runtime.h b/runtime/runtime.h
index c155489eb1..10ee4aed90 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -291,13 +291,16 @@ class Runtime {
   jobject GetSystemClassLoader() const;
 
   // Attaches the calling native thread to the runtime.
-  bool AttachCurrentThread(const char* thread_name, bool as_daemon, jobject thread_group,
-                           bool create_peer);
+  bool AttachCurrentThread(const char* thread_name,
+                           bool as_daemon,
+                           jobject thread_group,
+                           bool create_peer,
+                           bool should_run_callbacks = true);
 
   void CallExitHook(jint status);
 
   // Detaches the current native thread from the runtime.
-  void DetachCurrentThread() REQUIRES(!Locks::mutator_lock_);
+  void DetachCurrentThread(bool should_run_callbacks = true) REQUIRES(!Locks::mutator_lock_);
 
   void DumpDeoptimizations(std::ostream& os);
   void DumpForSigQuit(std::ostream& os);
diff --git a/runtime/thread.cc b/runtime/thread.cc
index ad6b8c6118..58f1dc1d17 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -669,7 +669,7 @@ void* Thread::CreateCallback(void* arg) {
     InvokeVirtualOrInterfaceWithJValues(soa, ref.get(), mid, nullptr);
   }
   // Detach and delete self.
-  Runtime::Current()->GetThreadList()->Unregister(self);
+  Runtime::Current()->GetThreadList()->Unregister(self, /* should_run_callbacks= */ true);
 
   return nullptr;
 }
@@ -989,7 +989,10 @@ bool Thread::Init(ThreadList* thread_list, JavaVMExt* java_vm, JNIEnvExt* jni_en
 }
 
 template <typename PeerAction>
-Thread* Thread::Attach(const char* thread_name, bool as_daemon, PeerAction peer_action) {
+Thread* Thread::Attach(const char* thread_name,
+                       bool as_daemon,
+                       PeerAction peer_action,
+                       bool should_run_callbacks) {
   Runtime* runtime = Runtime::Current();
   ScopedTrace trace("Thread::Attach");
   if (runtime == nullptr) {
@@ -1024,7 +1027,7 @@ Thread* Thread::Attach(const char* thread_name, bool as_daemon, PeerAction peer_
 
   // Run the action that is acting on the peer.
   if (!peer_action(self)) {
-    runtime->GetThreadList()->Unregister(self);
+    runtime->GetThreadList()->Unregister(self, should_run_callbacks);
     // Unregister deletes self, no need to do this here.
     return nullptr;
   }
@@ -1039,7 +1042,7 @@ Thread* Thread::Attach(const char* thread_name, bool as_daemon, PeerAction peer_
     self->Dump(LOG_STREAM(INFO));
   }
 
-  {
+  if (should_run_callbacks) {
     ScopedObjectAccess soa(self);
     runtime->GetRuntimeCallbacks()->ThreadStart(self);
   }
@@ -1050,7 +1053,8 @@ Thread* Thread::Attach(const char* thread_name, bool as_daemon, PeerAction peer_
 Thread* Thread::Attach(const char* thread_name,
                        bool as_daemon,
                        jobject thread_group,
-                       bool create_peer) {
+                       bool create_peer,
+                       bool should_run_callbacks) {
   auto create_peer_action = [&](Thread* self) {
     // If we're the main thread, ClassLinker won't be created until after we're attached,
     // so that thread needs a two-stage attach. Regular threads don't need this hack.
@@ -1083,7 +1087,7 @@ Thread* Thread::Attach(const char* thread_name,
     }
     return true;
   };
-  return Attach(thread_name, as_daemon, create_peer_action);
+  return Attach(thread_name, as_daemon, create_peer_action, should_run_callbacks);
 }
 
 Thread* Thread::Attach(const char* thread_name, bool as_daemon, jobject thread_peer) {
@@ -1099,7 +1103,7 @@ Thread* Thread::Attach(const char* thread_name, bool as_daemon, jobject thread_p
                                     reinterpret_cast64<jlong>(self));
     return true;
   };
-  return Attach(thread_name, as_daemon, set_peer_action);
+  return Attach(thread_name, as_daemon, set_peer_action, /* should_run_callbacks= */ true);
 }
 
 void Thread::CreatePeer(const char* name, bool as_daemon, jobject thread_group) {
@@ -2540,7 +2544,7 @@ class MonitorExitVisitor : public SingleRootVisitor {
   Thread* const self_;
 };
 
-void Thread::Destroy() {
+void Thread::Destroy(bool should_run_callbacks) {
   Thread* self = this;
   DCHECK_EQ(self, Thread::Current());
 
@@ -2569,7 +2573,7 @@ void Thread::Destroy() {
     HandleUncaughtExceptions(soa);
     RemoveFromThreadGroup(soa);
     Runtime* runtime = Runtime::Current();
-    if (runtime != nullptr) {
+    if (runtime != nullptr && should_run_callbacks) {
       runtime->GetRuntimeCallbacks()->ThreadDeath(self);
     }
 
diff --git a/runtime/thread.h b/runtime/thread.h
index dda4c086f4..bbffaf94b0 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -232,8 +232,11 @@ class Thread {
 
   // Attaches the calling native thread to the runtime, returning the new native peer.
   // Used to implement JNI AttachCurrentThread and AttachCurrentThreadAsDaemon calls.
-  static Thread* Attach(const char* thread_name, bool as_daemon, jobject thread_group,
-                        bool create_peer);
+  static Thread* Attach(const char* thread_name,
+                        bool as_daemon,
+                        jobject thread_group,
+                        bool create_peer,
+                        bool should_run_callbacks);
   // Attaches the calling native thread to the runtime, returning the new native peer.
   static Thread* Attach(const char* thread_name, bool as_daemon, jobject thread_peer);
 
@@ -1466,7 +1469,7 @@ class Thread {
  private:
   explicit Thread(bool daemon);
   ~Thread() REQUIRES(!Locks::mutator_lock_, !Locks::thread_suspend_count_lock_);
-  void Destroy();
+  void Destroy(bool should_run_callbacks);
 
   // Deletes and clears the tlsPtr_.jpeer field. Done in a way so that both it and opeer cannot be
   // observed to be set at the same time by instrumentation.
@@ -1477,7 +1480,8 @@ class Thread {
   template <typename PeerAction>
   static Thread* Attach(const char* thread_name,
                         bool as_daemon,
-                        PeerAction p);
+                        PeerAction p,
+                        bool should_run_callbacks);
 
   void CreatePeer(const char* name, bool as_daemon, jobject thread_group);
 
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index f36e922fa6..d2614a69b5 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -1298,7 +1298,7 @@ void ThreadList::Register(Thread* self) {
   }
 }
 
-void ThreadList::Unregister(Thread* self) {
+void ThreadList::Unregister(Thread* self, bool should_run_callbacks) {
   DCHECK_EQ(self, Thread::Current());
   CHECK_NE(self->GetState(), ThreadState::kRunnable);
   Locks::mutator_lock_->AssertNotHeld(self);
@@ -1319,7 +1319,7 @@ void ThreadList::Unregister(Thread* self) {
   // causes the threads to join. It is important to do this after incrementing unregistering_count_
   // since we want the runtime to wait for the daemon threads to exit before deleting the thread
   // list.
-  self->Destroy();
+  self->Destroy(should_run_callbacks);
 
   // If tracing, remember thread id and name before thread exits.
   Trace::StoreExitingThreadInfo(self);
diff --git a/runtime/thread_list.h b/runtime/thread_list.h
index 17a53fa58d..c1ffe9e59d 100644
--- a/runtime/thread_list.h
+++ b/runtime/thread_list.h
@@ -153,7 +153,7 @@ class ThreadList {
       REQUIRES(!Locks::mutator_lock_,
                !Locks::thread_list_lock_,
                !Locks::thread_suspend_count_lock_);
-  void Unregister(Thread* self)
+  void Unregister(Thread* self, bool should_run_callbacks)
       REQUIRES(!Locks::mutator_lock_,
                !Locks::thread_list_lock_,
                !Locks::thread_suspend_count_lock_);
diff --git a/runtime/thread_pool.cc b/runtime/thread_pool.cc
index b361d16d95..92ac8452ba 100644
--- a/runtime/thread_pool.cc
+++ b/runtime/thread_pool.cc
@@ -119,6 +119,12 @@ void ThreadPoolWorker::Run() {
 void* ThreadPoolWorker::Callback(void* arg) {
   ThreadPoolWorker* worker = reinterpret_cast<ThreadPoolWorker*>(arg);
   Runtime* runtime = Runtime::Current();
+  // Don't run callbacks for ThreadPoolWorkers. These are created for JITThreadPool and
+  // HeapThreadPool and are purely internal threads of the runtime and we don't need to run
+  // callbacks for the thread attach / detach listeners.
+  // (b/251163712) Calling callbacks for heap thread pool workers causes deadlocks in some libjdwp
+  // tests. Deadlocks happen when a GC thread is attached while libjdwp holds the event handler
+  // lock for an event that triggers an entrypoint update from deopt manager.
   CHECK(runtime->AttachCurrentThread(
       worker->name_.c_str(),
       true,
@@ -129,13 +135,14 @@ void* ThreadPoolWorker::Callback(void* arg) {
       // rely on being able to (for example) wait for all threads to finish some task. If debuggers
       // are suspending these threads that might not be possible.
       worker->thread_pool_->create_peers_ ? runtime->GetSystemThreadGroup() : nullptr,
-      worker->thread_pool_->create_peers_));
+      worker->thread_pool_->create_peers_,
+      /* should_run_callbacks= */ false));
   worker->thread_ = Thread::Current();
   // Mark thread pool workers as runtime-threads.
   worker->thread_->SetIsRuntimeThread(true);
   // Do work until its time to shut down.
   worker->Run();
-  runtime->DetachCurrentThread();
+  runtime->DetachCurrentThread(/* should_run_callbacks= */ false);
   return nullptr;
 }
 
diff --git a/tools/external_oj_libjdwp_art_no_read_barrier_failures.txt b/tools/external_oj_libjdwp_art_no_read_barrier_failures.txt
index d60862e999..920b611e43 100644
--- a/tools/external_oj_libjdwp_art_no_read_barrier_failures.txt
+++ b/tools/external_oj_libjdwp_art_no_read_barrier_failures.txt
@@ -6,14 +6,4 @@
  * test groups on the chromium buildbot running without read-barrier.
  */
 [
-{
-  description: "Failing with userfaultfd GC",
-  result: EXEC_FAILED,
-  bug: 242181443,
-  names: [ "org.apache.harmony.jpda.tests.jdwp.ObjectReference_DisableCollectionTest#testDisableCollection001",
-           "org.apache.harmony.jpda.tests.jdwp.ObjectReference_DisableCollectionTest#testDisableCollection_invalid",
-           "org.apache.harmony.jpda.tests.jdwp.ObjectReference_IsCollectedTest#testIsCollected001",
-           "org.apache.harmony.jpda.tests.jdwp.ObjectReference_IsCollectedTest#testIsCollected_invalid",
-           "org.apache.harmony.jpda.tests.jdwp.ObjectReference_IsCollectedTest#testIsCollected_null" ]
-}
 ]