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);
+    remaining_time = max_completion_time - TimeTicks::Now();
+  } while (!AtomicRefCountIsZero(&active_runners_) &&
+           remaining_time > TimeDelta());
+
+  DCHECK(AtomicRefCountIsZero(&active_runners_))

[danakj, 2016/08/12 20:49:10]

Hm, I guess this is better than nothing? Idk, it's still racey. You can destroy
your delegate during run, but as long as you finish before 200ms this DCHECK
won't trip.

WTB giving ownership of the delegate to the DelegateSimpleThread.

[gab, 2016/08/12 21:23:17]

Right... I can't think of a way around this. My thinking is if something
incorrectly deletes the Delegate while it's running it won't typically finish at
the same time (and if it's truly wrong -- i.e. if the Run() method uses state
from its Delegate after it was deleted -- then it would trip ASan... I guess
this being covered by ASan could be an argument about not having the dchecks at
all).
The current API allows non-joinable DelegateSimpleThread to be deleted *during*
Run() so it's lifetime doesn't let it take Delegate. As described in the CL
description, this is the API I eventually think makes sense for base::Thread.

+      << "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();
+

[danakj, 2016/08/12 20:49:09]

would it help to DCHECK here that ~Delegate hasn't started to run yet?

[gab, 2016/08/12 21:23:17]

It's correct for a Delegate to say "DeleteSoon(this)" at the very end of its
Run() method, which could technically execute before we make it here. So a hard
DCHECK is incorrect for the same reason as the race described on line 80 (this
is why this DCHECK is kind of hard and needs to assume "reasonable unwind
timing").

+#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();
   }
 }