Fixes for OSThread creation shutdown race.

Also, backs out bad change to the thread pool. The real fix is to
move the disable of OSThread creation until after the thread pool
shuts down.

[email protected]

Review URL: https://codereview.chromium.org/1557033002 .

runtime/vm/dart.cc

@@ -248,15 +248,18 @@ const char* Dart::Cleanup() {
     // before shutting down the thread pool.
     WaitForIsolateShutdown();
 
+    // Shutdown the thread pool. On return, all thread pool threads have exited.
+    delete thread_pool_;
+    thread_pool_ = NULL;
+
     // Disable creation of any new OSThread structures which means no more new
     // threads can do an EnterIsolate. This must come after isolate shutdown
     // because new threads may need to be spawned to shutdown the isolates.
+    // This must come after deletion of the thread pool to avoid a race in which
+    // a thread spawned by the thread pool does not exit through the thread
+    // pool, messing up its bookkeeping.
     OSThread::DisableOSThreadCreation();
 
-    // Shutdown the thread pool. On return, all thread pool threads have exited.
-    delete thread_pool_;
-    thread_pool_ = NULL;
-
     // Set the VM isolate as current isolate.
     bool result = Thread::EnterIsolate(vm_isolate_);
     ASSERT(result);

runtime/vm/os_thread.cc

@@ -36,18 +36,14 @@ OSThread::OSThread() :
 
 
 OSThread* OSThread::CreateOSThread() {
-  if (thread_list_lock_ == NULL) {
+  ASSERT(thread_list_lock_ != NULL);
+  MutexLocker ml(thread_list_lock_);
+  if (!creation_enabled_) {
     return NULL;
   }
-  {
-    MutexLocker ml(thread_list_lock_);
-    if (!creation_enabled_) {
-      return NULL;
-    }
-    OSThread* os_thread = new OSThread();
-    AddThreadToListLocked(os_thread);
-    return os_thread;
-  }
+  OSThread* os_thread = new OSThread();
+  AddThreadToListLocked(os_thread);
+  return os_thread;
 }
 
 
@@ -120,6 +116,9 @@ void OSThread::InitOnce() {
 
 
 void OSThread::Cleanup() {
+  // We cannot delete the thread local key and thread list lock,  yet.
+  // See the note on thread_list_lock_ in os_thread.h.
+#if 0
   if (thread_list_lock_ != NULL) {
     // Delete the thread local key.
     ASSERT(thread_key_ != kUnsetThreadLocalKey);
@@ -131,6 +130,7 @@ void OSThread::Cleanup() {
     delete thread_list_lock_;
     thread_list_lock_ = NULL;
   }
+#endif
 }
 
 

runtime/vm/os_thread.h

@@ -228,8 +228,13 @@ class OSThread : public BaseThread {
   uword stack_base_;
   Thread* thread_;
 
-  static OSThread* thread_list_head_;
+  // thread_list_lock_ cannot have a static lifetime because the order in which
+  // destructors run is undefined. At the moment this lock cannot be deleted
+  // either since otherwise, if a thread only begins to run after we have
+  // started to run TLS destructors for a call to exit(), there will be a race
+  // on its deletion in CreateOSThread().
   static Mutex* thread_list_lock_;
+  static OSThread* thread_list_head_;
   static bool creation_enabled_;
 
   friend class OSThreadIterator;

runtime/vm/thread_pool.cc

@@ -107,16 +107,10 @@ void ThreadPool::Shutdown() {
     while (current != NULL) {
       Worker* next = current->all_next_;
       ThreadId currentId = current->id();
-      ASSERT(id != currentId);
-      if (currentId == OSThread::kInvalidThreadId) {
-        // If the thread id is invalid, it means the thread never started
-        // because OSThread creation was disabled. Destroy the Task and Worker.
-        delete current->task_;
-        delete current;
-      } else {
+      if (currentId != id) {
         AddWorkerToShutdownList(current);
-        current->Shutdown();
       }
+      current->Shutdown();
       current = next;
     }
     saved = NULL;