Reland (3rd try): Lazily initialize MessageLoop for faster thread startup

Original review: https://codereview.chromium.org/1011683002/
2nd try: https://codereview.chromium.org/1129953004/

2nd try reverted due to race reports on Linux:
https://crbug.com/489263 Data races on valid_thread_id_ after r330329

This fixes:
- Race in MessageLoopProxyImpl by introducing lock
- Race in BrowserMainLoop/BrowserThreadImpl, where BrowserThread::CurrentlyOn()
  called on one of BrowserThreads tries to touch other thread's message_loop()
  via global thread table.

Reg: the latter race, the code flow that causes this race is like following:

  // On the main thread, we create all known browser threads:
  for (...) {
    {
      AutoLock lock(g_lock);
      g_threads[id] = new BrowserProcessSubThread();
    }
    // [A] This initializes the thread's message_loop, which causes a race
    // against [B] in the new code because new threads can start running
    // immediately.
    thread->StartWithOptions();
  }

  // On the new thread's main function, it calls CurrentlyOn() which does:
  {
    // [B] This touches other thread's Thread::message_loop.
    AutoLock lock(g_lock);
    return g_threads[other_thread_id] &&
           g_threads[other_thread_id]->message_loop() == MessageLoop::current();
  }

This was safe before because both message_loop initialization and the first
call to CurrentlyOn() on the new thread was done synchronously in
StartWithOptions() while the main thread was blocked. In the new code
new threads can start accessing message_loop() asynchronously while
the main thread's for loop is running.

PS1 is the original patch (2nd try) that got reverted.

BUG=465458, 489263

Review URL: https://codereview.chromium.org/1131513007

Cr-Commit-Position: refs/heads/master@{#331235}

6 files changed, 134 insertions, 103 deletions

diff --git a/base/threading/platform_thread.h b/base/threading/platform_thread.h
index d8f06e5..69a2b0d 100644
--- a/base/threading/platform_thread.h
+++ b/base/threading/platform_thread.h
@@ -89,6 +89,10 @@ class PlatformThreadHandle {
         id_(id) {
   }
 
+  PlatformThreadId id() const {
+    return id_;
+  }
+
   bool is_equal(const PlatformThreadHandle& other) const {
     return handle_ == other.handle_;
   }
diff --git a/base/threading/platform_thread_win.cc b/base/threading/platform_thread_win.cc
index 4eb2cb2..aeaa7c7 100644
--- a/base/threading/platform_thread_win.cc
+++ b/base/threading/platform_thread_win.cc
@@ -108,15 +108,16 @@ bool CreateThreadInternal(size_t stack_size,
   // have to work running on CreateThread() threads anyway, since we run code
   // on the Windows thread pool, etc.  For some background on the difference:
   //   http://www.microsoft.com/msj/1099/win32/win321099.aspx
+  PlatformThreadId thread_id;
   void* thread_handle = CreateThread(
-      NULL, stack_size, ThreadFunc, params, flags, NULL);
+      NULL, stack_size, ThreadFunc, params, flags, &thread_id);
   if (!thread_handle) {
     delete params;
     return false;
   }
 
   if (out_thread_handle)
-    *out_thread_handle = PlatformThreadHandle(thread_handle);
+    *out_thread_handle = PlatformThreadHandle(thread_handle, thread_id);
   else
     CloseHandle(thread_handle);
   return true;
diff --git a/base/threading/thread.cc b/base/threading/thread.cc
index 0e4aab2..b753a6b 100644
--- a/base/threading/thread.cc
+++ b/base/threading/thread.cc
@@ -37,20 +37,6 @@ void ThreadQuitHelper() {
   Thread::SetThreadWasQuitProperly(true);
 }
 
-// Used to pass data to ThreadMain.  This structure is allocated on the stack
-// from within StartWithOptions.
-struct Thread::StartupData {
-  // We get away with a const reference here because of how we are allocated.
-  const Thread::Options& options;
-
-  // Used to synchronize thread startup.
-  WaitableEvent event;
-
-  explicit StartupData(const Options& opt)
-      : options(opt),
-        event(false, false) {}
-};
-
 Thread::Options::Options()
     : message_loop_type(MessageLoop::TYPE_DEFAULT),
       timer_slack(TIMER_SLACK_NONE),
@@ -72,13 +58,11 @@ Thread::Thread(const std::string& name)
 #if defined(OS_WIN)
       com_status_(NONE),
 #endif
-      started_(false),
       stopping_(false),
       running_(false),
-      startup_data_(NULL),
       thread_(0),
-      message_loop_(NULL),
-      thread_id_(kInvalidThreadId),
+      message_loop_(nullptr),
+      message_loop_timer_slack_(TIMER_SLACK_NONE),
       name_(name) {
 }
 
@@ -104,34 +88,50 @@ bool Thread::StartWithOptions(const Options& options) {
 
   SetThreadWasQuitProperly(false);
 
-  StartupData startup_data(options);
-  startup_data_ = &startup_data;
+  MessageLoop::Type type = options.message_loop_type;
+  if (!options.message_pump_factory.is_null())
+    type = MessageLoop::TYPE_CUSTOM;
 
-  if (!PlatformThread::Create(options.stack_size, this, &thread_)) {
-    DLOG(ERROR) << "failed to create thread";
-    startup_data_ = NULL;
-    return false;
-  }
+  message_loop_timer_slack_ = options.timer_slack;
+  message_loop_ = new MessageLoop(type, options.message_pump_factory);
 
-  // TODO(kinuko): Remove once crbug.com/465458 is solved.
-  tracked_objects::ScopedTracker tracking_profile_wait(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "465458 base::Thread::StartWithOptions (Wait)"));
+  start_event_.reset(new WaitableEvent(false, false));
 
-  // Wait for the thread to start and initialize message_loop_
-  base::ThreadRestrictions::ScopedAllowWait allow_wait;
-  startup_data.event.Wait();
-
-  // set it to NULL so we don't keep a pointer to some object on the stack.
-  startup_data_ = NULL;
-  started_ = true;
+  // 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_);
+    if (!PlatformThread::Create(options.stack_size, this, &thread_)) {
+      DLOG(ERROR) << "failed to create thread";
+      delete message_loop_;
+      message_loop_ = nullptr;
+      start_event_.reset();
+      return false;
+    }
+  }
 
   DCHECK(message_loop_);
   return true;
 }
 
+bool Thread::StartAndWaitForTesting() {
+  bool result = Start();
+  if (!result)
+    return false;
+  WaitUntilThreadStarted();
+  return true;
+}
+
+bool Thread::WaitUntilThreadStarted() {
+  if (!start_event_)
+    return false;
+  base::ThreadRestrictions::ScopedAllowWait allow_wait;
+  start_event_->Wait();
+  return true;
+}
+
 void Thread::Stop() {
-  if (!started_)
+  if (!start_event_)
     return;
 
   StopSoon();
@@ -147,7 +147,7 @@ void Thread::Stop() {
   DCHECK(!message_loop_);
 
   // The thread no longer needs to be joined.
-  started_ = false;
+  start_event_.reset();
 
   stopping_ = false;
 }
@@ -155,9 +155,7 @@ void Thread::Stop() {
 void Thread::StopSoon() {
   // We should only be called on the same thread that started us.
 
-  // Reading thread_id_ without a lock can lead to a benign data race
-  // with ThreadMain, so we annotate it to stay silent under ThreadSanitizer.
-  DCHECK_NE(ANNOTATE_UNPROTECTED_READ(thread_id_), PlatformThread::CurrentId());
+  DCHECK_NE(thread_id(), PlatformThread::CurrentId());
 
   if (stopping_ || !message_loop_)
     return;
@@ -166,14 +164,28 @@ void Thread::StopSoon() {
   task_runner()->PostTask(FROM_HERE, base::Bind(&ThreadQuitHelper));
 }
 
+PlatformThreadId Thread::thread_id() const {
+  AutoLock lock(thread_lock_);
+  return thread_.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.)
+  if (message_loop_ && !stopping_)
+    return true;
+  // 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::SetPriority(ThreadPriority priority) {
   // The thread must be started (and id known) for this to be
   // compatible with all platforms.
-  DCHECK_NE(thread_id_, kInvalidThreadId);
+  DCHECK(message_loop_ != nullptr);
   PlatformThread::SetThreadPriority(thread_, priority);
 }
 
@@ -194,60 +206,60 @@ bool Thread::GetThreadWasQuitProperly() {
 }
 
 void Thread::ThreadMain() {
-  {
-    // The message loop for this thread.
-    // Allocated on the heap to centralize any leak reports at this line.
-    scoped_ptr<MessageLoop> message_loop;
-    if (!startup_data_->options.message_pump_factory.is_null()) {
-      message_loop.reset(
-          new MessageLoop(startup_data_->options.message_pump_factory.Run()));
-    } else {
-      message_loop.reset(
-          new MessageLoop(startup_data_->options.message_loop_type));
-    }
+  // Complete the initialization of our Thread object.
+  PlatformThread::SetName(name_.c_str());
+  ANNOTATE_THREAD_NAME(name_.c_str());  // Tell the name to race detector.
 
-    // Complete the initialization of our Thread object.
-    thread_id_ = PlatformThread::CurrentId();
-    PlatformThread::SetName(name_);
-    ANNOTATE_THREAD_NAME(name_.c_str());  // Tell the name to race detector.
-    message_loop->set_thread_name(name_);
-    message_loop->SetTimerSlack(startup_data_->options.timer_slack);
-    message_loop_ = message_loop.get();
+  // Lazily initialize the message_loop so that it can run on this thread.
+  DCHECK(message_loop_);
+  scoped_ptr<MessageLoop> message_loop(message_loop_);
+  message_loop_->BindToCurrentThread();
+  message_loop_->set_thread_name(name_);
+  message_loop_->SetTimerSlack(message_loop_timer_slack_);
 
 #if defined(OS_WIN)
-    scoped_ptr<win::ScopedCOMInitializer> com_initializer;
-    if (com_status_ != NONE) {
-      com_initializer.reset((com_status_ == STA) ?
-          new win::ScopedCOMInitializer() :
-          new win::ScopedCOMInitializer(win::ScopedCOMInitializer::kMTA));
-    }
+  scoped_ptr<win::ScopedCOMInitializer> com_initializer;
+  if (com_status_ != NONE) {
+    com_initializer.reset((com_status_ == STA) ?
+        new win::ScopedCOMInitializer() :
+        new win::ScopedCOMInitializer(win::ScopedCOMInitializer::kMTA));
+  }
 #endif
 
-    // Let the thread do extra initialization.
-    // Let's do this before signaling we are started.
-    Init();
+  // Make sure the thread_id() returns current thread.
+  // (This internally acquires lock against PlatformThread::Create)
+  DCHECK_EQ(thread_id(), PlatformThread::CurrentId());
 
+  // Let the thread do extra initialization.
+  Init();
+
+  {
+    AutoLock lock(running_lock_);
     running_ = true;
-    startup_data_->event.Signal();
-    // startup_data_ can't be touched anymore since the starting thread is now
-    // unlocked.
+  }
+
+  start_event_->Signal();
 
-    Run(message_loop_);
+  Run(message_loop_);
+
+  {
+    AutoLock lock(running_lock_);
     running_ = false;
+  }
 
-    // Let the thread do extra cleanup.
-    CleanUp();
+  // Let the thread do extra cleanup.
+  CleanUp();
 
 #if defined(OS_WIN)
-    com_initializer.reset();
+  com_initializer.reset();
 #endif
 
-    // Assert that MessageLoop::Quit was called by ThreadQuitHelper.
-    DCHECK(GetThreadWasQuitProperly());
+  // Assert that MessageLoop::Quit was called by ThreadQuitHelper.
+  DCHECK(GetThreadWasQuitProperly());
 
-    // We can't receive messages anymore.
-    message_loop_ = NULL;
-  }
+  // We can't receive messages anymore.
+  // (The message loop is destructed at the end of this block)
+  message_loop_ = NULL;
 }
 
 }  // namespace base
diff --git a/base/threading/thread.h b/base/threading/thread.h
index 4915606..6b2f1c5 100644
--- a/base/threading/thread.h
+++ b/base/threading/thread.h
@@ -13,11 +13,13 @@
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/timer_slack.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
 
 namespace base {
 
 class MessagePump;
+class WaitableEvent;
 
 // A simple thread abstraction that establishes a MessageLoop on a new thread.
 // The consumer uses the MessageLoop of the thread to cause code to execute on
@@ -45,7 +47,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
     // This is ignored if message_pump_factory.is_null() is false.
     MessageLoop::Type message_loop_type;
 
-    // Specify timer slack for thread message loop.
+    // Specifies timer slack for thread message loop.
     TimerSlack timer_slack;
 
     // Used to create the MessagePump for the MessageLoop. The callback is Run()
@@ -81,7 +83,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   // init_com_with_mta(false) and then StartWithOptions() with any message loop
   // type other than TYPE_UI.
   void init_com_with_mta(bool use_mta) {
-    DCHECK(!started_);
+    DCHECK(!start_event_);
     com_status_ = use_mta ? MTA : STA;
   }
 #endif
@@ -103,6 +105,18 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   // callback.
   bool StartWithOptions(const Options& options);
 
+  // Starts the thread and wait for the thread to start and run initialization
+  // before returning. It's same as calling Start() and then
+  // WaitUntilThreadStarted().
+  // Note that using this (instead of Start() or StartWithOptions() causes
+  // jank on the calling thread, should be used only in testing code.
+  bool StartAndWaitForTesting();
+
+  // Blocks until the thread starts running. Called within StartAndWait().
+  // Note that calling this causes jank on the calling thread, must be used
+  // carefully for production code.
+  bool WaitUntilThreadStarted();
+
   // Signals the thread to exit and returns once the thread has exited.  After
   // this method returns, the Thread object is completely reset and may be used
   // as if it were newly constructed (i.e., Start may be called again).
@@ -166,7 +180,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   PlatformThreadHandle thread_handle() { return thread_; }
 
   // The thread ID.
-  PlatformThreadId thread_id() const { return thread_id_; }
+  PlatformThreadId thread_id() const;
 
   // Returns true if the thread has been started, and not yet stopped.
   bool IsRunning() const;
@@ -208,33 +222,32 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   ComStatus com_status_;
 #endif
 
-  // Whether we successfully started the thread.
-  bool started_;
-
   // If true, we're in the middle of stopping, and shouldn't access
   // |message_loop_|. It may non-NULL and invalid.
   bool stopping_;
 
   // True while inside of Run().
   bool running_;
-
-  // Used to pass data to ThreadMain.
-  struct StartupData;
-  StartupData* startup_data_;
+  mutable base::Lock running_lock_;  // Protects running_.
 
   // The thread's handle.
   PlatformThreadHandle thread_;
+  mutable base::Lock thread_lock_;  // Protects thread_.
 
   // The thread's message loop.  Valid only while the thread is alive.  Set
   // by the created thread.
   MessageLoop* message_loop_;
 
-  // Our thread's ID.
-  PlatformThreadId thread_id_;
+  // Stores Options::timer_slack_ until the message loop has been bound to
+  // a thread.
+  TimerSlack message_loop_timer_slack_;
 
   // The name of the thread.  Used for debugging purposes.
   std::string name_;
 
+  // Non-null if the thread has successfully started.
+  scoped_ptr<WaitableEvent> start_event_;
+
   friend void ThreadQuitHelper();
 
   DISALLOW_COPY_AND_ASSIGN(Thread);
diff --git a/base/threading/thread_id_name_manager_unittest.cc b/base/threading/thread_id_name_manager_unittest.cc
index b5953d5..b17c681 100644
--- a/base/threading/thread_id_name_manager_unittest.cc
+++ b/base/threading/thread_id_name_manager_unittest.cc
@@ -21,8 +21,8 @@ TEST_F(ThreadIdNameManagerTest, AddThreads) {
   base::Thread thread_a(kAThread);
   base::Thread thread_b(kBThread);
 
-  thread_a.Start();
-  thread_b.Start();
+  thread_a.StartAndWaitForTesting();
+  thread_b.StartAndWaitForTesting();
 
   EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id()));
   EXPECT_STREQ(kBThread, manager->GetName(thread_b.thread_id()));
@@ -35,10 +35,10 @@ TEST_F(ThreadIdNameManagerTest, RemoveThreads) {
   base::ThreadIdNameManager* manager = base::ThreadIdNameManager::GetInstance();
   base::Thread thread_a(kAThread);
 
-  thread_a.Start();
+  thread_a.StartAndWaitForTesting();
   {
     base::Thread thread_b(kBThread);
-    thread_b.Start();
+    thread_b.StartAndWaitForTesting();
     thread_b.Stop();
   }
   EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id()));
@@ -51,12 +51,12 @@ TEST_F(ThreadIdNameManagerTest, RestartThread) {
   base::ThreadIdNameManager* manager = base::ThreadIdNameManager::GetInstance();
   base::Thread thread_a(kAThread);
 
-  thread_a.Start();
+  thread_a.StartAndWaitForTesting();
   base::PlatformThreadId a_id = thread_a.thread_id();
   EXPECT_STREQ(kAThread, manager->GetName(a_id));
   thread_a.Stop();
 
-  thread_a.Start();
+  thread_a.StartAndWaitForTesting();
   EXPECT_STREQ("", manager->GetName(a_id));
   EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id()));
   thread_a.Stop();
diff --git a/base/threading/thread_unittest.cc b/base/threading/thread_unittest.cc
index a89768e..e86c758 100644
--- a/base/threading/thread_unittest.cc
+++ b/base/threading/thread_unittest.cc
@@ -194,11 +194,12 @@ TEST_F(ThreadTest, ThreadName) {
   EXPECT_EQ("ThreadName", a.thread_name());
 }
 
-// Make sure we can't use a thread between Start() and Init().
+// Make sure Init() is called after Start() and before
+// WaitUntilThreadInitialized() returns.
 TEST_F(ThreadTest, SleepInsideInit) {
   SleepInsideInitThread t;
   EXPECT_FALSE(t.InitCalled());
-  t.Start();
+  t.StartAndWaitForTesting();
   EXPECT_TRUE(t.InitCalled());
 }