From c1a3aa38f8ed156c9a18102f3b816a638585072e Mon Sep 17 00:00:00 2001
From: Tom Poole <thomas.poole@roli.com>
Date: Fri, 1 Dec 2017 11:52:05 +0000
Subject: [PATCH] Fixed a data race in the Thread destructor

---
 .../juce_core/native/juce_posix_SharedCode.h   |  8 ++++----
 .../juce_core/native/juce_win32_Threads.cpp    |  6 +++---
 modules/juce_core/threads/juce_Thread.cpp      | 18 +++++++++++-------
 modules/juce_core/threads/juce_Thread.h        |  6 +++---
 4 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/modules/juce_core/native/juce_posix_SharedCode.h b/modules/juce_core/native/juce_posix_SharedCode.h
index 6f1257e7d7..2360847d77 100644
--- a/modules/juce_core/native/juce_posix_SharedCode.h
+++ b/modules/juce_core/native/juce_posix_SharedCode.h
@@ -944,7 +944,7 @@ void Thread::launchThread()
     {
        #if JUCE_ANDROID_REALTIME_THREAD_AVAILABLE
         threadHandle = (void*) juce_createRealtimeAudioThread (threadEntryProc, this);
-        threadId = (ThreadID) threadHandle;
+        threadId = (ThreadID) threadHandle.get();
 
         return;
        #else
@@ -969,7 +969,7 @@ void Thread::launchThread()
     {
         pthread_detach (handle);
         threadHandle = (void*) handle;
-        threadId = (ThreadID) threadHandle;
+        threadId = (ThreadID) threadHandle.get();
     }
 
     if (attrPtr != nullptr)
@@ -984,12 +984,12 @@ void Thread::closeThreadHandle()
 
 void Thread::killThread()
 {
-    if (threadHandle != 0)
+    if (threadHandle.get() != 0)
     {
        #if JUCE_ANDROID
         jassertfalse; // pthread_cancel not available!
        #else
-        pthread_cancel ((pthread_t) threadHandle);
+        pthread_cancel ((pthread_t) threadHandle.get());
        #endif
     }
 }
diff --git a/modules/juce_core/native/juce_win32_Threads.cpp b/modules/juce_core/native/juce_win32_Threads.cpp
index 1eaa837317..4a95dbdea0 100644
--- a/modules/juce_core/native/juce_win32_Threads.cpp
+++ b/modules/juce_core/native/juce_win32_Threads.cpp
@@ -87,19 +87,19 @@ void Thread::launchThread()
 
 void Thread::closeThreadHandle()
 {
-    CloseHandle ((HANDLE) threadHandle);
+    CloseHandle ((HANDLE) threadHandle.get());
     threadId = 0;
     threadHandle = 0;
 }
 
 void Thread::killThread()
 {
-    if (threadHandle != 0)
+    if (threadHandle.get() != 0)
     {
        #if JUCE_DEBUG
         OutputDebugStringA ("** Warning - Forced thread termination **\n");
        #endif
-        TerminateThread (threadHandle, 0);
+        TerminateThread (threadHandle.get(), 0);
     }
 }
 
diff --git a/modules/juce_core/threads/juce_Thread.cpp b/modules/juce_core/threads/juce_Thread.cpp
index 07580f73bf..85f6e3d870 100644
--- a/modules/juce_core/threads/juce_Thread.cpp
+++ b/modules/juce_core/threads/juce_Thread.cpp
@@ -86,7 +86,7 @@ void Thread::threadEntryPoint()
 
     if (startSuspensionEvent.wait (10000))
     {
-        jassert (getCurrentThreadId() == threadId);
+        jassert (getCurrentThreadId() == threadId.get());
 
         if (affinityMask != 0)
             setCurrentThreadAffinityMask (affinityMask);
@@ -102,9 +102,13 @@ void Thread::threadEntryPoint()
     }
 
     currentThreadHolder->value.releaseCurrentThreadStorage();
+
+    // Once closeThreadHandle is called this class may be deleted by a different
+    // thread, so we need to store deleteOnThreadEnd in a local variable.
+    auto shouldDeleteThis = deleteOnThreadEnd;
     closeThreadHandle();
 
-    if (deleteOnThreadEnd)
+    if (shouldDeleteThis)
         delete this;
 }
 
@@ -121,10 +125,10 @@ void Thread::startThread()
 
     shouldExit = false;
 
-    if (threadHandle == nullptr)
+    if (threadHandle.get() == nullptr)
     {
         launchThread();
-        setThreadPriority (threadHandle, threadPriority);
+        setThreadPriority (threadHandle.get(), threadPriority);
         startSuspensionEvent.signal();
     }
 }
@@ -133,7 +137,7 @@ void Thread::startThread (int priority)
 {
     const ScopedLock sl (startStopLock);
 
-    if (threadHandle == nullptr)
+    if (threadHandle.get() == nullptr)
     {
         auto isRealtime = (priority == realtimeAudioPriority);
 
@@ -155,7 +159,7 @@ void Thread::startThread (int priority)
 
 bool Thread::isThreadRunning() const
 {
-    return threadHandle != nullptr;
+    return threadHandle.get() != nullptr;
 }
 
 Thread* JUCE_CALLTYPE Thread::getCurrentThread()
@@ -263,7 +267,7 @@ bool Thread::setPriority (int newPriority)
     isAndroidRealtimeThread = isRealtime;
    #endif
 
-    if ((! isThreadRunning()) || setThreadPriority (threadHandle, newPriority))
+    if ((! isThreadRunning()) || setThreadPriority (threadHandle.get(), newPriority))
     {
         threadPriority = newPriority;
         return true;
diff --git a/modules/juce_core/threads/juce_Thread.h b/modules/juce_core/threads/juce_Thread.h
index 8799b1ee95..3dd8761b8f 100644
--- a/modules/juce_core/threads/juce_Thread.h
+++ b/modules/juce_core/threads/juce_Thread.h
@@ -309,7 +309,7 @@ public:
         thread's not actually running.
         @see getCurrentThreadId
     */
-    ThreadID getThreadId() const noexcept                           { return threadId; }
+    ThreadID getThreadId() const noexcept                           { return threadId.get(); }
 
     /** Returns the name of the thread.
         This is the name that gets set in the constructor.
@@ -325,8 +325,8 @@ public:
 private:
     //==============================================================================
     const String threadName;
-    void* volatile threadHandle = nullptr;
-    ThreadID threadId = {};
+    Atomic<void*> threadHandle { nullptr };
+    Atomic<ThreadID> threadId = {};
     CriticalSection startStopLock;
     WaitableEvent startSuspensionEvent, defaultEvent;
     int threadPriority = 5;