Do not handle suspension request in StartGC

Switching back to kRunnable state while appearing to be running a
collection can lead to deadlock. Don't do that.

Unfortunately, StartGC is one of several functions in this area that
can be called with or without the mutator lock, complicating matters.

We implement this by adding a ThreadState value, so we don't need two
clones of TransitionFromRunnableToSuspended. This required trivial
updates to various unrelated switch statements to handle the new case.

Add some checking for kSuspensionImmune abuses. (An earlier attempt
tried to use that here.)

Bug: 304929145
Test: Treehugger
Change-Id: Id5e16fef3255ff243c4e7e306762ababd8cf2f70

11 files changed, 81 insertions, 15 deletions

diff --git a/openjdkjvmti/ti_monitor.cc b/openjdkjvmti/ti_monitor.cc
index 469693d37f..7630b19106 100644
--- a/openjdkjvmti/ti_monitor.cc
+++ b/openjdkjvmti/ti_monitor.cc
@@ -410,7 +410,8 @@ jvmtiError MonitorUtil::GetCurrentContendedMonitor([[maybe_unused]] jvmtiEnv* en
           // We aren't currently (explicitly) waiting for a monitor so just return null.
           return;
         case art::ThreadState::kObsoleteRunnable:
-          LOG(FATAL) << "UNREACHABLE";  // Obsolete value.
+        case art::ThreadState::kInvalidState:
+          LOG(FATAL) << "UNREACHABLE";  // Obsolete or invalid value.
           UNREACHABLE();
         }
       }
diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc
index 324d1ecc1b..6b3cf30aee 100644
--- a/openjdkjvmti/ti_thread.cc
+++ b/openjdkjvmti/ti_thread.cc
@@ -489,6 +489,7 @@ static jint GetJvmtiThreadStateFromInternal(const InternalThreadState& state) {
     case art::ThreadState::kObsoleteRunnable:  // Obsolete value.
     case art::ThreadState::kStarting:
     case art::ThreadState::kTerminated:
+    case art::ThreadState::kInvalidState:
       // We only call this if we are alive so we shouldn't see either of these states.
       LOG(FATAL) << "Should not be in state " << internal_thread_state;
       UNREACHABLE();
@@ -541,7 +542,8 @@ static jint GetJavaStateFromInternal(const InternalThreadState& state) {
       return JVMTI_JAVA_LANG_THREAD_STATE_WAITING;
 
     case art::ThreadState::kObsoleteRunnable:
-      break;  // Obsolete value.
+    case art::ThreadState::kInvalidState:
+      break;  // Obsolete or invalid value.
   }
   LOG(FATAL) << "Unreachable";
   UNREACHABLE();
diff --git a/runtime/gc/collector/garbage_collector.h b/runtime/gc/collector/garbage_collector.h
index ea5acdf202..010ea49c62 100644
--- a/runtime/gc/collector/garbage_collector.h
+++ b/runtime/gc/collector/garbage_collector.h
@@ -147,7 +147,7 @@ class GarbageCollector : public RootVisitor, public IsMarkedVisitor, public Mark
 
  protected:
   // Run all of the GC phases.
-  virtual void RunPhases() = 0;
+  virtual void RunPhases() REQUIRES(!Locks::mutator_lock_) = 0;
   // Revoke all the thread-local buffers.
   virtual void RevokeAllThreadLocalBuffers() = 0;
 
diff --git a/runtime/gc/heap.cc b/runtime/gc/heap.cc
index c1505dd7f5..990bc82249 100644
--- a/runtime/gc/heap.cc
+++ b/runtime/gc/heap.cc
@@ -1638,15 +1638,50 @@ void Heap::TrimIndirectReferenceTables(Thread* self) {
 }
 
 void Heap::StartGC(Thread* self, GcCause cause, CollectorType collector_type) {
-  // Need to do this before acquiring the locks since we don't want to get suspended while
-  // holding any locks.
-  ScopedThreadStateChange tsc(self, ThreadState::kWaitingForGcToComplete);
+  // This can be called in either kRunnable or suspended states.
+  // TODO: Consider fixing that?
+  ThreadState old_thread_state = self->GetState();
+  if (old_thread_state == ThreadState::kRunnable) {
+    Locks::mutator_lock_->AssertSharedHeld(self);
+    // Manually inlining the following call breaks thread-safety analysis.
+    StartGCRunnable(self, cause, collector_type);
+    return;
+  }
+  Locks::mutator_lock_->AssertNotHeld(self);
+  self->SetState(ThreadState::kWaitingForGcToComplete);
   MutexLock mu(self, *gc_complete_lock_);
-  // Ensure there is only one GC at a time.
   WaitForGcToCompleteLocked(cause, self);
   collector_type_running_ = collector_type;
   last_gc_cause_ = cause;
   thread_running_gc_ = self;
+  self->SetState(old_thread_state);
+}
+
+void Heap::StartGCRunnable(Thread* self, GcCause cause, CollectorType collector_type) {
+  Locks::mutator_lock_->AssertSharedHeld(self);
+  while (true) {
+    self->TransitionFromRunnableToSuspended(ThreadState::kWaitingForGcToComplete);
+    {
+      MutexLock mu(self, *gc_complete_lock_);
+      // Ensure there is only one GC at a time.
+      WaitForGcToCompleteLocked(cause, self);
+      collector_type_running_ = collector_type;
+      last_gc_cause_ = cause;
+      thread_running_gc_ = self;
+    }
+    // We have to be careful returning to runnable state, since that could cause us to block.
+    // That would be bad, since collector_type_running_ is set, and hence no GC is possible in this
+    // state, allowing deadlock.
+    if (LIKELY(self->TryTransitionFromSuspendedToRunnable())) {
+      return;
+    }
+    {
+      MutexLock mu(self, *gc_complete_lock_);
+      collector_type_running_ = kCollectorTypeNone;
+      thread_running_gc_ = nullptr;
+    }
+    self->TransitionFromSuspendedToRunnable();  // Will handle suspension request and block.
+  }
 }
 
 void Heap::TrimSpaces(Thread* self) {
@@ -2748,6 +2783,8 @@ collector::GcType Heap::CollectGarbageInternal(collector::GcType gc_type,
     bool compacting_gc;
     {
       gc_complete_lock_->AssertNotHeld(self);
+      // Already not runnable; just switch suspended states. We remain in a suspended state until
+      // FinishGC(). This avoids the complicated dance in StartGC().
       ScopedThreadStateChange tsc2(self, ThreadState::kWaitingForGcToComplete);
       MutexLock mu(self, *gc_complete_lock_);
       // Ensure there is only one GC at a time.
@@ -2847,6 +2884,7 @@ collector::GcType Heap::CollectGarbageInternal(collector::GcType gc_type,
     old_native_bytes_allocated_.store(GetNativeBytes());
     LogGC(gc_cause, collector);
     FinishGC(self, gc_type);
+    // We're suspended up to this point.
   }
   // Actually enqueue all cleared references. Do this after the GC has officially finished since
   // otherwise we can deadlock.
diff --git a/runtime/gc/heap.h b/runtime/gc/heap.h
index 1243f4f59a..0f64655d15 100644
--- a/runtime/gc/heap.h
+++ b/runtime/gc/heap.h
@@ -1050,6 +1050,8 @@ class Heap {
   void LogGC(GcCause gc_cause, collector::GarbageCollector* collector);
   void StartGC(Thread* self, GcCause cause, CollectorType collector_type)
       REQUIRES(!*gc_complete_lock_);
+  void StartGCRunnable(Thread* self, GcCause cause, CollectorType collector_type)
+      REQUIRES(!*gc_complete_lock_) REQUIRES_SHARED(Locks::mutator_lock_);
   void FinishGC(Thread* self, collector::GcType gc_type) REQUIRES(!*gc_complete_lock_);
 
   double CalculateGcWeightedAllocatedBytes(uint64_t gc_last_process_cpu_time_ns,
@@ -1461,7 +1463,7 @@ class Heap {
   // Collector type of the running GC.
   volatile CollectorType collector_type_running_ GUARDED_BY(gc_complete_lock_);
 
-  // Cause of the last running GC.
+  // Cause of the last running or attempted GC or GC-like action.
   volatile GcCause last_gc_cause_ GUARDED_BY(gc_complete_lock_);
 
   // The thread currently running the GC.
diff --git a/runtime/native/java_lang_Thread.cc b/runtime/native/java_lang_Thread.cc
index 65e3009c0f..2c79a38ca0 100644
--- a/runtime/native/java_lang_Thread.cc
+++ b/runtime/native/java_lang_Thread.cc
@@ -108,6 +108,7 @@ static jint Thread_nativeGetStatus(JNIEnv* env, jobject java_thread, jboolean ha
     case ThreadState::kWaitingForGcThreadFlip:         return kJavaWaiting;
     case ThreadState::kNativeForAbort:                 return kJavaWaiting;
     case ThreadState::kSuspended:                      return kJavaRunnable;
+    case ThreadState::kInvalidState:                   break;
     // Don't add a 'default' here so the compiler can spot incompatible enum changes.
   }
   LOG(ERROR) << "Unexpected thread state: " << internal_thread_state;
diff --git a/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc b/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
index 6da4529289..2ba10fa7e3 100644
--- a/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
+++ b/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
@@ -108,7 +108,8 @@ static constexpr uint8_t ToJdwpThreadStatus(ThreadState state) {
     case ThreadState::kSuspended:
       return TS_RUNNING;
     case ThreadState::kObsoleteRunnable:
-      break;  // Obsolete value.
+    case ThreadState::kInvalidState:
+      break;  // Obsolete or invalid value.
     case ThreadState::kSleeping:
       return TS_SLEEPING;
     case ThreadState::kStarting:
diff --git a/runtime/scoped_thread_state_change-inl.h b/runtime/scoped_thread_state_change-inl.h
index f4e2bf996f..ddcb6a3939 100644
--- a/runtime/scoped_thread_state_change-inl.h
+++ b/runtime/scoped_thread_state_change-inl.h
@@ -48,7 +48,7 @@ inline ScopedThreadStateChange::ScopedThreadStateChange(Thread* self, ThreadStat
       } else if (old_thread_state_ == ThreadState::kRunnable) {
         self_->TransitionFromRunnableToSuspended(new_thread_state);
       } else {
-        // A suspended transition to another effectively suspended transition, ok to use Unsafe.
+        // A transition between suspended states.
         self_->SetState(new_thread_state);
       }
     }
@@ -65,7 +65,7 @@ inline ScopedThreadStateChange::~ScopedThreadStateChange() {
       } else if (thread_state_ == ThreadState::kRunnable) {
         self_->TransitionFromRunnableToSuspended(old_thread_state_);
       } else {
-        // A suspended transition to another effectively suspended transition, ok to use Unsafe.
+        // A transition between suspended states.
         self_->SetState(old_thread_state_);
       }
     }
diff --git a/runtime/thread-inl.h b/runtime/thread-inl.h
index 57d606cab6..66771230fe 100644
--- a/runtime/thread-inl.h
+++ b/runtime/thread-inl.h
@@ -335,7 +335,7 @@ inline void Thread::TransitionFromRunnableToSuspended(ThreadState new_state) {
   CheckActiveSuspendBarriers();
 }
 
-inline ThreadState Thread::TransitionFromSuspendedToRunnable() {
+inline ThreadState Thread::TransitionFromSuspendedToRunnable(bool fail_on_suspend_req) {
   // Note: JNI stubs inline a fast path of this method that transitions to Runnable if
   // there are no flags set and then stores the mutator lock to `held_mutexes[kMutatorLock]`
   // (this comes from a specialized `BaseMutex::RegisterAsUnlockedImpl(., kMutatorLock)`
@@ -347,6 +347,7 @@ inline ThreadState Thread::TransitionFromSuspendedToRunnable() {
   ThreadState old_state = old_state_and_flags.GetState();
   DCHECK_NE(old_state, ThreadState::kRunnable);
   while (true) {
+    DCHECK(!old_state_and_flags.IsFlagSet(ThreadFlag::kSuspensionImmune));
     GetMutatorLock()->AssertNotHeld(this);  // Otherwise we starve GC.
     // Optimize for the return from native code case - this is the fast path.
     // Atomically change from suspended to runnable if no suspend request pending.
@@ -374,6 +375,13 @@ inline ThreadState Thread::TransitionFromSuspendedToRunnable() {
                  << " flags=" << old_state_and_flags.WithState(ThreadState::kRunnable).GetValue()
                  << " state=" << old_state_and_flags.GetState();
     } else if (old_state_and_flags.IsFlagSet(ThreadFlag::kSuspendRequest)) {
+      auto fake_mutator_locker = []() SHARED_LOCK_FUNCTION(Locks::mutator_lock_)
+                                     NO_THREAD_SAFETY_ANALYSIS {};
+      if (fail_on_suspend_req) {
+        // Should get here EXTREMELY rarely.
+        fake_mutator_locker();  // We lie to make thread-safety analysis mostly work. See thread.h.
+        return ThreadState::kInvalidState;
+      }
       // Wait while our suspend count is non-zero.
 
       // We pass null to the MutexLock as we may be in a situation where the
diff --git a/runtime/thread.h b/runtime/thread.h
index 8bcd8153ae..a823b93c66 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -159,6 +159,7 @@ enum class ThreadFlag : uint32_t {
   // Prevents a situation in which we are asked to suspend just before we suspend all
   // other threads, and then notice the suspension request and suspend ourselves,
   // leading to deadlock. Guarded by suspend_count_lock_ .
+  // Should not ever be set when we try to transition to kRunnable.
   // TODO(b/296639267): Generalize use to prevent SuspendAll from blocking
   // in-progress GC.
   kSuspensionImmune = 1u << 6,
@@ -483,10 +484,21 @@ class EXPORT Thread {
       REQUIRES(!Locks::thread_suspend_count_lock_)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
-  // Transition from non-runnable to runnable state acquiring share on mutator_lock_.
-  ALWAYS_INLINE ThreadState TransitionFromSuspendedToRunnable()
+  // Transition from non-runnable to runnable state acquiring share on mutator_lock_. Returns the
+  // old state, or kInvalidState if we failed because allow_failure and kSuspensionImmune were set.
+  // Should not be called with an argument except by the next function below.
+  ALWAYS_INLINE ThreadState TransitionFromSuspendedToRunnable(bool fail_on_suspend_req = false)
+      REQUIRES(!Locks::thread_suspend_count_lock_) SHARED_LOCK_FUNCTION(Locks::mutator_lock_);
+
+  // A version that does not return the old ThreadState, and fails by returning false if it would
+  // have needed to handle a pending suspension request.
+  ALWAYS_INLINE bool TryTransitionFromSuspendedToRunnable()
       REQUIRES(!Locks::thread_suspend_count_lock_)
-      SHARED_LOCK_FUNCTION(Locks::mutator_lock_);
+      SHARED_TRYLOCK_FUNCTION(true, Locks::mutator_lock_) NO_THREAD_SAFETY_ANALYSIS {
+    // The above function does not really acquire the lock when we pass true and it returns
+    // kInvalidState. We lie in both places, but clients see correct behavior.
+    return TransitionFromSuspendedToRunnable(true) != ThreadState::kInvalidState;
+  }
 
   // Transition from runnable into a state where mutator privileges are denied. Releases share of
   // mutator lock.
diff --git a/runtime/thread_state.h b/runtime/thread_state.h
index 120f72e396..07574fc967 100644
--- a/runtime/thread_state.h
+++ b/runtime/thread_state.h
@@ -64,6 +64,7 @@ enum class ThreadState : uint8_t {
   kStarting,                        // NEW            TS_WAIT      native thread started, not yet ready to run managed code
   kNative,                          // RUNNABLE       TS_RUNNING   running in a JNI native method
   kSuspended,                       // RUNNABLE       TS_RUNNING   suspended by GC or debugger
+  kInvalidState,                    // Used as error value; never stored.
 };
 EXPORT std::ostream& operator<<(std::ostream& os, ThreadState rhs);