Add joinable option to SimpleThread::Options as was just done for Thread.

base/threading/simple_thread.cc

Index: base/threading/simple_thread.cc
diff --git a/base/threading/simple_thread.cc b/base/threading/simple_thread.cc
index 6c64a17d6ab2fea1ec792715ad4697adcc21bd94..41b577b403771a12b3e7f5543bf63d418b181634 100644
--- a/base/threading/simple_thread.cc
+++ b/base/threading/simple_thread.cc
@@ -4,7 +4,6 @@
 
 #include "base/threading/simple_thread.h"
 
-#include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_restrictions.h"
@@ -12,53 +11,46 @@
 namespace base {
 
 SimpleThread::SimpleThread(const std::string& name_prefix)
-    : name_prefix_(name_prefix),
-      name_(name_prefix),
-      thread_(),
-      event_(WaitableEvent::ResetPolicy::MANUAL,
-             WaitableEvent::InitialState::NOT_SIGNALED),
-      tid_(0),
-      joined_(false) {}
+    : SimpleThread(name_prefix, Options()) {}
 
 SimpleThread::SimpleThread(const std::string& name_prefix,
                            const Options& options)
     : name_prefix_(name_prefix),
       name_(name_prefix),
       options_(options),
-      thread_(),
       event_(WaitableEvent::ResetPolicy::MANUAL,
-             WaitableEvent::InitialState::NOT_SIGNALED),
-      tid_(0),
-      joined_(false) {}
+             WaitableEvent::InitialState::NOT_SIGNALED) {}
 
 SimpleThread::~SimpleThread() {
   DCHECK(HasBeenStarted()) << "SimpleThread was never started.";
-  DCHECK(HasBeenJoined()) << "SimpleThread destroyed without being Join()ed.";
+  DCHECK(!options_.joinable || HasBeenJoined())
+      << "Joinable SimpleThread destroyed without being Join()ed.";
 }
 
 void SimpleThread::Start() {
   DCHECK(!HasBeenStarted()) << "Tried to Start a thread multiple times.";
-  bool success;
-  if (options_.priority() == ThreadPriority::NORMAL) {
-    success = PlatformThread::Create(options_.stack_size(), this, &thread_);
-  } else {
-    success = PlatformThread::CreateWithPriority(options_.stack_size(), this,
-                                                 &thread_, options_.priority());
-  }
+  bool success =
+      options_.joinable
+          ? PlatformThread::CreateWithPriority(options_.stack_size, this,
+                                               &thread_, options_.priority)
+          : PlatformThread::CreateNonJoinableWithPriority(
+                options_.stack_size, this, options_.priority);
   DCHECK(success);
-  base::ThreadRestrictions::ScopedAllowWait allow_wait;
+  ThreadRestrictions::ScopedAllowWait allow_wait;
   event_.Wait();  // Wait for the thread to complete initialization.
 }
 
 void SimpleThread::Join() {
+  DCHECK(options_.joinable) << "A non-joinable thread can't be joined.";
   DCHECK(HasBeenStarted()) << "Tried to Join a never-started thread.";
   DCHECK(!HasBeenJoined()) << "Tried to Join a thread multiple times.";
   PlatformThread::Join(thread_);
+  thread_ = PlatformThreadHandle();
   joined_ = true;
 }
 
 bool SimpleThread::HasBeenStarted() {
-  base::ThreadRestrictions::ScopedAllowWait allow_wait;
+  ThreadRestrictions::ScopedAllowWait allow_wait;
   return event_.IsSignaled();
 }
 
@@ -75,26 +67,75 @@ void SimpleThread::ThreadMain() {
   Run();
 }
 
+DelegateSimpleThread::Delegate::Delegate()
+#if DCHECK_IS_ON()
+    : active_runners_decremented_(WaitableEvent::ResetPolicy::AUTOMATIC,
+                                  WaitableEvent::InitialState::NOT_SIGNALED)
+#endif
+{
+}
+
+DelegateSimpleThread::Delegate::~Delegate() {
+#if DCHECK_IS_ON()
+  // A Delegate is allowed to be destroyed after returning from Run(). This
+  // means that destruction can legally occur between the end of Run() and
+  // |active_runners_| being decremented in RunHelper(). Allow a minimal
+  // reasonable delay for this race to unwind.
+  constexpr TimeDelta kMaxCompletionWait = TimeDelta::FromMilliseconds(200);
+  TimeDelta remaining_time = kMaxCompletionWait;
+  const TimeTicks max_completion_time = TimeTicks::Now() + kMaxCompletionWait;
+  do {
+    // At least a single Wait() call is required (even when |active_runners_| is
+    // already zero) in order to synchronize with the matching Signal() call;
+    // otherwise there is a race between it and |active_runners_decremented_|'s
+    // destructor (as highlighted by TSAN); hence the do-while.
+    ThreadRestrictions::ScopedAllowWait allow_wait;
+    active_runners_decremented_.TimedWait(remaining_time);

[Lei Zhang, 2016/08/10 21:12:38]

Can we just Wait() instead of doing TimedWait() ?

[gab, 2016/08/10 22:37:00]

That would make the check not as good IMO as it would produce a hang instead of
A DCHECK on failure.

+    remaining_time = max_completion_time - TimeTicks::Now();
+  } while (!AtomicRefCountIsZero(&active_runners_) &&
+           remaining_time > TimeDelta());
+
+  DCHECK(AtomicRefCountIsZero(&active_runners_))
+      << "Delegate cannot be deleted with Run() in progress.";
+#endif  // DCHECK_IS_ON()
+}
+
+void DelegateSimpleThread::Delegate::RunHelper() {
+#if DCHECK_IS_ON()
+  AtomicRefCountInc(&active_runners_);
+#endif  // DCHECK_IS_ON()
+
+  Run();
+
+#if DCHECK_IS_ON()
+  AtomicRefCountDec(&active_runners_);
+  active_runners_decremented_.Signal();
+#endif  // DCHECK_IS_ON()
+}
+
 DelegateSimpleThread::DelegateSimpleThread(Delegate* delegate,
                                            const std::string& name_prefix)
-    : SimpleThread(name_prefix),
-      delegate_(delegate) {
-}
+    : DelegateSimpleThread(delegate, name_prefix, Options()) {}
 
 DelegateSimpleThread::DelegateSimpleThread(Delegate* delegate,
                                            const std::string& name_prefix,
                                            const Options& options)
     : SimpleThread(name_prefix, options),
       delegate_(delegate) {
+  DCHECK(delegate_);
 }
 
-DelegateSimpleThread::~DelegateSimpleThread() {
-}
+DelegateSimpleThread::~DelegateSimpleThread() = default;
 
 void DelegateSimpleThread::Run() {
-  DCHECK(delegate_) << "Tried to call Run without a delegate (called twice?)";
-  delegate_->Run();
-  delegate_ = NULL;
+#if DCHECK_IS_ON()
+  // |ran_| must be verified and updated before calling RunHelper() as non-
+  // joinable DelegateSimpleThread can be deleted before it returns.
+  DCHECK(!ran_) << "Ran twice?";
+  ran_ = true;
+#endif  // DCHECK_IS_ON()
+
+  delegate_->RunHelper();
 }
 
 DelegateSimpleThreadPool::DelegateSimpleThreadPool(
@@ -167,7 +208,7 @@ void DelegateSimpleThreadPool::Run() {
     if (!work)
       break;
 
-    work->Run();
+    work->RunHelper();
   }
 }