Bug 1406421 - Fix js::Thread race on windows (r=till)
authorLuke Wagner <luke@mozilla.com>
Tue, 17 Oct 2017 08:52:40 -0500
changeset 386681 47fc470d7a9376d8519e15935b362dbd0b7bce1c
parent 386680 ba08f77ec79c235805c606e8963121262bf247bc
child 386682 712aa74b91cef65b961b37fd7fa0dbc71a4672f4
push id32699
push userarchaeopteryx@coole-files.de
push dateTue, 17 Oct 2017 21:52:51 +0000
treeherdermozilla-central@f78d59473334 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstill
bugs1406421
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1406421 - Fix js::Thread race on windows (r=till) MozReview-Commit-ID: 32MrHVeATH5
js/src/threading/Thread.h
js/src/threading/posix/Thread.cpp
js/src/threading/windows/Thread.cpp
js/src/vm/MutexIDs.h
--- a/js/src/threading/Thread.h
+++ b/js/src/threading/Thread.h
@@ -12,16 +12,19 @@
 #include "mozilla/HashFunctions.h"
 #include "mozilla/IndexSequence.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/Tuple.h"
 
 #include <stdint.h>
 
 #include "js/Utility.h"
+#include "threading/LockGuard.h"
+#include "threading/Mutex.h"
+#include "vm/MutexIDs.h"
 
 #ifdef XP_WIN
 # define THREAD_RETURN_TYPE unsigned int
 # define THREAD_CALL_API __stdcall
 #else
 # define THREAD_RETURN_TYPE void*
 # define THREAD_CALL_API
 #endif
@@ -90,42 +93,41 @@ public:
   template <typename O = Options,
             // SFINAE to make sure we don't try and treat functors for the other
             // constructor as an Options and vice versa.
             typename NonConstO = typename mozilla::RemoveConst<O>::Type,
             typename DerefO = typename mozilla::RemoveReference<NonConstO>::Type,
             typename = typename mozilla::EnableIf<mozilla::IsSame<DerefO, Options>::value,
                                                   void*>::Type>
   explicit Thread(O&& options = Options())
-    : id_(Id())
+    : idMutex_(mutexid::ThreadId)
+    , id_(Id())
     , options_(mozilla::Forward<O>(options))
   { }
 
   // Start a thread of execution at functor |f| with parameters |args|. This
   // method will return false if thread creation fails. This Thread must not
   // already have been created. Note that the arguments must be either POD or
   // rvalue references (mozilla::Move). Attempting to pass a reference will
   // result in the value being copied, which may not be the intended behavior.
   // See the comment below on ThreadTrampoline::args for an explanation.
   template <typename F, typename... Args>
   MOZ_MUST_USE bool init(F&& f, Args&&... args) {
-    MOZ_RELEASE_ASSERT(!joinable());
+    MOZ_RELEASE_ASSERT(id_ == Id());
     using Trampoline = detail::ThreadTrampoline<F, Args...>;
     AutoEnterOOMUnsafeRegion oom;
     auto trampoline = js_new<Trampoline>(mozilla::Forward<F>(f),
                                          mozilla::Forward<Args>(args)...);
     if (!trampoline)
       oom.crash("js::Thread::init");
     return create(Trampoline::Start, trampoline);
   }
 
   // The thread must be joined or detached before destruction.
-  ~Thread() {
-    MOZ_RELEASE_ASSERT(!joinable());
-  }
+  ~Thread();
 
   // Move the thread into the detached state without blocking. In the detatched
   // state, the thread continues to run until it exits, but cannot be joined.
   // After this method returns, this Thread no longer represents a thread of
   // execution. When the thread exits, its resources will be cleaned up by the
   // system. At process exit, if the thread is still running, the thread's TLS
   // storage will be destructed, but the thread stack will *not* be unrolled.
   void detach();
@@ -134,34 +136,37 @@ public:
   // created with. The thread's resources will be cleaned up before this
   // function returns. After this method returns, this Thread no longer
   // represents a thread of execution.
   void join();
 
   // Return true if this thread has not yet been joined or detached. If this
   // method returns false, this Thread does not have an associated thread of
   // execution, for example, if it has been previously moved or joined.
-  bool joinable() const {
-    return get_id() != Id();
-  }
+  bool joinable();
 
   // Returns the id of this thread if this represents a thread of execution or
   // the default constructed Id() if not. The thread ID is guaranteed to
   // uniquely identify a thread and can be compared with the == operator.
-  Id get_id() const { return id_; }
+  Id get_id();
 
   // Allow threads to be moved so that they can be stored in containers.
   Thread(Thread&& aOther);
   Thread& operator=(Thread&& aOther);
 
 private:
   // Disallow copy as that's not sensible for unique resources.
   Thread(const Thread&) = delete;
   void operator=(const Thread&) = delete;
 
+  bool joinable(LockGuard<Mutex>& lock);
+
+  // Synchronize id_ initialization during thread startup.
+  Mutex idMutex_;
+
   // Provide a process global ID to each thread.
   Id id_;
 
   // Overridable thread creation options.
   Options options_;
 
   // Dispatch to per-platform implementation of thread creation.
   MOZ_MUST_USE bool create(THREAD_RETURN_TYPE (THREAD_CALL_API *aMain)(void*), void* aArg);
--- a/js/src/threading/posix/Thread.cpp
+++ b/js/src/threading/posix/Thread.cpp
@@ -69,66 +69,99 @@ js::Thread::Id::operator==(const Id& aOt
 {
   const PlatformData& self = *platformData();
   const PlatformData& other = *aOther.platformData();
   return (!self.hasThread && !other.hasThread) ||
          (self.hasThread == other.hasThread &&
           pthread_equal(self.ptThread, other.ptThread));
 }
 
+js::Thread::~Thread()
+{
+  LockGuard<Mutex> lock(idMutex_);
+  MOZ_RELEASE_ASSERT(!joinable(lock));
+}
+
 js::Thread::Thread(Thread&& aOther)
+  : idMutex_(mutexid::ThreadId)
 {
+  LockGuard<Mutex> lock(aOther.idMutex_);
   id_ = aOther.id_;
   aOther.id_ = Id();
   options_ = aOther.options_;
 }
 
 js::Thread&
 js::Thread::operator=(Thread&& aOther)
 {
-  MOZ_RELEASE_ASSERT(!joinable());
+  LockGuard<Mutex> lock(idMutex_);
+  MOZ_RELEASE_ASSERT(!joinable(lock));
   id_ = aOther.id_;
   aOther.id_ = Id();
   options_ = aOther.options_;
   return *this;
 }
 
 bool
 js::Thread::create(void* (*aMain)(void*), void* aArg)
 {
+  LockGuard<Mutex> lock(idMutex_);
+
   pthread_attr_t attrs;
   int r = pthread_attr_init(&attrs);
   MOZ_RELEASE_ASSERT(!r);
   if (options_.stackSize()) {
     r = pthread_attr_setstacksize(&attrs, options_.stackSize());
     MOZ_RELEASE_ASSERT(!r);
   }
-  id_.platformData()->hasThread = true;
   r = pthread_create(&id_.platformData()->ptThread, &attrs, aMain, aArg);
   if (r) {
     // |pthread_create| may leave id_ in an undefined state.
     id_ = Id();
     return false;
   }
+  id_.platformData()->hasThread = true;
   return true;
 }
 
 void
 js::Thread::join()
 {
-  MOZ_RELEASE_ASSERT(joinable());
+  LockGuard<Mutex> lock(idMutex_);
+  MOZ_RELEASE_ASSERT(joinable(lock));
   int r = pthread_join(id_.platformData()->ptThread, nullptr);
   MOZ_RELEASE_ASSERT(!r);
   id_ = Id();
 }
 
+js::Thread::Id
+js::Thread::get_id()
+{
+  LockGuard<Mutex> lock(idMutex_);
+  return id_;
+}
+
+bool
+js::Thread::joinable(LockGuard<Mutex>& lock)
+{
+  return id_ != Id();
+}
+
+bool
+js::Thread::joinable()
+{
+  LockGuard<Mutex> lock(idMutex_);
+  return joinable(lock);
+}
+
 void
 js::Thread::detach()
 {
-  MOZ_RELEASE_ASSERT(joinable());
+  LockGuard<Mutex> lock(idMutex_);
+  MOZ_RELEASE_ASSERT(joinable(lock));
   int r = pthread_detach(id_.platformData()->ptThread);
   MOZ_RELEASE_ASSERT(!r);
   id_ = Id();
 }
 
 js::Thread::Id
 js::ThisThread::GetId()
 {
--- a/js/src/threading/windows/Thread.cpp
+++ b/js/src/threading/windows/Thread.cpp
@@ -49,36 +49,47 @@ js::Thread::Id::Id()
 }
 
 bool
 js::Thread::Id::operator==(const Id& aOther) const
 {
   return platformData()->id == aOther.platformData()->id;
 }
 
+js::Thread::~Thread()
+{
+  LockGuard<Mutex> lock(idMutex_);
+  MOZ_RELEASE_ASSERT(!joinable(lock));
+}
+
 js::Thread::Thread(Thread&& aOther)
+  : idMutex_(mutexid::ThreadId)
 {
+  LockGuard<Mutex> lock(aOther.idMutex_);
   id_ = aOther.id_;
   aOther.id_ = Id();
   options_ = aOther.options_;
 }
 
 js::Thread&
 js::Thread::operator=(Thread&& aOther)
 {
-  MOZ_RELEASE_ASSERT(!joinable());
+  LockGuard<Mutex> lock(idMutex_);
+  MOZ_RELEASE_ASSERT(!joinable(lock));
   id_ = aOther.id_;
   aOther.id_ = Id();
   options_ = aOther.options_;
   return *this;
 }
 
 bool
 js::Thread::create(unsigned int (__stdcall* aMain)(void*), void* aArg)
 {
+  LockGuard<Mutex> lock(idMutex_);
+
   // Use _beginthreadex and not CreateThread, because threads that are
   // created with the latter leak a small amount of memory when they use
   // certain msvcrt functions and then exit.
   uintptr_t handle = _beginthreadex(nullptr, options_.stackSize(),
                                     aMain, aArg,
                                     STACK_SIZE_PARAM_IS_A_RESERVATION,
                                     &id_.platformData()->id);
   if (!handle) {
@@ -89,28 +100,50 @@ js::Thread::create(unsigned int (__stdca
   }
   id_.platformData()->handle = reinterpret_cast<HANDLE>(handle);
   return true;
 }
 
 void
 js::Thread::join()
 {
-  MOZ_RELEASE_ASSERT(joinable());
+  LockGuard<Mutex> lock(idMutex_);
+  MOZ_RELEASE_ASSERT(joinable(lock));
   DWORD r = WaitForSingleObject(id_.platformData()->handle, INFINITE);
   MOZ_RELEASE_ASSERT(r == WAIT_OBJECT_0);
   BOOL success = CloseHandle(id_.platformData()->handle);
   MOZ_RELEASE_ASSERT(success);
   id_ = Id();
 }
 
+js::Thread::Id
+js::Thread::get_id()
+{
+  LockGuard<Mutex> lock(idMutex_);
+  return id_;
+}
+
+bool
+js::Thread::joinable(LockGuard<Mutex>& lock)
+{
+  return id_ != Id();
+}
+
+bool
+js::Thread::joinable()
+{
+  LockGuard<Mutex> lock(idMutex_);
+  return joinable(lock);
+}
+
 void
 js::Thread::detach()
 {
-  MOZ_RELEASE_ASSERT(joinable());
+  LockGuard<Mutex> lock(idMutex_);
+  MOZ_RELEASE_ASSERT(joinable(lock));
   BOOL success = CloseHandle(id_.platformData()->handle);
   MOZ_RELEASE_ASSERT(success);
   id_ = Id();
 }
 
 js::Thread::Id
 js::ThisThread::GetId()
 {
--- a/js/src/vm/MutexIDs.h
+++ b/js/src/vm/MutexIDs.h
@@ -42,16 +42,17 @@
   _(IcuTimeZoneStateMutex,       500) \
   _(ProcessExecutableRegion,     500) \
   _(OffThreadPromiseState,       500) \
   _(BufferStreamState,           500) \
   _(WasmCodeProfilingLabels,     500) \
   _(WasmModuleTieringLock,       500) \
   _(WasmCompileTaskState,        500) \
                                       \
+  _(ThreadId,                    600) \
   _(WasmCodeSegmentMap,          600) \
   _(TraceLoggerGraphState,       600) \
   _(VTuneLock,                   600)
 
 namespace js {
 namespace mutexid {
 
 #define DEFINE_MUTEX_ID(name, order)  \