Bug 956899 - Use mozilla::Decay to copy js::Thread args off thread; r=froydnj

--HG--
extra : rebase_source : 22e8e115386843dc6a7ca2c449f2373b6b8dabfa

js/src/jsapi-tests/testThreadingConditionVariable.cpp

@@ -20,13 +20,13 @@ struct TestState {
       : flag(false)
     {
         if (createThread)
-            MOZ_RELEASE_ASSERT(testThread.init(setFlag, *this));
+            MOZ_RELEASE_ASSERT(testThread.init(setFlag, this));
     }
 
-    static void setFlag(TestState& state) {
-        js::UniqueLock<js::Mutex> lock(state.mutex);
-        state.flag = true;
-        state.condition.notify_one();
+    static void setFlag(TestState* state) {
+        js::UniqueLock<js::Mutex> lock(state->mutex);
+        state->flag = true;
+        state->condition.notify_one();
     }
 
     void join() {

js/src/jsapi-tests/testThreadingExclusiveData.cpp

@@ -44,23 +44,23 @@ printDiagnosticMessage(uint64_t seen)
 }
 
 void
-setBitAndCheck(CounterAndBit& counterAndBit)
+setBitAndCheck(CounterAndBit* counterAndBit)
 {
     while (true) {
         {
             // Set our bit. Repeatedly setting it is idempotent.
-            auto guard = counterAndBit.counter.lock();
+            auto guard = counterAndBit->counter.lock();
             printDiagnosticMessage(guard);
-            guard |= (uint64_t(1) << counterAndBit.bit);
+            guard |= (uint64_t(1) << counterAndBit->bit);
         }
 
         {
             // Check to see if we have observed all the other threads setting
             // their bit as well.
-            auto guard = counterAndBit.counter.lock();
+            auto guard = counterAndBit->counter.lock();
             printDiagnosticMessage(guard);
             if (guard == UINT64_MAX) {
-                js_delete(&counterAndBit);
+                js_delete(counterAndBit);
                 return;
             }
         }
@@ -77,7 +77,7 @@ BEGIN_TEST(testExclusiveData)
     for (auto i : mozilla::MakeRange(NumThreads)) {
         auto counterAndBit = js_new<CounterAndBit>(i, counter);
         CHECK(counterAndBit);
-        CHECK(threads.emplaceBack(setBitAndCheck, *counterAndBit));
+        CHECK(threads.emplaceBack(setBitAndCheck, counterAndBit));
     }
 
     for (auto& thread : threads)

js/src/jsapi-tests/testThreadingThread.cpp

@@ -73,3 +73,22 @@ BEGIN_TEST(testThreadingThreadVectorMoveConstruct)
     return true;
 }
 END_TEST(testThreadingThreadVectorMoveConstruct)
+
+// This test is checking that args are using "decay" copy, per spec. If we do
+// not use decay copy properly, the rvalue reference |bool&& b| in the
+// constructor will automatically become an lvalue reference |bool& b| in the
+// trampoline, causing us to read through the reference when passing |bool bb|
+// from the trampoline. If the parent runs before the child, the bool may have
+// already become false, causing the trampoline to read the changed value, thus
+// causing the child's assertion to fail.
+BEGIN_TEST(testThreadingThreadArgCopy)
+{
+    for (size_t i = 0; i < 10000; ++i) {
+        bool b = true;
+        js::Thread thread([](bool bb){MOZ_RELEASE_ASSERT(bb);}, b);
+        b = false;
+        thread.join();
+    }
+    return true;
+}
+END_TEST(testThreadingThreadArgCopy)

js/src/threading/Thread.h

@@ -58,7 +58,11 @@ public:
   // be created for this Thread by calling |init|.
   Thread() : id_(Id()) {}
 
-  // Start a thread of execution at functor |f| with parameters |args|.
+  // Start a thread of execution at functor |f| with parameters |args|. 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>
   explicit Thread(F&& f, Args&&... args) {
     MOZ_RELEASE_ASSERT(init(mozilla::Forward<F>(f),
@@ -145,13 +149,32 @@ namespace detail {
 template <typename F, typename... Args>
 class ThreadTrampoline
 {
+  // The functor to call.
   F f;
-  mozilla::Tuple<Args...> args;
+
+  // A std::decay copy of the arguments, as specified by std::thread. Using an
+  // rvalue reference for the arguments to Thread and ThreadTrampoline gives us
+  // move semantics for large structures, allowing us to quickly and easily pass
+  // enormous amounts of data to a new thread. Unfortunately, there is a
+  // downside: rvalue references becomes lvalue references when used with POD
+  // types. This becomes dangerous when attempting to pass POD stored on the
+  // stack to the new thread; the rvalue reference will implicitly become an
+  // lvalue reference to the stack location. Thus, the value may not exist if
+  // the parent thread leaves the frame before the read happens in the new
+  // thread. To avoid this dangerous and highly non-obvious footgun, the
+  // standard requires a "decay" copy of the arguments at the cost of making it
+  // impossible to pass references between threads.
+  mozilla::Tuple<typename mozilla::Decay<Args>::Type...> args;
 
 public:
-  explicit ThreadTrampoline(F&& aF, Args&&... aArgs)
-    : f(mozilla::Forward<F>(aF)),
-      args(mozilla::Forward<Args>(aArgs)...)
+  // Note that this template instatiation duplicates and is identical to the
+  // class template instantiation. It is required for perfect forwarding of
+  // rvalue references, which is only enabled for calls to a function template,
+  // even if the class template arguments are correct.
+  template <typename G, typename... ArgsT>
+  explicit ThreadTrampoline(G&& aG, ArgsT&&... aArgsT)
+    : f(mozilla::Forward<F>(aG)),
+      args(mozilla::Forward<Args>(aArgsT)...)
   {
   }
 

js/src/threading/posix/Thread.cpp

@@ -87,8 +87,11 @@ bool
 js::Thread::create(void* (*aMain)(void*), void* aArg)
 {
   int r = pthread_create(&id_.platformData()->ptThread, nullptr, aMain, aArg);
-  if (r)
+  if (r) {
+    // |pthread_create| may leave id_ in an undefined state.
+    id_ = Id();
     return false;
+  }
   id_.platformData()->hasThread = true;
   return true;
 }

js/src/threading/windows/Thread.cpp

@@ -72,8 +72,12 @@ js::Thread::create(unsigned int (__stdcall* aMain)(void*), void* aArg)
   // certain msvcrt functions and then exit.
   uintptr_t handle = _beginthreadex(nullptr, 0, aMain, aArg, 0,
                                     &id_.platformData()->id);
-  if (!handle)
+  if (!handle) {
+    // The documentation does not say what state the thread id has if the method
+    // fails, so assume that it is undefined and reset it manually.
+    id_ = Id();
     return false;
+  }
   id_.platformData()->handle = reinterpret_cast<HANDLE>(handle);
   return true;
 }