Revert of Reland: Lazily initialize MessageLoop for faster thread startup (patchset #5 id:160001 of https://codereview.chromium.org/1129953004/)

Reason for revert:
Massive data race reports, see https://crbug.com/489263

Original issue's description:
> Reland: Lazily initialize MessageLoop for faster thread startup
>
> Original review: https://codereview.chromium.org/1011683002/
>
> Reverted because it's suspected for following flakiness issues:
> http://crbug.com/485157 - Windows race
> http://crbug.com/485091 - Android ThreadWatcher
> http://crbug.com/485178 - interactive_ui_tests Menu* tests
>
> PS1 is the original patch set that gets reverted.
>
> BUG=465458, 485157, 485091, 485178
> TBR=jam
>
> Committed: https://crrev.com/8b6133a69f16702a32a3c3104630c4d9ac393b7a
> Cr-Commit-Position: refs/heads/master@{#330329}

TBR=thakis@chromium.org,toyoshim@chromium.org,jam@chromium.org,kinuko@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=465458, 485157, 485091, 485178

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

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

6 files changed, 103 insertions, 134 deletions

diff --git a/base/threading/platform_thread.h b/base/threading/platform_thread.h
index 69a2b0d..d8f06e5 100644
--- a/base/threading/platform_thread.h
+++ b/base/threading/platform_thread.h
@@ -89,10 +89,6 @@ 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 aeaa7c7..4eb2cb2 100644
--- a/base/threading/platform_thread_win.cc
+++ b/base/threading/platform_thread_win.cc
@@ -108,16 +108,15 @@ 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, &thread_id);
+      NULL, stack_size, ThreadFunc, params, flags, NULL);
   if (!thread_handle) {
     delete params;
     return false;
   }
 
   if (out_thread_handle)
-    *out_thread_handle = PlatformThreadHandle(thread_handle, thread_id);
+    *out_thread_handle = PlatformThreadHandle(thread_handle);
   else
     CloseHandle(thread_handle);
   return true;
diff --git a/base/threading/thread.cc b/base/threading/thread.cc
index b753a6b..0e4aab2 100644
--- a/base/threading/thread.cc
+++ b/base/threading/thread.cc
@@ -37,6 +37,20 @@ 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),
@@ -58,11 +72,13 @@ 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_(nullptr),
-      message_loop_timer_slack_(TIMER_SLACK_NONE),
+      message_loop_(NULL),
+      thread_id_(kInvalidThreadId),
       name_(name) {
 }
 
@@ -88,50 +104,34 @@ bool Thread::StartWithOptions(const Options& options) {
 
   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;
-  message_loop_ = new MessageLoop(type, options.message_pump_factory);
-
-  start_event_.reset(new WaitableEvent(false, false));
+  StartupData startup_data(options);
+  startup_data_ = &startup_data;
 
-  // 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;
-    }
+  if (!PlatformThread::Create(options.stack_size, this, &thread_)) {
+    DLOG(ERROR) << "failed to create thread";
+    startup_data_ = NULL;
+    return false;
   }
 
-  DCHECK(message_loop_);
-  return true;
-}
-
-bool Thread::StartAndWaitForTesting() {
-  bool result = Start();
-  if (!result)
-    return false;
-  WaitUntilThreadStarted();
-  return true;
-}
+  // 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)"));
 
-bool Thread::WaitUntilThreadStarted() {
-  if (!start_event_)
-    return false;
+  // Wait for the thread to start and initialize message_loop_
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
-  start_event_->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;
+
+  DCHECK(message_loop_);
   return true;
 }
 
 void Thread::Stop() {
-  if (!start_event_)
+  if (!started_)
     return;
 
   StopSoon();
@@ -147,7 +147,7 @@ void Thread::Stop() {
   DCHECK(!message_loop_);
 
   // The thread no longer needs to be joined.
-  start_event_.reset();
+  started_ = false;
 
   stopping_ = false;
 }
@@ -155,7 +155,9 @@ void Thread::Stop() {
 void Thread::StopSoon() {
   // We should only be called on the same thread that started us.
 
-  DCHECK_NE(thread_id(), PlatformThread::CurrentId());
+  // 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());
 
   if (stopping_ || !message_loop_)
     return;
@@ -164,28 +166,14 @@ 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(message_loop_ != nullptr);
+  DCHECK_NE(thread_id_, kInvalidThreadId);
   PlatformThread::SetThreadPriority(thread_, priority);
 }
 
@@ -206,60 +194,60 @@ bool Thread::GetThreadWasQuitProperly() {
 }
 
 void Thread::ThreadMain() {
-  // Complete the initialization of our Thread object.
-  PlatformThread::SetName(name_.c_str());
-  ANNOTATE_THREAD_NAME(name_.c_str());  // Tell the name to race detector.
+  {
+    // 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));
+    }
 
-  // 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_);
+    // 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();
 
 #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
 
-  // 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.
+    // Let's do this before signaling we are started.
+    Init();
 
-  // Let the thread do extra initialization.
-  Init();
-
-  {
-    AutoLock lock(running_lock_);
     running_ = true;
-  }
-
-  start_event_->Signal();
+    startup_data_->event.Signal();
+    // startup_data_ can't be touched anymore since the starting thread is now
+    // unlocked.
 
-  Run(message_loop_);
-
-  {
-    AutoLock lock(running_lock_);
+    Run(message_loop_);
     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.
-  // (The message loop is destructed at the end of this block)
-  message_loop_ = NULL;
+    // We can't receive messages anymore.
+    message_loop_ = NULL;
+  }
 }
 
 }  // namespace base
diff --git a/base/threading/thread.h b/base/threading/thread.h
index 6b2f1c5..4915606 100644
--- a/base/threading/thread.h
+++ b/base/threading/thread.h
@@ -13,13 +13,11 @@
 #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
@@ -47,7 +45,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
     // This is ignored if message_pump_factory.is_null() is false.
     MessageLoop::Type message_loop_type;
 
-    // Specifies timer slack for thread message loop.
+    // Specify timer slack for thread message loop.
     TimerSlack timer_slack;
 
     // Used to create the MessagePump for the MessageLoop. The callback is Run()
@@ -83,7 +81,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(!start_event_);
+    DCHECK(!started_);
     com_status_ = use_mta ? MTA : STA;
   }
 #endif
@@ -105,18 +103,6 @@ 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).
@@ -180,7 +166,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   PlatformThreadHandle thread_handle() { return thread_; }
 
   // The thread ID.
-  PlatformThreadId thread_id() const;
+  PlatformThreadId thread_id() const { return thread_id_; }
 
   // Returns true if the thread has been started, and not yet stopped.
   bool IsRunning() const;
@@ -222,32 +208,33 @@ 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_;
-  mutable base::Lock running_lock_;  // Protects running_.
+
+  // Used to pass data to ThreadMain.
+  struct StartupData;
+  StartupData* startup_data_;
 
   // 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_;
 
-  // Stores Options::timer_slack_ until the message loop has been bound to
-  // a thread.
-  TimerSlack message_loop_timer_slack_;
+  // Our thread's ID.
+  PlatformThreadId thread_id_;
 
   // 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 b17c681..b5953d5 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.StartAndWaitForTesting();
-  thread_b.StartAndWaitForTesting();
+  thread_a.Start();
+  thread_b.Start();
 
   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.StartAndWaitForTesting();
+  thread_a.Start();
   {
     base::Thread thread_b(kBThread);
-    thread_b.StartAndWaitForTesting();
+    thread_b.Start();
     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.StartAndWaitForTesting();
+  thread_a.Start();
   base::PlatformThreadId a_id = thread_a.thread_id();
   EXPECT_STREQ(kAThread, manager->GetName(a_id));
   thread_a.Stop();
 
-  thread_a.StartAndWaitForTesting();
+  thread_a.Start();
   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 e86c758..a89768e 100644
--- a/base/threading/thread_unittest.cc
+++ b/base/threading/thread_unittest.cc
@@ -194,12 +194,11 @@ TEST_F(ThreadTest, ThreadName) {
   EXPECT_EQ("ThreadName", a.thread_name());
 }
 
-// Make sure Init() is called after Start() and before
-// WaitUntilThreadInitialized() returns.
+// Make sure we can't use a thread between Start() and Init().
 TEST_F(ThreadTest, SleepInsideInit) {
   SleepInsideInitThread t;
   EXPECT_FALSE(t.InitCalled());
-  t.StartAndWaitForTesting();
+  t.Start();
   EXPECT_TRUE(t.InitCalled());
 }