Fix thread-interrupter shutdown on Windows.

This reworks the ThreadId on Windows to be the tid, and not an open HANDLE.

BUG=
R=johnmccutchan@google.com

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

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@36502 260f80e4-7a28-3924-810f-c04153c831b5

runtime/platform/thread.h

@@ -42,6 +42,7 @@ class Thread {
   static void SetThreadLocal(ThreadLocalKey key, uword value);
   static intptr_t GetMaxStackSize();
   static ThreadId GetCurrentThreadId();
+  static bool Join(ThreadId id);
   static intptr_t ThreadIdToIntPtr(ThreadId id);
   static bool Compare(ThreadId a, ThreadId b);
   static void GetThreadCpuUsage(ThreadId thread_id, int64_t* cpu_usage);

runtime/platform/thread_android.cc

@@ -149,6 +149,11 @@ ThreadId Thread::GetCurrentThreadId() {
 }
 
 
+bool Thread::Join(ThreadId id) {
+  return false;
+}
+
+
 intptr_t Thread::ThreadIdToIntPtr(ThreadId id) {
   ASSERT(sizeof(id) == sizeof(intptr_t));
   return static_cast<intptr_t>(id);

runtime/platform/thread_linux.cc

@@ -150,6 +150,11 @@ ThreadId Thread::GetCurrentThreadId() {
 }
 
 
+bool Thread::Join(ThreadId id) {
+  return false;
+}
+
+
 intptr_t Thread::ThreadIdToIntPtr(ThreadId id) {
   ASSERT(sizeof(id) == sizeof(intptr_t));
   return static_cast<intptr_t>(id);

runtime/platform/thread_macos.cc

@@ -142,6 +142,11 @@ ThreadId Thread::GetCurrentThreadId() {
 }
 
 
+bool Thread::Join(ThreadId id) {
+  return false;
+}
+
+
 intptr_t Thread::ThreadIdToIntPtr(ThreadId id) {
   ASSERT(sizeof(id) == sizeof(intptr_t));
   return reinterpret_cast<intptr_t>(id);

runtime/platform/thread_win.cc

@@ -33,18 +33,12 @@ class ThreadStartData {
 // is used to ensure that the thread is properly destroyed if the thread just
 // exits.
 static unsigned int __stdcall ThreadEntry(void* data_ptr) {
-  ThreadStartData* data =  reinterpret_cast<ThreadStartData*>(data_ptr);
+  ThreadStartData* data = reinterpret_cast<ThreadStartData*>(data_ptr);
 
   Thread::ThreadStartFunction function = data->function();
   uword parameter = data->parameter();
   delete data;
 
-  ASSERT(ThreadInlineImpl::thread_id_key != Thread::kUnsetThreadLocalKey);
-
-  ThreadId thread_id = ThreadInlineImpl::CreateThreadId();
-  // Set thread ID in TLS.
-  Thread::SetThreadLocal(ThreadInlineImpl::thread_id_key,
-                         reinterpret_cast<DWORD>(thread_id));
   MonitorData::GetMonitorWaitDataForThread();
 
   // Call the supplied thread start function handing it its parameters.
@@ -53,10 +47,6 @@ static unsigned int __stdcall ThreadEntry(void* data_ptr) {
   // Clean up the monitor wait data for this thread.
   MonitorWaitData::ThreadExit();
 
-  // Clear thread ID in TLS.
-  Thread::SetThreadLocal(ThreadInlineImpl::thread_id_key, NULL);
-  ThreadInlineImpl::DestroyThreadId(thread_id);
-
   return 0;
 }
 
@@ -73,34 +63,14 @@ int Thread::Start(ThreadStartFunction function, uword parameter) {
     return errno;
   }
 
+  // Close the handle, so we don't leak the thread object.
+  CloseHandle(reinterpret_cast<HANDLE>(thread));
+
   return 0;
 }
 
-
-ThreadId ThreadInlineImpl::CreateThreadId() {
-  // Create an ID for this thread that can be shared with other threads.
-  HANDLE thread_id = OpenThread(THREAD_GET_CONTEXT |
-                                THREAD_SUSPEND_RESUME |
-                                THREAD_QUERY_INFORMATION,
-                                false,
-                                GetCurrentThreadId());
-  ASSERT(thread_id != NULL);
-  return thread_id;
-}
-
-
-void ThreadInlineImpl::DestroyThreadId(ThreadId thread_id) {
-  ASSERT(thread_id != NULL);
-  // Destroy thread ID.
-  CloseHandle(thread_id);
-}
-
-
-ThreadLocalKey ThreadInlineImpl::thread_id_key = Thread::kUnsetThreadLocalKey;
-
 ThreadLocalKey Thread::kUnsetThreadLocalKey = TLS_OUT_OF_INDEXES;
-ThreadId Thread::kInvalidThreadId =
-    reinterpret_cast<ThreadId>(INVALID_HANDLE_VALUE);
+ThreadId Thread::kInvalidThreadId = 0;
 
 ThreadLocalKey Thread::CreateThreadLocal() {
   ThreadLocalKey key = TlsAlloc();
@@ -127,16 +97,24 @@ intptr_t Thread::GetMaxStackSize() {
 
 
 ThreadId Thread::GetCurrentThreadId() {
-  ThreadId id = reinterpret_cast<ThreadId>(
-      Thread::GetThreadLocal(ThreadInlineImpl::thread_id_key));
-  ASSERT(id != NULL);
-  return id;
+  return ::GetCurrentThreadId();
+}
+
+
+bool Thread::Join(ThreadId id) {
+  HANDLE handle = OpenThread(SYNCHRONIZE, false, id);
+  if (handle == INVALID_HANDLE_VALUE) {
+    return false;
+  }
+  DWORD res = WaitForSingleObject(handle, INFINITE);
+  CloseHandle(handle);
+  return res == WAIT_OBJECT_0;
 }
 
 
 intptr_t Thread::ThreadIdToIntPtr(ThreadId id) {
-  ASSERT(sizeof(id) == sizeof(intptr_t));
-  return reinterpret_cast<intptr_t>(id);
+  ASSERT(sizeof(id) <= sizeof(intptr_t));
+  return static_cast<intptr_t>(id);
 }
 
 
@@ -163,11 +141,13 @@ void Thread::GetThreadCpuUsage(ThreadId thread_id, int64_t* cpu_usage) {
   TimeStamp exited;
   TimeStamp kernel;
   TimeStamp user;
-  BOOL result = GetThreadTimes(thread_id,
+  HANDLE handle = OpenThread(THREAD_QUERY_INFORMATION, false, thread_id);
+  BOOL result = GetThreadTimes(handle,
                                &created.ft_,
                                &exited.ft_,
                                &kernel.ft_,
                                &user.ft_);
+  CloseHandle(handle);
   if (!result) {
     FATAL1("GetThreadCpuUsage failed %d\n", GetLastError());
   }

runtime/platform/thread_win.h

@@ -15,7 +15,7 @@
 namespace dart {
 
 typedef DWORD ThreadLocalKey;
-typedef HANDLE ThreadId;
+typedef DWORD ThreadId;
 
 
 class ThreadInlineImpl {
@@ -23,9 +23,6 @@ class ThreadInlineImpl {
   ThreadInlineImpl() {}
   ~ThreadInlineImpl() {}
 
-  static ThreadLocalKey thread_id_key;
-  static ThreadId CreateThreadId();
-  static void DestroyThreadId(ThreadId);
   static uword GetThreadLocal(ThreadLocalKey key) {
     static ThreadLocalKey kUnsetThreadLocalKey = TLS_OUT_OF_INDEXES;
     ASSERT(key != kUnsetThreadLocalKey);
@@ -33,7 +30,6 @@ class ThreadInlineImpl {
   }
 
   friend class Thread;
-  friend class OS;
   friend unsigned int __stdcall ThreadEntry(void* data_ptr);
 
   DISALLOW_ALLOCATION();

runtime/vm/os_win.cc

@@ -316,12 +316,8 @@ void OS::InitOnce() {
   init_once_called = true;
   // Do not pop up a message box when abort is called.
   _set_abort_behavior(0, _WRITE_ABORT_MSG);
-  ThreadInlineImpl::thread_id_key = Thread::CreateThreadLocal();
   MonitorWaitData::monitor_wait_data_key_ = Thread::CreateThreadLocal();
   MonitorData::GetMonitorWaitDataForThread();
-  ThreadId thread_id = ThreadInlineImpl::CreateThreadId();
-  Thread::SetThreadLocal(ThreadInlineImpl::thread_id_key,
-                         reinterpret_cast<DWORD>(thread_id));
 }
 
 

runtime/vm/profiler.cc

@@ -21,8 +21,7 @@
 namespace dart {
 
 
-#if defined(USING_SIMULATOR) || defined(TARGET_OS_WINDOWS) || \
-    defined(TARGET_OS_ANDROID)
+#if defined(USING_SIMULATOR) || defined(TARGET_OS_ANDROID)
   DEFINE_FLAG(bool, profile, false, "Enable Sampling Profiler");
 #else
   DEFINE_FLAG(bool, profile, true, "Enable Sampling Profiler");

runtime/vm/thread_interrupter.cc

@@ -86,25 +86,28 @@ void ThreadInterrupter::Startup() {
 
 
 void ThreadInterrupter::Shutdown() {
-  if (shutdown_) {
-    // Already shutdown.
-    return;
-  }
-  ASSERT(initialized_);
-  if (FLAG_trace_thread_interrupter) {
-    OS::Print("ThreadInterrupter shutting down.\n");
-  }
-  {
-    MonitorLocker ml(monitor_);
-    shutdown_ = true;
-  }
   {
     MonitorLocker shutdown_ml(monitor_);
+    if (shutdown_) {
+      // Already shutdown.
+      return;
+    }
+    shutdown_ = true;
+    ASSERT(initialized_);
+    if (FLAG_trace_thread_interrupter) {
+      OS::Print("ThreadInterrupter shutting down.\n");
+    }
     while (thread_running_) {
       shutdown_ml.Wait();
     }
+    // Join in the interrupter thread. On Windows, a thread's exit-code can
+    // leak into the process's exit-code, if exiting 'at same time' as the
+    // process ends.
+    if (interrupter_thread_id_ != Thread::kInvalidThreadId) {
+      Thread::Join(interrupter_thread_id_);
+      interrupter_thread_id_ = Thread::kInvalidThreadId;
+    }
   }
-  interrupter_thread_id_ = Thread::kInvalidThreadId;
   if (FLAG_trace_thread_interrupter) {
     OS::Print("ThreadInterrupter shut down.\n");
   }
@@ -234,8 +237,8 @@ void ThreadInterrupter::ThreadMain(uword parameters) {
   {
     // Signal to main thread we are ready.
     MonitorLocker startup_ml(monitor_);
-    thread_running_ = true;
     interrupter_thread_id_ = Thread::GetCurrentThreadId();
+    thread_running_ = true;
     startup_ml.Notify();
   }
   {

runtime/vm/thread_interrupter_win.cc

@@ -16,11 +16,11 @@ DECLARE_FLAG(bool, trace_thread_interrupter);
 
 class ThreadInterrupterWin : public AllStatic {
  public:
-  static bool GrabRegisters(ThreadId thread, InterruptedThreadState* state) {
+  static bool GrabRegisters(HANDLE handle, InterruptedThreadState* state) {
     CONTEXT context;
     memset(&context, 0, sizeof(context));
     context.ContextFlags = CONTEXT_FULL;
-    if (GetThreadContext(thread, &context) != 0) {
+    if (GetThreadContext(handle, &context) != 0) {
 #if defined(TARGET_ARCH_IA32)
       state->pc = static_cast<uintptr_t>(context.Eip);
       state->fp = static_cast<uintptr_t>(context.Ebp);
@@ -39,33 +39,42 @@ class ThreadInterrupterWin : public AllStatic {
 
 
   static void Interrupt(InterruptableThreadState* state) {
-    ASSERT(GetCurrentThread() != state->id);
-    DWORD result = SuspendThread(state->id);
+    ASSERT(!Thread::Compare(GetCurrentThreadId(), state->id));
+    HANDLE handle = OpenThread(THREAD_GET_CONTEXT |
+                               THREAD_SUSPEND_RESUME,
+                               false,
+                               state->id);
+    ASSERT(handle != NULL);
+    DWORD result = SuspendThread(handle);
     if (result == kThreadError) {
       if (FLAG_trace_thread_interrupter) {
         OS::Print("ThreadInterrupted failed to suspend thread %p\n",
                   reinterpret_cast<void*>(state->id));
       }
+      CloseHandle(handle);
       return;
     }
     InterruptedThreadState its;
     its.tid = state->id;
-    if (!GrabRegisters(state->id, &its)) {
+    if (!GrabRegisters(handle, &its)) {
       // Failed to get thread registers.
-      ResumeThread(state->id);
+      ResumeThread(handle);
       if (FLAG_trace_thread_interrupter) {
         OS::Print("ThreadInterrupted failed to get registers for %p\n",
                   reinterpret_cast<void*>(state->id));
       }
+      CloseHandle(handle);
       return;
     }
     if (state->callback == NULL) {
       // No callback registered.
-      ResumeThread(state->id);
+      ResumeThread(handle);
+      CloseHandle(handle);
       return;
     }
     state->callback(its, state->data);
-    ResumeThread(state->id);
+    ResumeThread(handle);
+    CloseHandle(handle);
   }
 };