Modernize base::Thread

base/threading/thread.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/threading/thread.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
@@ skipping 15 matching lines @@
 
 // We use this thread-local variable to record whether or not a thread exited
 // because its Stop method was called.  This allows us to catch cases where
 // MessageLoop::QuitWhenIdle() is called directly, which is unexpected when
 // using a Thread to setup and run a MessageLoop.
 base::LazyInstance<base::ThreadLocalBoolean> lazy_tls_bool =
     LAZY_INSTANCE_INITIALIZER;
 
 }  // 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

[danakj, 2016/07/21 19:43:26]

nice comment

[gab, 2016/07/22 16:02:54]

:-)

+  // 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.
+  sequence_checker_.DetachFromSequence();
 }
 
 Thread::~Thread() {
   Stop();
 }
 
 bool Thread::Start() {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+
   Options options;
 #if defined(OS_WIN)
   if (com_status_ == STA)
     options.message_loop_type = MessageLoop::TYPE_UI;
 #endif
   return StartWithOptions(options);
 }
 
 bool Thread::StartWithOptions(const Options& options) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(!message_loop_);
+  DCHECK(!IsRunning());
 #if defined(OS_WIN)
   DCHECK((com_status_ != STA) ||
       (options.message_loop_type == MessageLoop::TYPE_UI));
 #endif
 
   // Reset |id_| here to support restarting the thread.
   id_event_.Reset();
   id_ = kInvalidThreadId;
 
   SetThreadWasQuitProperly(false);
 
   MessageLoop::Type type = options.message_loop_type;
   if (!options.message_pump_factory.is_null())
     type = MessageLoop::TYPE_CUSTOM;
 
   message_loop_timer_slack_ = options.timer_slack;
   std::unique_ptr<MessageLoop> message_loop =

[danakj, 2016/07/21 19:43:26]

can you name this so that there is more difference between it and the member
variable than an underscore? message_loop_owned maybe?

[gab, 2016/07/22 16:02:55]

Done.

       MessageLoop::CreateUnbound(type, options.message_pump_factory);
   message_loop_ = message_loop.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.
-  {
-    AutoLock lock(thread_lock_);

[danakj, 2016/07/21 19:43:26]

I think you do need this until http://crbug.com/629139 is fixed, because Stop()
uses it for example.

[gab, 2016/07/22 16:02:55]

Hmmm I guess yea :-(, AFAICT there's only one bad test that depends on this
(fixed in https://codereview.chromium.org/2170953002/), but no harm keeping the
lock for a little while longer while this is fixed (and we confirm that the
check passes on all bots after).

-    if (!PlatformThread::CreateWithPriority(options.stack_size, this, &thread_,
-                                            options.priority)) {
-      DLOG(ERROR) << "failed to create thread";
-      message_loop_ = nullptr;
-      return false;
-    }
+  if (!PlatformThread::CreateWithPriority(options.stack_size, this, &thread_,
+                                          options.priority)) {
+    DLOG(ERROR) << "failed to create thread";
+    message_loop_ = nullptr;
+    return false;
   }
 
-  // The ownership of message_loop is managemed by the newly created thread
+  // The ownership of |message_loop| is managed by the newly created thread

[danakj, 2016/07/21 19:43:25]

I think saying "|message_loop_|" (ie the member) here would be even more clear.

[gab, 2016/07/22 16:02:55]

Done.

   // within the ThreadMain.
   ignore_result(message_loop.release());
 
   DCHECK(message_loop_);
   return true;
 }
 
 bool Thread::StartAndWaitForTesting() {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   bool result = Start();
   if (!result)
     return false;
   WaitUntilThreadStarted();
   return true;
 }
 
 bool Thread::WaitUntilThreadStarted() const {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   if (!message_loop_)
     return false;
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
   start_event_.Wait();
   return true;
 }
 
 void Thread::Stop() {
-  AutoLock lock(thread_lock_);
+  // TODO(gab): Fix improper usage of this API (http://crbug.com/629139) and
+  // enable this check.
+  // DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+
   if (thread_.is_null())
     return;
 
   StopSoon();
 
   // 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(sequence_checker_.CalledOnValidSequencedThread());
 
   if (stopping_ || !message_loop_)
     return;
 
   stopping_ = true;
   task_runner()->PostTask(
       FROM_HERE, base::Bind(&Thread::ThreadQuitHelper, Unretained(this)));
 }
 
 PlatformThreadId Thread::GetThreadId() const {
   // If the thread is created but not started yet, wait for |id_| being ready.
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
   id_event_.Wait();
   return id_;
 }
 
 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(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
   quit_properly = lazy_tls_bool.Pointer()->Get();
 #endif
   return quit_properly;
 }
 
 void Thread::ThreadMain() {
   // First, make GetThreadId() available to avoid deadlocks. It could be called
   // any place in the following thread initialization code.
   id_ = PlatformThread::CurrentId();
   DCHECK_NE(kInvalidThreadId, id_);
   id_event_.Signal();
 
   // Complete the initialization of our Thread object.
   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();
   message_loop_->SetTimerSlack(message_loop_timer_slack_);
 
 #if defined(OS_WIN)
   std::unique_ptr<win::ScopedCOMInitializer> com_initializer;
   if (com_status_ != NONE) {
     com_initializer.reset((com_status_ == STA) ?
         new win::ScopedCOMInitializer() :
@@ skipping 40 matching lines @@
   run_loop_ = nullptr;
 }
 
 void Thread::ThreadQuitHelper() {
   DCHECK(run_loop_);
   run_loop_->QuitWhenIdle();
   SetThreadWasQuitProperly(true);
 }
 
 }  // namespace base