Modernize base::Thread

base/threading/thread.h

Index: base/threading/thread.h
diff --git a/base/threading/thread.h b/base/threading/thread.h
index f68154e12d25c8d85fbf02ea8ae1d7f7a546d0c6..1652a5e85ec636bc2fa12ea81dd5db25d5b94958 100644
--- a/base/threading/thread.h
+++ b/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 it's 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,37 @@ 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 owner, this can also safely be called from the

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

"Thread's owning sequence" sounds more strictly correct. "owner" sounds more
like memory ownership ie via a unique_ptr.

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

Done.

+  // 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().
+    DCHECK(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 owner, this can also safely be called from the

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

ditto "owner"

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

Done.

+  // underlying thread itself.
   scoped_refptr<SingleThreadTaskRunner> task_runner() const {
+    // Ref. comment on |message_loop()| DCHECK.

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

"Refer to the DCHECK and comment inside |message_loop()|.

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

Done.

+    DCHECK(sequence_checker_.CalledOnValidSequencedThread() ||
+           id_ == PlatformThread::CurrentId() || message_loop_)
+        << id_ << " vs " << PlatformThread::CurrentId();
     return message_loop_ ? message_loop_->task_runner() : nullptr;
   }
 
@@ -179,6 +207,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 +227,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   static bool GetThreadWasQuitProperly();
 
   void set_message_loop(MessageLoop* message_loop) {
+    DCHECK(sequence_checker_.CalledOnValidSequencedThread());
     message_loop_ = message_loop;
   }
 
@@ -217,42 +247,44 @@ 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.
   PlatformThreadHandle thread_;
-  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_;

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

Yay const

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

:-)

 
   // Signaled when the created thread gets ready to use the message loop.
   mutable WaitableEvent start_event_;
 
+  // This class is not thread-safe.

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

...thread-safe, use this to verify access from the owning sequence of the
Thread.

+  SequenceChecker sequence_checker_;

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

nit: maybe call this owning_sequence_checker_? or something u like better, but i
like when checkers say which sequence/thread they are checking for.

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

Done.

+
   DISALLOW_COPY_AND_ASSIGN(Thread);
 };