Thread fixes for shutdown.

runtime/vm/os_thread.cc

Index: runtime/vm/os_thread.cc
diff --git a/runtime/vm/os_thread.cc b/runtime/vm/os_thread.cc
index 766bbbed97d9f46800e24be459df8d8e0f2b6fe0..f6a7498280a6a2545b49266a08391066d99aced8 100644
--- a/runtime/vm/os_thread.cc
+++ b/runtime/vm/os_thread.cc
@@ -36,14 +36,18 @@ OSThread::OSThread() :
 
 
 OSThread* OSThread::CreateOSThread() {
-  ASSERT(thread_list_lock_ != NULL);
-  MutexLocker ml(thread_list_lock_);
-  if (!creation_enabled_) {
+  if (thread_list_lock_ == NULL) {
     return NULL;
   }
-  OSThread* os_thread = new OSThread();
-  AddThreadToListLocked(os_thread);
-  return os_thread;
+  {
+    MutexLocker ml(thread_list_lock_);

[Vyacheslav Egorov (Google), 2016/01/02 16:41:30]

Code like this is a red-flag in multithreaded environment for two reasons:

1. You fetch a value twice and expect that it does not change in between but
there is no lock spanning these two accesses. Depending on how compiler compiles
this code you might end up with a race.

2. Manual managed lifetime of the mutex is bad. Mutex might be freed by another
thread right when we try to acquire it, so even rewriting the code like this:

Mutex* lock = thread_list_lock_;
if (lock == NULL) return NULL;
MutexLocker ml(lock);

guarantees *nothing*. 

Global locks should usually have static lifetime to guarantee correctness. 

[zra, 2016/01/04 17:19:10]

Acknowledged.

+    if (!creation_enabled_) {
+      return NULL;
+    }
+    OSThread* os_thread = new OSThread();
+    AddThreadToListLocked(os_thread);
+    return os_thread;
+  }
 }