Fix occasional deadlock caused by checkpoint root marking.

There was a race where a new worker thread would attach during the
checkpoint. This caused the thread to wait since suspend count != 0.
But when we decremented the suspend count, we did not broadcast to
the resume condition.

Added a create peer parameter to Thread::Attach and
AttachCurrentThread. This is used by the threadpool since we don't
need a java peer for worker threads.

Change-Id: I632926b5a6b52eeb0684b6e1dcbf3db42ba3d35c

10 files changed, 29 insertions, 14 deletions

diff --git a/src/gc/space.h b/src/gc/space.h
index d816a10..dfbc9d2 100644
--- a/src/gc/space.h
+++ b/src/gc/space.h
@@ -379,7 +379,7 @@ class DlMallocSpace : public MemMapSpace, public AllocSpace {
   static const size_t kChunkOverhead = kWordSize;
 
   // Used to ensure mutual exclusion when the allocation spaces data structures are being modified.
-  Mutex lock_;
+  Mutex lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
 
   // Underlying malloc space
   void* const mspace_;
diff --git a/src/jdwp/jdwp_main.cc b/src/jdwp/jdwp_main.cc
index 064e566..5e368e7 100644
--- a/src/jdwp/jdwp_main.cc
+++ b/src/jdwp/jdwp_main.cc
@@ -284,7 +284,8 @@ static void* StartJdwpThread(void* arg) {
 
 void JdwpState::Run() {
   Runtime* runtime = Runtime::Current();
-  CHECK(runtime->AttachCurrentThread("JDWP", true, runtime->GetSystemThreadGroup()));
+  CHECK(runtime->AttachCurrentThread("JDWP", true, runtime->GetSystemThreadGroup(),
+                                     !runtime->IsCompiler()));
 
   VLOG(jdwp) << "JDWP: thread running";
 
diff --git a/src/jni_internal.cc b/src/jni_internal.cc
index 097d587..8a16bc7 100644
--- a/src/jni_internal.cc
+++ b/src/jni_internal.cc
@@ -383,7 +383,7 @@ static jint JII_AttachCurrentThread(JavaVM* vm, JNIEnv** p_env, void* raw_args,
     thread_group = args->group;
   }
 
-  if (!runtime->AttachCurrentThread(thread_name, as_daemon, thread_group)) {
+  if (!runtime->AttachCurrentThread(thread_name, as_daemon, thread_group, !runtime->IsCompiler())) {
     *p_env = NULL;
     return JNI_ERR;
   } else {
diff --git a/src/runtime.cc b/src/runtime.cc
index 100a198..6dc8435 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -786,9 +786,11 @@ bool Runtime::Init(const Options& raw_options, bool ignore_unrecognized) {
 
   Thread::Startup();
 
-  // 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.
-  Thread* self = Thread::Attach("main", false, NULL);
+  // 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, NULL, false);
+  CHECK_EQ(self->thin_lock_id_, ThreadList::kMainId);
   CHECK(self != NULL);
 
   // Set us to runnable so tools using a runtime can allocate and GC by default
@@ -984,8 +986,9 @@ void Runtime::BlockSignals() {
   signals.Block();
 }
 
-bool Runtime::AttachCurrentThread(const char* thread_name, bool as_daemon, jobject thread_group) {
-  bool success = Thread::Attach(thread_name, as_daemon, thread_group) != NULL;
+bool Runtime::AttachCurrentThread(const char* thread_name, bool as_daemon, jobject thread_group,
+                                  bool create_peer) {
+  bool success = Thread::Attach(thread_name, as_daemon, thread_group, create_peer) != NULL;
   if (thread_name == NULL) {
     LOG(WARNING) << *Thread::Current() << " attached without supplying a name";
   }
diff --git a/src/runtime.h b/src/runtime.h
index 02f5206..94ab0c4 100644
--- a/src/runtime.h
+++ b/src/runtime.h
@@ -163,7 +163,8 @@ class Runtime {
   jobject GetSystemThreadGroup() const;
 
   // Attaches the calling native thread to the runtime.
-  bool AttachCurrentThread(const char* thread_name, bool as_daemon, jobject thread_group);
+  bool AttachCurrentThread(const char* thread_name, bool as_daemon, jobject thread_group,
+                           bool create_peer);
 
   void CallExitHook(jint status);
 
diff --git a/src/signal_catcher.cc b/src/signal_catcher.cc
index b54c819..db0d2c7 100644
--- a/src/signal_catcher.cc
+++ b/src/signal_catcher.cc
@@ -186,7 +186,8 @@ void* SignalCatcher::Run(void* arg) {
   CHECK(signal_catcher != NULL);
 
   Runtime* runtime = Runtime::Current();
-  CHECK(runtime->AttachCurrentThread("Signal Catcher", true, runtime->GetSystemThreadGroup()));
+  CHECK(runtime->AttachCurrentThread("Signal Catcher", true, runtime->GetSystemThreadGroup(),
+                                     !runtime->IsCompiler()));
 
   Thread* self = Thread::Current();
   DCHECK_NE(self->GetState(), kRunnable);
diff --git a/src/thread.cc b/src/thread.cc
index f6e7249..00aaabd 100644
--- a/src/thread.cc
+++ b/src/thread.cc
@@ -308,7 +308,8 @@ void Thread::Init(ThreadList* thread_list, JavaVMExt* java_vm) {
   thread_list->Register(this);
 }
 
-Thread* Thread::Attach(const char* thread_name, bool as_daemon, jobject thread_group) {
+Thread* Thread::Attach(const char* thread_name, bool as_daemon, jobject thread_group,
+                       bool create_peer) {
   Thread* self;
   Runtime* runtime = Runtime::Current();
   if (runtime == NULL) {
@@ -335,7 +336,7 @@ Thread* Thread::Attach(const char* thread_name, bool as_daemon, jobject thread_g
   // so that thread needs a two-stage attach. Regular threads don't need this hack.
   // In the compiler, all threads need this hack, because no-one's going to be getting
   // a native peer!
-  if (self->thin_lock_id_ != ThreadList::kMainId && !Runtime::Current()->IsCompiler()) {
+  if (create_peer) {
     self->CreatePeer(thread_name, as_daemon, thread_group);
   } else {
     // These aren't necessary, but they improve diagnostics for unit tests & command-line tools.
diff --git a/src/thread.h b/src/thread.h
index d281ea2..84b88f2 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -117,7 +117,8 @@ class PACKED 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);
+  static Thread* Attach(const char* thread_name, bool as_daemon, jobject thread_group,
+                        bool create_peer);
 
   // Reset internal state of child thread after fork.
   void InitAfterFork();
diff --git a/src/thread_list.cc b/src/thread_list.cc
index 1a2fd47..84d0054 100644
--- a/src/thread_list.cc
+++ b/src/thread_list.cc
@@ -219,6 +219,13 @@ size_t ThreadList::RunCheckpoint(Closure* checkpoint_function) {
     }
   }
 
+  {
+    // Imitate ResumeAll, threads may be waiting on Thread::resume_cond_ since we raised their
+    // suspend count. Now the suspend_count_ is lowered so we must do the broadcast.
+    MutexLock mu2(self, *Locks::thread_suspend_count_lock_);
+    Thread::resume_cond_->Broadcast(self);
+  }
+
   // Add one for self.
   return count + suspended_count_modified_threads.size() + 1;
 }
diff --git a/src/thread_pool.cc b/src/thread_pool.cc
index 26c83d2..3ded42a 100644
--- a/src/thread_pool.cc
+++ b/src/thread_pool.cc
@@ -35,7 +35,7 @@ void ThreadPoolWorker::Run() {
 void* ThreadPoolWorker::Callback(void* arg) {
   ThreadPoolWorker* worker = reinterpret_cast<ThreadPoolWorker*>(arg);
   Runtime* runtime = Runtime::Current();
-  CHECK(runtime->AttachCurrentThread(worker->name_.c_str(), true, NULL));
+  CHECK(runtime->AttachCurrentThread(worker->name_.c_str(), true, NULL, false));
   // Do work until its time to shut down.
   worker->Run();
   runtime->DetachCurrentThread();