Revert^17 "Thread suspension cleanup and deadlock fix"

This reverts commit c6371b52df0da31acc174a3526274417b7aac0a7.

Reason for revert: This seems to have two remaining issues:

1. The second DCHECK in WaitForFlipFunction is not completely guaranteed to hold, resulting in failures for 658-fp-read-barrier.
2. WaitForSuspendBarrier seems to time out occasionally, possibly spuriously so. We fail when the futex times out once. That's probably incompatible with the app freezer. We should retry a few times.

Change-Id: Ibd8909b31083fc29e6d4f1fcde003d08eb16fc0a

3 files changed, 40 insertions, 21 deletions

diff --git a/runtime/native/dalvik_system_VMStack.cc b/runtime/native/dalvik_system_VMStack.cc
index 596b2eb435..71078c9ad2 100644
--- a/runtime/native/dalvik_system_VMStack.cc
+++ b/runtime/native/dalvik_system_VMStack.cc
@@ -45,30 +45,35 @@ static ResultT GetThreadStack(const ScopedFastNativeObjectAccess& soa,
   ObjPtr<mirror::Object> decoded_peer = soa.Decode<mirror::Object>(peer);
   if (decoded_peer == soa.Self()->GetPeer()) {
     trace = fn(soa.Self(), soa);
-    return trace;
-  }
-  // Suspend thread to build stack trace.
-  ScopedThreadSuspension sts(soa.Self(), ThreadState::kNative);
-  Runtime* runtime = Runtime::Current();
-  ThreadList* thread_list = runtime->GetThreadList();
-  Thread* thread = thread_list->SuspendThreadByPeer(peer, SuspendReason::kInternal);
-  if (thread != nullptr) {
-    // If we were asked for the HeapTaskDaemon's stack trace, we went ahead and suspended it.
-    // It's usually already in a suspended state anyway. But we should immediately give up and
-    // resume it, since we must be able to allocate while generating the stack trace.
-    if (!runtime->GetHeap()->GetTaskProcessor()->IsRunningThread(thread, /*wait=*/true)) {
+  } else {
+    // Never allow suspending the heap task thread since it may deadlock if allocations are
+    // required for the stack trace.
+    Thread* heap_task_thread =
+        Runtime::Current()->GetHeap()->GetTaskProcessor()->GetRunningThread();
+    // heap_task_thread could be null if the daemons aren't yet started.
+    if (heap_task_thread != nullptr && decoded_peer == heap_task_thread->GetPeerFromOtherThread()) {
+      return nullptr;
+    }
+    // Suspend thread to build stack trace.
+    ScopedThreadSuspension sts(soa.Self(), ThreadState::kNative);
+    ThreadList* thread_list = Runtime::Current()->GetThreadList();
+    bool timed_out;
+    Thread* thread = thread_list->SuspendThreadByPeer(peer,
+                                                      SuspendReason::kInternal,
+                                                      &timed_out);
+    if (thread != nullptr) {
+      // Must be runnable to create returned array.
       {
-        // Must be runnable to create returned array.
         ScopedObjectAccess soa2(soa.Self());
         trace = fn(thread, soa);
       }
-      // Else either thread is the HeapTaskDaemon, or we couldn't identify the thread yet. The
-      // HeapTaskDaemon can appear in enumerations before it is registered with the task
-      // processor, and we don't wait indefinitely, so there is a tiny chance of the latter.
+      // Restart suspended thread.
+      bool resumed = thread_list->Resume(thread, SuspendReason::kInternal);
+      DCHECK(resumed);
+    } else if (timed_out) {
+      LOG(ERROR) << "Trying to get thread's stack failed as the thread failed to suspend within a "
+          "generous timeout.";
     }
-    // Restart suspended thread.
-    bool resumed = thread_list->Resume(thread, SuspendReason::kInternal);
-    DCHECK(resumed);
   }
   return trace;
 }
diff --git a/runtime/native/java_lang_Thread.cc b/runtime/native/java_lang_Thread.cc
index fd67a0a7fa..570c554782 100644
--- a/runtime/native/java_lang_Thread.cc
+++ b/runtime/native/java_lang_Thread.cc
@@ -147,8 +147,11 @@ static void Thread_setNativeName(JNIEnv* env, jobject peer, jstring java_name) {
   // thread list lock to avoid this, as setting the thread name causes mutator to lock/unlock
   // in the DDMS send code.
   ThreadList* thread_list = Runtime::Current()->GetThreadList();
+  bool timed_out;
   // Take suspend thread lock to avoid races with threads trying to suspend this one.
-  Thread* thread = thread_list->SuspendThreadByPeer(peer, SuspendReason::kInternal);
+  Thread* thread = thread_list->SuspendThreadByPeer(peer,
+                                                    SuspendReason::kInternal,
+                                                    &timed_out);
   if (thread != nullptr) {
     {
       ScopedObjectAccess soa(env);
@@ -156,6 +159,9 @@ static void Thread_setNativeName(JNIEnv* env, jobject peer, jstring java_name) {
     }
     bool resumed = thread_list->Resume(thread, SuspendReason::kInternal);
     DCHECK(resumed);
+  } else if (timed_out) {
+    LOG(ERROR) << "Trying to set thread name to '" << name.c_str() << "' failed as the thread "
+        "failed to suspend within a generous timeout.";
   }
 }
 
diff --git a/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc b/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
index f20cd281e8..081ec2043a 100644
--- a/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
+++ b/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
@@ -59,6 +59,7 @@ static jobjectArray DdmVmInternal_getStackTraceById(JNIEnv* env, jclass, jint th
     trace = Thread::InternalStackTraceToStackTraceElementArray(soa, internal_trace);
   } else {
     ThreadList* thread_list = Runtime::Current()->GetThreadList();
+    bool timed_out;
 
     // Check for valid thread
     if (thin_lock_id == ThreadList::kInvalidThreadId) {
@@ -66,7 +67,9 @@ static jobjectArray DdmVmInternal_getStackTraceById(JNIEnv* env, jclass, jint th
     }
 
     // Suspend thread to build stack trace.
-    Thread* thread = thread_list->SuspendThreadByThreadId(thin_lock_id, SuspendReason::kInternal);
+    Thread* thread = thread_list->SuspendThreadByThreadId(thin_lock_id,
+                                                          SuspendReason::kInternal,
+                                                          &timed_out);
     if (thread != nullptr) {
       {
         ScopedObjectAccess soa(env);
@@ -76,6 +79,11 @@ static jobjectArray DdmVmInternal_getStackTraceById(JNIEnv* env, jclass, jint th
       // Restart suspended thread.
       bool resumed = thread_list->Resume(thread, SuspendReason::kInternal);
       DCHECK(resumed);
+    } else {
+      if (timed_out) {
+        LOG(ERROR) << "Trying to get thread's stack by id failed as the thread failed to suspend "
+            "within a generous timeout.";
+      }
     }
   }
   return trace;