Modernize base::Thread

No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001

In this CL:

 - Explicitly document the threading requirements of the API after grokking its use of locks
   in some places and lack of in some others)
   - (It's mostly thread unsafe; except for a few calls which are okay from the owner
      as well as the underlying thread; and one method which is actually thread-safe)
   - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139)
   - Remove |thread_lock_| which is unnecessary under the documented conditions
     (delayed until http://crbug.com/629139 is fixed..).

 - C++11 member initializers (makes it cleaner to add new POD members and
                              makes constructors simpler -- e.g. |run_loop_| was uninitialized)
 - Assertions and tests for current API which allows Start/Stop/Start cycles
   but wasn't tested... (actually more than that is poorly/not tested in this class
   but I'm merely adding tests for parts I'm stressing in my actual follow-up CL).

 - Comment nits

 - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool.

BUG=629716, 629139

Review-Url: https://codereview.chromium.org/2145463002
Cr-Commit-Position: refs/heads/master@{#407265}

base/threading/thread.cc

@@ -33,42 +33,26 @@ base::LazyInstance<base::ThreadLocalBoolean> lazy_tls_bool =
 
 }  // namespace
 
-Thread::Options::Options()
-    : message_loop_type(MessageLoop::TYPE_DEFAULT),
-      timer_slack(TIMER_SLACK_NONE),
-      stack_size(0),
-      priority(ThreadPriority::NORMAL) {
-}
+Thread::Options::Options() = default;
 
-Thread::Options::Options(MessageLoop::Type type,
-                         size_t size)
-    : message_loop_type(type),
-      timer_slack(TIMER_SLACK_NONE),
-      stack_size(size),
-      priority(ThreadPriority::NORMAL) {
-}
+Thread::Options::Options(MessageLoop::Type type, size_t size)
+    : message_loop_type(type), stack_size(size) {}
 
 Thread::Options::Options(const Options& other) = default;
 
-Thread::Options::~Options() {
-}
+Thread::Options::~Options() = default;
 
 Thread::Thread(const std::string& name)
-    :
-#if defined(OS_WIN)
-      com_status_(NONE),
-#endif
-      stopping_(false),
-      running_(false),
-      thread_(0),
-      id_(kInvalidThreadId),
-      id_event_(WaitableEvent::ResetPolicy::MANUAL,
+    : id_event_(WaitableEvent::ResetPolicy::MANUAL,
                 WaitableEvent::InitialState::NOT_SIGNALED),
-      message_loop_(nullptr),
-      message_loop_timer_slack_(TIMER_SLACK_NONE),
       name_(name),
       start_event_(WaitableEvent::ResetPolicy::MANUAL,
                    WaitableEvent::InitialState::NOT_SIGNALED) {
+  // Only bind the sequence on Start(): the state is constant between
+  // construction and Start() and it's thus valid for Start() to be called on
+  // another sequence as long as every other operation is then performed on that
+  // sequence.
+  owning_sequence_checker_.DetachFromSequence();
 }
 
 Thread::~Thread() {
@@ -76,6 +60,8 @@ Thread::~Thread() {
 }
 
 bool Thread::Start() {
+  DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
+
   Options options;
 #if defined(OS_WIN)
   if (com_status_ == STA)
@@ -85,7 +71,9 @@ bool Thread::Start() {
 }
 
 bool Thread::StartWithOptions(const Options& options) {
+  DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(!message_loop_);
+  DCHECK(!IsRunning());
 #if defined(OS_WIN)
   DCHECK((com_status_ != STA) ||
       (options.message_loop_type == MessageLoop::TYPE_UI));
@@ -102,13 +90,14 @@ bool Thread::StartWithOptions(const Options& options) {
     type = MessageLoop::TYPE_CUSTOM;
 
   message_loop_timer_slack_ = options.timer_slack;
-  std::unique_ptr<MessageLoop> message_loop =
+  std::unique_ptr<MessageLoop> message_loop_owned =
       MessageLoop::CreateUnbound(type, options.message_pump_factory);
-  message_loop_ = message_loop.get();
+  message_loop_ = message_loop_owned.get();
   start_event_.Reset();
 
-  // Hold the thread_lock_ while starting a new thread, so that we can make sure
-  // that thread_ is populated before the newly created thread accesses it.
+  // Hold |thread_lock_| while starting the new thread to synchronize with
+  // Stop() while it's not guaranteed to be sequenced (until crbug/629139 is
+  // fixed).
   {
     AutoLock lock(thread_lock_);
     if (!PlatformThread::CreateWithPriority(options.stack_size, this, &thread_,
@@ -119,15 +108,16 @@ bool Thread::StartWithOptions(const Options& options) {
     }
   }
 
-  // The ownership of message_loop is managemed by the newly created thread
+  // The ownership of |message_loop_| is managed by the newly created thread
   // within the ThreadMain.
-  ignore_result(message_loop.release());
+  ignore_result(message_loop_owned.release());
 
   DCHECK(message_loop_);
   return true;
 }
 
 bool Thread::StartAndWaitForTesting() {
+  DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
   bool result = Start();
   if (!result)
     return false;
@@ -136,6 +126,7 @@ bool Thread::StartAndWaitForTesting() {
 }
 
 bool Thread::WaitUntilThreadStarted() const {
+  DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
   if (!message_loop_)
     return false;
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
@@ -144,7 +135,12 @@ bool Thread::WaitUntilThreadStarted() const {
 }
 
 void Thread::Stop() {
+  // TODO(gab): Fix improper usage of this API (http://crbug.com/629139) and
+  // enable this check, until then synchronization with Start() via
+  // |thread_lock_| is required...
+  // DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
   AutoLock lock(thread_lock_);
+
   if (thread_.is_null())
     return;
 
@@ -152,22 +148,23 @@ void Thread::Stop() {
 
   // Wait for the thread to exit.
   //
-  // TODO(darin): Unfortunately, we need to keep message_loop_ around until
+  // TODO(darin): Unfortunately, we need to keep |message_loop_| around until
   // the thread exits.  Some consumers are abusing the API.  Make them stop.
   //
   PlatformThread::Join(thread_);
   thread_ = base::PlatformThreadHandle();
 
-  // The thread should nullify message_loop_ on exit.
+  // The thread should nullify |message_loop_| on exit (note: Join() adds an
+  // implicit memory barrier and no lock is thus required for this check).
   DCHECK(!message_loop_);
 
   stopping_ = false;
 }
 
 void Thread::StopSoon() {
-  // We should only be called on the same thread that started us.
-
-  DCHECK_NE(GetThreadId(), PlatformThread::CurrentId());
+  // TODO(gab): Fix improper usage of this API (http://crbug.com/629139) and
+  // enable this check.
+  // DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
 
   if (stopping_ || !message_loop_)
     return;
@@ -185,26 +182,35 @@ PlatformThreadId Thread::GetThreadId() const {
 }
 
 bool Thread::IsRunning() const {
-  // If the thread's already started (i.e. message_loop_ is non-null) and
-  // not yet requested to stop (i.e. stopping_ is false) we can just return
-  // true. (Note that stopping_ is touched only on the same thread that
-  // starts / started the new thread so we need no locking here.)
+  // TODO(gab): Fix improper usage of this API (http://crbug.com/629139) and
+  // enable this check.
+  // DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
+
+  // If the thread's already started (i.e. |message_loop_| is non-null) and not
+  // yet requested to stop (i.e. |stopping_| is false) we can just return true.
+  // (Note that |stopping_| is touched only on the same sequence that starts /
+  // started the new thread so we need no locking here.)
   if (message_loop_ && !stopping_)
     return true;
-  // Otherwise check the running_ flag, which is set to true by the new thread
+  // Otherwise check the |running_| flag, which is set to true by the new thread
   // only while it is inside Run().
   AutoLock lock(running_lock_);
   return running_;
 }
 
 void Thread::Run(RunLoop* run_loop) {
+  // Overridable protected method to be called from our |thread_| only.
+  DCHECK_EQ(id_, PlatformThread::CurrentId());
+
   run_loop->Run();
 }
 
+// static
 void Thread::SetThreadWasQuitProperly(bool flag) {
   lazy_tls_bool.Pointer()->Set(flag);
 }
 
+// static
 bool Thread::GetThreadWasQuitProperly() {
   bool quit_properly = true;
 #ifndef NDEBUG
@@ -224,7 +230,7 @@ void Thread::ThreadMain() {
   PlatformThread::SetName(name_.c_str());
   ANNOTATE_THREAD_NAME(name_.c_str());  // Tell the name to race detector.
 
-  // Lazily initialize the message_loop so that it can run on this thread.
+  // Lazily initialize the |message_loop| so that it can run on this thread.
   DCHECK(message_loop_);
   std::unique_ptr<MessageLoop> message_loop(message_loop_);
   message_loop_->BindToCurrentThread();

base/threading/thread.h

@@ -15,6 +15,7 @@
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/timer_slack.h"
+#include "base/sequence_checker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
@@ -39,6 +40,10 @@ class RunLoop;
 //  (1) Thread::CleanUp()
 //  (2) MessageLoop::~MessageLoop
 //  (3.b)    MessageLoop::DestructionObserver::WillDestroyCurrentMessageLoop
+//
+// This API is not thread-safe: unless indicated otherwise its methods are only
+// valid from the owning sequence (which is the one from which Start() is
+// invoked, should it differ from the one on which it was constructed).
 class BASE_EXPORT Thread : PlatformThread::Delegate {
  public:
   struct BASE_EXPORT Options {
@@ -51,10 +56,10 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
 
     // Specifies the type of message loop that will be allocated on the thread.
     // This is ignored if message_pump_factory.is_null() is false.
-    MessageLoop::Type message_loop_type;
+    MessageLoop::Type message_loop_type = MessageLoop::TYPE_DEFAULT;
 
     // Specifies timer slack for thread message loop.
-    TimerSlack timer_slack;
+    TimerSlack timer_slack = TIMER_SLACK_NONE;
 
     // Used to create the MessagePump for the MessageLoop. The callback is Run()
     // on the thread. If message_pump_factory.is_null(), then a MessagePump
@@ -65,10 +70,10 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
     // Specifies the maximum stack size that the thread is allowed to use.
     // This does not necessarily correspond to the thread's initial stack size.
     // A value of 0 indicates that the default maximum should be used.
-    size_t stack_size;
+    size_t stack_size = 0;
 
     // Specifies the initial thread priority.
-    ThreadPriority priority;
+    ThreadPriority priority = ThreadPriority::NORMAL;
   };
 
   // Constructor.
@@ -159,14 +164,39 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   // NOTE: You must not call this MessageLoop's Quit method directly.  Use
   // the Thread's Stop method instead.
   //
-  MessageLoop* message_loop() const { return message_loop_; }
+  // In addition to this Thread's owning sequence, this can also safely be
+  // called from the underlying thread itself.
+  MessageLoop* message_loop() const {
+    // This class doesn't provide synchronization around |message_loop_| and as
+    // such only the owner should access it (and the underlying thread which
+    // never sees it before it's set). In practice, many callers are coming from
+    // unrelated threads but provide their own implicit (e.g. memory barriers
+    // from task posting) or explicit (e.g. locks) synchronization making the
+    // access of |message_loop_| safe... Changing all of those callers is
+    // unfeasible; instead verify that they can reliably see
+    // |message_loop_ != nullptr| without synchronization as a proof that their
+    // external synchronization catches the unsynchronized effects of Start().
+    // TODO(gab): Despite all of the above this test has to be disabled for now
+    // per crbug.com/629139#c6.
+    // DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread() ||
+    //        id_ == PlatformThread::CurrentId() || message_loop_)
+    //     << id_ << " vs " << PlatformThread::CurrentId();
+    return message_loop_;
+  }
 
   // Returns a TaskRunner for this thread. Use the TaskRunner's PostTask
   // methods to execute code on the thread. Returns nullptr if the thread is not
   // running (e.g. before Start or after Stop have been called). Callers can
   // hold on to this even after the thread is gone; in this situation, attempts
   // to PostTask() will fail.
+  //
+  // In addition to this Thread's owning sequence, this can also safely be
+  // called from the underlying thread itself.
   scoped_refptr<SingleThreadTaskRunner> task_runner() const {
+    // Refer to the DCHECK and comment inside |message_loop()|.
+    DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread() ||
+           id_ == PlatformThread::CurrentId() || message_loop_)
+        << id_ << " vs " << PlatformThread::CurrentId();
     return message_loop_ ? message_loop_->task_runner() : nullptr;
   }
 
@@ -179,6 +209,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   //
   // WARNING: This function will block if the thread hasn't started yet.
   //
+  // This method is thread-safe.
   PlatformThreadId GetThreadId() const;
 
   // Returns true if the thread has been started, and not yet stopped.
@@ -198,6 +229,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   static bool GetThreadWasQuitProperly();
 
   void set_message_loop(MessageLoop* message_loop) {
+    DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
     message_loop_ = message_loop;
   }
 
@@ -217,17 +249,17 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
 
 #if defined(OS_WIN)
   // Whether this thread needs to initialize COM, and if so, in what mode.
-  ComStatus com_status_;
+  ComStatus com_status_ = NONE;
 #endif
 
   // If true, we're in the middle of stopping, and shouldn't access
   // |message_loop_|. It may non-nullptr and invalid.
   // Should be written on the thread that created this thread. Also read data
   // could be wrong on other threads.
-  bool stopping_;
+  bool stopping_ = false;
 
   // True while inside of Run().
-  bool running_;
+  bool running_ = false;
   mutable base::Lock running_lock_;  // Protects |running_|.
 
   // The thread's handle.
@@ -235,24 +267,28 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   mutable base::Lock thread_lock_;  // Protects |thread_|.
 
   // The thread's id once it has started.
-  PlatformThreadId id_;
+  PlatformThreadId id_ = kInvalidThreadId;
   mutable WaitableEvent id_event_;  // Protects |id_|.
 
   // The thread's MessageLoop and RunLoop. Valid only while the thread is alive.
   // Set by the created thread.
-  MessageLoop* message_loop_;
-  RunLoop* run_loop_;
+  MessageLoop* message_loop_ = nullptr;
+  RunLoop* run_loop_ = nullptr;
 
   // Stores Options::timer_slack_ until the message loop has been bound to
   // a thread.
-  TimerSlack message_loop_timer_slack_;
+  TimerSlack message_loop_timer_slack_ = TIMER_SLACK_NONE;
 
   // The name of the thread.  Used for debugging purposes.
-  std::string name_;
+  const std::string name_;
 
   // Signaled when the created thread gets ready to use the message loop.
   mutable WaitableEvent start_event_;
 
+  // This class is not thread-safe, use this to verify access from the owning
+  // sequence of the Thread.
+  SequenceChecker owning_sequence_checker_;
+
   DISALLOW_COPY_AND_ASSIGN(Thread);
 };
 

base/threading/thread_unittest.cc

@@ -157,16 +157,11 @@ TEST_F(ThreadTest, StartWithOptions_StackSize) {
   EXPECT_TRUE(a.message_loop());
   EXPECT_TRUE(a.IsRunning());
 
-  bool was_invoked = false;
-  a.task_runner()->PostTask(FROM_HERE, base::Bind(&ToggleValue, &was_invoked));
-
-  // wait for the task to run (we could use a kernel event here
-  // instead to avoid busy waiting, but this is sufficient for
-  // testing purposes).
-  for (int i = 100; i >= 0 && !was_invoked; --i) {
-    base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(10));
-  }
-  EXPECT_TRUE(was_invoked);
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::AUTOMATIC,
+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+  a.task_runner()->PostTask(FROM_HERE, base::Bind(&base::WaitableEvent::Signal,
+                                                  base::Unretained(&event)));
+  event.Wait();
 }
 
 TEST_F(ThreadTest, TwoTasks) {
@@ -195,8 +190,43 @@ TEST_F(ThreadTest, StopSoon) {
   EXPECT_TRUE(a.message_loop());
   EXPECT_TRUE(a.IsRunning());
   a.StopSoon();
+  a.Stop();
+  EXPECT_FALSE(a.message_loop());
+  EXPECT_FALSE(a.IsRunning());
+}
+
+TEST_F(ThreadTest, StopTwiceNop) {
+  Thread a("StopTwiceNop");
+  EXPECT_TRUE(a.Start());
+  EXPECT_TRUE(a.message_loop());
+  EXPECT_TRUE(a.IsRunning());
+  a.StopSoon();
+  // Calling StopSoon() a second time should be a nop.
   a.StopSoon();
   a.Stop();
+  // Same with Stop().
+  a.Stop();
+  EXPECT_FALSE(a.message_loop());
+  EXPECT_FALSE(a.IsRunning());
+  // Calling them when not running should also nop.
+  a.StopSoon();
+  a.Stop();
+}
+
+TEST_F(ThreadTest, StartTwice) {
+  Thread a("StartTwice");
+
+  EXPECT_TRUE(a.Start());
+  EXPECT_TRUE(a.message_loop());
+  EXPECT_TRUE(a.IsRunning());
+  a.Stop();
+  EXPECT_FALSE(a.message_loop());
+  EXPECT_FALSE(a.IsRunning());
+
+  EXPECT_TRUE(a.Start());
+  EXPECT_TRUE(a.message_loop());
+  EXPECT_TRUE(a.IsRunning());
+  a.Stop();
   EXPECT_FALSE(a.message_loop());
   EXPECT_FALSE(a.IsRunning());
 }