Revert "[vm] Call OSThread::Cleanup() during VM shutdown (as with all other Init/Cleanup functions)"

See b/157883819: Custom embedder doesn't correctly join threads that
interacted with Dart API, which causes us to hit the newly added
RELEASE_ASSERT.

This reverts commit ea4b175.

Change-Id: I9fec45196646f67ae46efccc2f83a43e8941a626
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/149592
Reviewed-by: David Morgan <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>

Author: mkustermann

runtime/vm/dart.cc

@@ -617,7 +617,12 @@ char* Dart::Cleanup() {
   Timeline::Cleanup();
 #endif
   Zone::Cleanup();
-  OSThread::Cleanup();
+  // Delete the current thread's TLS and set it's TLS to null.
+  // If it is the last thread then the destructor would call
+  // OSThread::Cleanup.
+  OSThread* os_thread = OSThread::Current();
+  OSThread::SetCurrent(NULL);
+  delete os_thread;
   if (FLAG_trace_shutdown) {
     OS::PrintErr("[+%" Pd64 "ms] SHUTDOWN: Deleted os_thread\n",
                  UptimeMillis());

runtime/vm/malloc_hooks_test.cc

@@ -25,17 +25,10 @@ static void MallocHookTestBufferInitializer(volatile char* buffer,
   }
 }
 
-// Only to be used in UNIT_TEST_CASE which runs without active VM.
-class OSThreadSupport : public ValueObject {
- public:
-  OSThreadSupport() { OSThread::Init(); }
-
-  ~OSThreadSupport() { OSThread::Cleanup(); }
-};
-
 class EnableMallocHooksScope : public ValueObject {
  public:
   EnableMallocHooksScope() {
+    OSThread::Current();  // Ensure not allocated during test.
     saved_enable_malloc_hooks_ = FLAG_profiler_native_memory;
     FLAG_profiler_native_memory = true;
     MallocHooks::Init();
@@ -73,7 +66,6 @@ class EnableMallocHooksAndStacksScope : public EnableMallocHooksScope {
 };
 
 UNIT_TEST_CASE(BasicMallocHookTest) {
-  OSThreadSupport os_thread_support;
   EnableMallocHooksScope scope;
 
   EXPECT_EQ(0L, MallocHooks::allocation_count());
@@ -92,7 +84,6 @@ UNIT_TEST_CASE(BasicMallocHookTest) {
 }
 
 UNIT_TEST_CASE(FreeUnseenMemoryMallocHookTest) {
-  OSThreadSupport os_thread_support;
   EnableMallocHooksScope scope;
 
   const intptr_t pre_hook_buffer_size = 3;

runtime/vm/os_thread.cc

@@ -134,18 +134,21 @@ bool OSThread::ThreadInterruptsEnabled() {
   return thread_interrupt_disabled_ == 0;
 }
 
-static void DeleteOSThreadTLS(void* thread) {
+static void DeleteThread(void* thread) {
   delete reinterpret_cast<OSThread*>(thread);
 }
 
 void OSThread::Init() {
   // Allocate the global OSThread lock.
-  ASSERT(thread_list_lock_ == nullptr);
-  thread_list_lock_ = new Mutex();
+  if (thread_list_lock_ == NULL) {
+    thread_list_lock_ = new Mutex();
+  }
+  ASSERT(thread_list_lock_ != NULL);
 
   // Create the thread local key.
-  ASSERT(thread_key_ == kUnsetThreadLocalKey);
-  thread_key_ = CreateThreadLocal(DeleteOSThreadTLS);
+  if (thread_key_ == kUnsetThreadLocalKey) {
+    thread_key_ = CreateThreadLocal(DeleteThread);
+  }
   ASSERT(thread_key_ != kUnsetThreadLocalKey);
 
   // Enable creation of OSThread structures in the VM.
@@ -159,25 +162,21 @@ void OSThread::Init() {
 }
 
 void OSThread::Cleanup() {
-  // Delete the current thread's TLS (if any).
-  OSThread* os_thread = OSThread::Current();
-  OSThread::SetCurrent(nullptr);
-  delete os_thread;
-
-  // At this point all OSThread structures should have been deleted.
-  // If not we have a bug in the code where a thread is not correctly joined
-  // before `Dart::Cleanup()`.
-  RELEASE_ASSERT(OSThread::thread_list_head_ == nullptr);
-
-  // Delete the thread local key.
-  ASSERT(thread_key_ != kUnsetThreadLocalKey);
-  DeleteThreadLocal(thread_key_);
-  thread_key_ = kUnsetThreadLocalKey;
-
-  // Delete the global OSThread lock.
-  ASSERT(thread_list_lock_ != nullptr);
-  delete thread_list_lock_;
-  thread_list_lock_ = nullptr;
+// 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);
+    DeleteThreadLocal(thread_key_);
+    thread_key_ = kUnsetThreadLocalKey;
+
+    // Delete the global OSThread lock.
+    ASSERT(thread_list_lock_ != NULL);
+    delete thread_list_lock_;
+    thread_list_lock_ = NULL;
+  }
+#endif
 }
 
 OSThread* OSThread::CreateAndSetUnknownThread() {
@@ -246,6 +245,7 @@ void OSThread::AddThreadToListLocked(OSThread* thread) {
 }
 
 void OSThread::RemoveThreadFromList(OSThread* thread) {
+  bool final_thread = false;
   {
     ASSERT(thread != NULL);
     ASSERT(thread_list_lock_ != NULL);
@@ -263,12 +263,18 @@ void OSThread::RemoveThreadFromList(OSThread* thread) {
           previous->thread_list_next_ = current->thread_list_next_;
         }
         thread->thread_list_next_ = NULL;
+        final_thread = !creation_enabled_ && (thread_list_head_ == NULL);
         break;
       }
       previous = current;
       current = current->thread_list_next_;
     }
   }
+  // Check if this is the last thread. The last thread does a cleanup
+  // which removes the thread local key and the associated mutex.
+  if (final_thread) {
+    Cleanup();
+  }
 }
 
 void OSThread::SetCurrentTLS(BaseThread* value) {

runtime/vm/os_thread.h

@@ -229,7 +229,6 @@ class OSThread : public BaseThread {
 
   // Called at VM startup and shutdown.
   static void Init();
-  static void Cleanup();
 
   static bool IsThreadInList(ThreadId id);
 
@@ -256,6 +255,7 @@ class OSThread : public BaseThread {
   ThreadState* thread() const { return thread_; }
   void set_thread(ThreadState* value) { thread_ = value; }
 
+  static void Cleanup();
 #ifdef SUPPORT_TIMELINE
   static ThreadId GetCurrentThreadTraceId();
 #endif  // PRODUCT
@@ -299,8 +299,11 @@ class OSThread : public BaseThread {
   // protected and should only be read/written by the OSThread itself.
   void* owning_thread_pool_worker_ = nullptr;
 
-  // [thread_list_lock_] cannot have a static lifetime because the order in
-  // which destructors run is undefined.
+  // 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_;

runtime/vm/thread_pool_test.cc

@@ -19,7 +19,12 @@ DECLARE_FLAG(int, worker_timeout_millis);
   UNIT_TEST_CASE(name) {                                                       \
     OSThread::Init();                                                          \
     name##helper();                                                            \
-    OSThread::Cleanup();                                                       \
+    /* Delete the current thread's TLS and set it's TLS to null. */            \
+    /* If it is the last thread then the destructor would call */              \
+    /* OSThread::Cleanup. */                                                   \
+    OSThread* os_thread = OSThread::Current();                                 \
+    OSThread::SetCurrent(nullptr);                                             \
+    delete os_thread;                                                          \
   }                                                                            \
   void name##helper()