Bug 1573774 - Protect a thread's id only during creation r=luke

Differential Revision: https://phabricator.services.mozilla.com/D42801

--HG--
extra : moz-landing-system : lando

js/src/threading/Thread.cpp

@@ -11,32 +11,22 @@ namespace js {
 
 Thread::~Thread() { MOZ_RELEASE_ASSERT(!joinable()); }
 
-Thread::Thread(Thread&& aOther) : idMutex_(mutexid::ThreadId) {
-  LockGuard<Mutex> lock(aOther.idMutex_);
+Thread::Thread(Thread&& aOther) {
   id_ = aOther.id_;
   aOther.id_ = Id();
   options_ = aOther.options_;
 }
 
 Thread& Thread::operator=(Thread&& aOther) {
-  LockGuard<Mutex> lock(idMutex_);
-  MOZ_RELEASE_ASSERT(!joinable(lock));
+  MOZ_RELEASE_ASSERT(!joinable());
   id_ = aOther.id_;
   aOther.id_ = Id();
   options_ = aOther.options_;
   return *this;
 }
 
-Thread::Id Thread::get_id() {
-  LockGuard<Mutex> lock(idMutex_);
-  return id_;
-}
+Thread::Id Thread::get_id() { return id_; }
 
-bool Thread::joinable(LockGuard<Mutex>& lock) { return id_ != Id(); }
-
-bool Thread::joinable() {
-  LockGuard<Mutex> lock(idMutex_);
-  return joinable(lock);
-}
+bool Thread::joinable() { return id_ != Id(); }
 
 }  // namespace js

js/src/threading/Thread.h

@@ -84,9 +84,7 @@ class Thread {
       typename = typename mozilla::EnableIf<
           mozilla::IsSame<DerefO, Options>::value, void*>::Type>
   explicit Thread(O&& options = Options())
-      : idMutex_(mutexid::ThreadId),
-        id_(Id()),
-        options_(std::forward<O>(options)) {
+      : id_(Id()), options_(std::forward<O>(options)) {
     MOZ_ASSERT(js::IsInitialized());
   }
 
@@ -106,6 +104,9 @@ class Thread {
     if (!trampoline) {
       oom.crash("js::Thread::init");
     }
+
+    // We hold this lock while create() sets the thread id.
+    LockGuard<Mutex> lock(trampoline->createMutex);
     return create(Trampoline::Start, trampoline);
   }
 
@@ -145,11 +146,6 @@ class Thread {
   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_;
 
@@ -203,6 +199,12 @@ class ThreadTrampoline {
   // impossible to pass references between threads.
   mozilla::Tuple<typename mozilla::Decay<Args>::Type...> args;
 
+  // Protect the thread id during creation.
+  Mutex createMutex;
+
+  // Thread can access createMutex.
+  friend class js::Thread;
+
  public:
   // Note that this template instatiation duplicates and is identical to the
   // class template instantiation. It is required for perfect forwarding of
@@ -210,7 +212,9 @@ class ThreadTrampoline {
   // even if the class template arguments are correct.
   template <typename G, typename... ArgsT>
   explicit ThreadTrampoline(G&& aG, ArgsT&&... aArgsT)
-      : f(std::forward<F>(aG)), args(std::forward<Args>(aArgsT)...) {}
+      : f(std::forward<F>(aG)),
+        args(std::forward<Args>(aArgsT)...),
+        createMutex(mutexid::ThreadId) {}
 
   static THREAD_RETURN_TYPE THREAD_CALL_API Start(void* aPack) {
     auto* pack = static_cast<ThreadTrampoline<F, Args...>*>(aPack);
@@ -221,6 +225,10 @@ class ThreadTrampoline {
 
   template <size_t... Indices>
   void callMain(std::index_sequence<Indices...>) {
+    // Pretend createMutex is a semaphore and wait for a notification that the
+    // thread that spawned us is ready.
+    createMutex.lock();
+    createMutex.unlock();
     f(mozilla::Get<Indices>(args)...);
   }
 };

js/src/threading/posix/PosixThread.cpp

@@ -35,8 +35,6 @@ bool Thread::Id::operator==(const Id& aOther) const {
 }
 
 bool 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);
@@ -44,6 +42,7 @@ bool Thread::create(void* (*aMain)(void*), void* aArg) {
     r = pthread_attr_setstacksize(&attrs, options_.stackSize());
     MOZ_RELEASE_ASSERT(!r);
   }
+
   r = pthread_create(&id_.platformData()->ptThread, &attrs, aMain, aArg);
   if (r) {
     // On either Windows or POSIX we can't be sure if id_ was initialised. So
@@ -56,16 +55,14 @@ bool Thread::create(void* (*aMain)(void*), void* aArg) {
 }
 
 void Thread::join() {
-  LockGuard<Mutex> lock(idMutex_);
-  MOZ_RELEASE_ASSERT(joinable(lock));
+  MOZ_RELEASE_ASSERT(joinable());
   int r = pthread_join(id_.platformData()->ptThread, nullptr);
   MOZ_RELEASE_ASSERT(!r);
   id_ = Id();
 }
 
 void Thread::detach() {
-  LockGuard<Mutex> lock(idMutex_);
-  MOZ_RELEASE_ASSERT(joinable(lock));
+  MOZ_RELEASE_ASSERT(joinable());
   int r = pthread_detach(id_.platformData()->ptThread);
   MOZ_RELEASE_ASSERT(!r);
   id_ = Id();

js/src/threading/windows/WindowsThread.cpp

@@ -32,8 +32,6 @@ bool Thread::Id::operator==(const Id& aOther) const {
 }
 
 bool 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.
@@ -51,8 +49,7 @@ bool Thread::create(unsigned int(__stdcall* aMain)(void*), void* aArg) {
 }
 
 void Thread::join() {
-  LockGuard<Mutex> lock(idMutex_);
-  MOZ_RELEASE_ASSERT(joinable(lock));
+  MOZ_RELEASE_ASSERT(joinable());
   DWORD r = WaitForSingleObject(id_.platformData()->handle, INFINITE);
   MOZ_RELEASE_ASSERT(r == WAIT_OBJECT_0);
   BOOL success = CloseHandle(id_.platformData()->handle);
@@ -61,8 +58,7 @@ void Thread::join() {
 }
 
 void Thread::detach() {
-  LockGuard<Mutex> lock(idMutex_);
-  MOZ_RELEASE_ASSERT(joinable(lock));
+  MOZ_RELEASE_ASSERT(joinable());
   BOOL success = CloseHandle(id_.platformData()->handle);
   MOZ_RELEASE_ASSERT(success);
   id_ = Id();