PlatformThreadHandle: remove public id() interface

base/threading/thread.cc

Index: base/threading/thread.cc
diff --git a/base/threading/thread.cc b/base/threading/thread.cc
index 7bff24232e2bf2d66fda4485a4246ebae2e98332..56f0c80659d22871b9c9a97628a005a090ad6de8 100644
--- a/base/threading/thread.cc
+++ b/base/threading/thread.cc
@@ -62,6 +62,7 @@ Thread::Thread(const std::string& name)
       stopping_(false),
       running_(false),
       thread_(0),
+      id_(kInvalidThreadId),
       message_loop_(nullptr),
       message_loop_timer_slack_(TIMER_SLACK_NONE),
       name_(name) {
@@ -97,25 +98,15 @@ bool Thread::StartWithOptions(const Options& options) {
   scoped_ptr<MessageLoop> message_loop = MessageLoop::CreateUnbound(
       type, options.message_pump_factory);
   message_loop_ = message_loop.get();
-  start_event_.reset(new WaitableEvent(false, false));
-
-  // 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_);
-    bool created;
-    if (options.priority == ThreadPriority::NORMAL) {
-      created = PlatformThread::Create(options.stack_size, this, &thread_);
-    } else {
-      created = PlatformThread::CreateWithPriority(options.stack_size, this,
-                                                   &thread_, options.priority);
-    }
-    if (!created) {
-      DLOG(ERROR) << "failed to create thread";
-      message_loop_ = nullptr;
-      start_event_.reset();
-      return false;
-    }
+  id_ = kInvalidThreadId;
+  start_event_.reset(new WaitableEvent(true, false));
+
+  if (!PlatformThread::CreateWithPriority(options.stack_size, this, &thread_,
+                                          options.priority)) {
+    DLOG(ERROR) << "failed to create thread";
+    message_loop_ = nullptr;
+    start_event_.reset();
+    return false;
   }
 
   // The ownership of message_loop is managemed by the newly created thread
@@ -134,7 +125,7 @@ bool Thread::StartAndWaitForTesting() {
   return true;
 }
 
-bool Thread::WaitUntilThreadStarted() {
+bool Thread::WaitUntilThreadStarted() const {
   if (!start_event_)
     return false;
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
@@ -165,20 +156,21 @@ void Thread::Stop() {
 }
 
 void Thread::StopSoon() {
-  // We should only be called on the same thread that started us.
-
-  DCHECK_NE(thread_id(), PlatformThread::CurrentId());
-
   if (stopping_ || !message_loop_)
     return;
 
+  // We should only be called on the same thread that started us.
+  DCHECK_NE(thread_id(), PlatformThread::CurrentId());

[gab, 2015/07/16 17:19:33]

Why move this? DCHECKs enforcing the method's contract typically go at the top.

[Takashi Toyoshima, 2015/07/16 19:51:11]

Because without this fix, this assertion fires in a test that calls Start() and
Stop() repeatedly. But I have another right fix in the newer patch set, and this
is already reverted.

+
   stopping_ = true;
   task_runner()->PostTask(FROM_HERE, base::Bind(&ThreadQuitHelper));
 }
 
 PlatformThreadId Thread::thread_id() const {
-  AutoLock lock(thread_lock_);
-  return thread_.id();
+  WaitUntilThreadStarted();
+  // Could fire if the method is called before starting the thread.
+  DCHECK_NE(kInvalidThreadId, id_);
+  return id_;
 }
 
 bool Thread::IsRunning() const {
@@ -231,10 +223,6 @@ void Thread::ThreadMain() {
   }
 #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.
   Init();
 
@@ -243,6 +231,9 @@ void Thread::ThreadMain() {
     running_ = true;
   }
 
+  id_ = PlatformThread::CurrentId();

[gab, 2015/07/16 17:19:33]

You still need a lock to protect this even if all the reads are after the only
write.

Well, you don't actually need a "lock", you need a "memory barrier" to force
other threads to see the new value (even if they never read it explicitly they
could have cached a page that contained it) -- and a "lock" is the simplest
version of such a barrier in our codebase (plus in non-contentious scenarios,
which you are creating here, there is pretty much zero overhead to a lock).

[Takashi Toyoshima, 2015/07/16 19:51:10]

Yeah, I understand memory consistency. As I commented in the header,
start_event_->Signal() implicitly performs membar operation. So this should
work. But the problem here is the original code does not protect start_event_.
scoped_ptr<WaitableEvent> must need a lock to access the internal pointer, but
we set and reset the pointer on multiple threads without any lock.

The reason why I use a event instead of lock is simple. id_ could not be
available until the created thread actually starts running. So reading id_ needs
to wait for the start_event_ signaled anyway.

+  DCHECK_NE(kInvalidThreadId, id_);
+
   start_event_->Signal();
 
   Run(message_loop_);
@@ -264,7 +255,7 @@ void Thread::ThreadMain() {
 
   // We can't receive messages anymore.
   // (The message loop is destructed at the end of this block)
-  message_loop_ = NULL;
+  message_loop_ = nullptr;
 }
 
 }  // namespace base