PlatformThreadHandle: remove public id() interface

Remove PlatformThreadHandle.id() interface.
Also remove id_ member that is not needed any more.

BUG=468793
TEST=./out/Debug/base_unittests
TEST=git cl try

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

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

8 files changed, 103 insertions, 53 deletions

diff --git a/base/threading/platform_thread.h b/base/threading/platform_thread.h
index 7b076a2..6b52cc4 100644
--- a/base/threading/platform_thread.h
+++ b/base/threading/platform_thread.h
@@ -73,26 +73,9 @@ class PlatformThreadHandle {
   typedef pthread_t Handle;
 #endif
 
-  PlatformThreadHandle()
-      : handle_(0),
-        id_(0) {
-  }
-
-  explicit PlatformThreadHandle(Handle handle)
-      : handle_(handle),
-        id_(0) {
-  }
+  PlatformThreadHandle() : handle_(0) {}
 
-  PlatformThreadHandle(Handle handle,
-                       PlatformThreadId id)
-      : handle_(handle),
-        id_(id) {
-  }
-
-  // TODO(toyoshim): Remove id() and use PlatformThread::CurrentId() instead.
-  PlatformThreadId id() const {
-    return id_;
-  }
+  explicit PlatformThreadHandle(Handle handle) : handle_(handle) {}
 
   bool is_equal(const PlatformThreadHandle& other) const {
     return handle_ == other.handle_;
@@ -108,7 +91,6 @@ class PlatformThreadHandle {
 
  private:
   Handle handle_;
-  PlatformThreadId id_;
 };
 
 const PlatformThreadId kInvalidThreadId(0);
diff --git a/base/threading/platform_thread_posix.cc b/base/threading/platform_thread_posix.cc
index 50c3357..44b691c 100644
--- a/base/threading/platform_thread_posix.cc
+++ b/base/threading/platform_thread_posix.cc
@@ -63,8 +63,7 @@ void* ThreadFunc(void* params) {
 
   // Stash the id in the handle so the calling thread has a complete
   // handle, and unblock the parent thread.
-  *(thread_params->handle) = PlatformThreadHandle(pthread_self(),
-                                                  PlatformThread::CurrentId());
+  *(thread_params->handle) = PlatformThreadHandle(pthread_self());
   thread_params->handle_set.Signal();
 
   ThreadIdNameManager::GetInstance()->RegisterThread(
@@ -165,7 +164,7 @@ PlatformThreadRef PlatformThread::CurrentRef() {
 
 // static
 PlatformThreadHandle PlatformThread::CurrentHandle() {
-  return PlatformThreadHandle(pthread_self(), CurrentId());
+  return PlatformThreadHandle(pthread_self());
 }
 
 // static
diff --git a/base/threading/platform_thread_win.cc b/base/threading/platform_thread_win.cc
index 0f7241a..25973bc 100644
--- a/base/threading/platform_thread_win.cc
+++ b/base/threading/platform_thread_win.cc
@@ -87,12 +87,12 @@ DWORD __stdcall ThreadFunc(void* params) {
         PlatformThread::CurrentId());
   }
 
-  return NULL;
+  return 0;
 }
 
 // CreateThreadInternal() matches PlatformThread::CreateWithPriority(), except
-// that |out_thread_handle| may be NULL, in which case a non-joinable thread is
-// created.
+// that |out_thread_handle| may be nullptr, in which case a non-joinable thread
+// is created.
 bool CreateThreadInternal(size_t stack_size,
                           PlatformThread::Delegate* delegate,
                           PlatformThreadHandle* out_thread_handle,
@@ -106,7 +106,7 @@ bool CreateThreadInternal(size_t stack_size,
 
   ThreadParams* params = new ThreadParams;
   params->delegate = delegate;
-  params->joinable = out_thread_handle != NULL;
+  params->joinable = out_thread_handle != nullptr;
   params->priority = priority;
 
   // Using CreateThread here vs _beginthreadex makes thread creation a bit
@@ -114,16 +114,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);
+  void* thread_handle =
+      ::CreateThread(nullptr, stack_size, ThreadFunc, params, flags, nullptr);
   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;
@@ -198,8 +197,8 @@ bool PlatformThread::CreateWithPriority(size_t stack_size, Delegate* delegate,
 
 // static
 bool PlatformThread::CreateNonJoinable(size_t stack_size, Delegate* delegate) {
-  return CreateThreadInternal(
-      stack_size, delegate, NULL, ThreadPriority::NORMAL);
+  return CreateThreadInternal(stack_size, delegate, nullptr,
+                              ThreadPriority::NORMAL);
 }
 
 // static
diff --git a/base/threading/thread.cc b/base/threading/thread.cc
index cf1d0b6..900b1a5 100644
--- a/base/threading/thread.cc
+++ b/base/threading/thread.cc
@@ -62,6 +62,8 @@ Thread::Thread(const std::string& name)
       stopping_(false),
       running_(false),
       thread_(0),
+      id_(kInvalidThreadId),
+      id_event_(true, false),
       message_loop_(nullptr),
       message_loop_timer_slack_(TIMER_SLACK_NONE),
       name_(name) {
@@ -87,6 +89,10 @@ bool Thread::StartWithOptions(const Options& options) {
       (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;
@@ -161,7 +167,7 @@ void Thread::Stop() {
 void Thread::StopSoon() {
   // We should only be called on the same thread that started us.
 
-  DCHECK_NE(thread_id(), PlatformThread::CurrentId());
+  DCHECK_NE(GetThreadId(), PlatformThread::CurrentId());
 
   if (stopping_ || !message_loop_)
     return;
@@ -170,9 +176,11 @@ void Thread::StopSoon() {
   task_runner()->PostTask(FROM_HERE, base::Bind(&ThreadQuitHelper));
 }
 
-PlatformThreadId Thread::thread_id() const {
-  AutoLock lock(thread_lock_);
-  return thread_.id();
+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 {
@@ -205,6 +213,12 @@ bool Thread::GetThreadWasQuitProperly() {
 }
 
 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.
@@ -225,10 +239,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();
 
diff --git a/base/threading/thread.h b/base/threading/thread.h
index 5126491..d69af84 100644
--- a/base/threading/thread.h
+++ b/base/threading/thread.h
@@ -14,12 +14,12 @@
 #include "base/message_loop/timer_slack.h"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.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
@@ -170,8 +170,13 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   // The native thread handle.
   PlatformThreadHandle thread_handle() { return thread_; }
 
-  // The thread ID.
-  PlatformThreadId thread_id() const;
+  // Returns the thread ID.  Should not be called before the first Start*()
+  // call.  Keeps on returning the same ID even after a Stop() call. The next
+  // Start*() call renews the ID.
+  //
+  // WARNING: This function will block if the thread hasn't started yet.
+  //
+  PlatformThreadId GetThreadId() const;
 
   // Returns true if the thread has been started, and not yet stopped.
   bool IsRunning() const;
@@ -216,11 +221,15 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
 
   // True while inside of Run().
   bool running_;
-  mutable base::Lock running_lock_;  // Protects running_.
+  mutable base::Lock running_lock_;  // Protects |running_|.
 
   // The thread's handle.
   PlatformThreadHandle thread_;
-  mutable base::Lock thread_lock_;  // Protects thread_.
+  mutable base::Lock thread_lock_;  // Protects |thread_|.
+
+  // The thread's id once it has started.
+  PlatformThreadId id_;
+  mutable WaitableEvent id_event_;  // Protects |id_|.
 
   // The thread's message loop.  Valid only while the thread is alive.  Set
   // by the created thread.
diff --git a/base/threading/thread_id_name_manager_unittest.cc b/base/threading/thread_id_name_manager_unittest.cc
index b17c681..350dc0f 100644
--- a/base/threading/thread_id_name_manager_unittest.cc
+++ b/base/threading/thread_id_name_manager_unittest.cc
@@ -24,8 +24,8 @@ TEST_F(ThreadIdNameManagerTest, AddThreads) {
   thread_a.StartAndWaitForTesting();
   thread_b.StartAndWaitForTesting();
 
-  EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id()));
-  EXPECT_STREQ(kBThread, manager->GetName(thread_b.thread_id()));
+  EXPECT_STREQ(kAThread, manager->GetName(thread_a.GetThreadId()));
+  EXPECT_STREQ(kBThread, manager->GetName(thread_b.GetThreadId()));
 
   thread_b.Stop();
   thread_a.Stop();
@@ -41,10 +41,10 @@ TEST_F(ThreadIdNameManagerTest, RemoveThreads) {
     thread_b.StartAndWaitForTesting();
     thread_b.Stop();
   }
-  EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id()));
+  EXPECT_STREQ(kAThread, manager->GetName(thread_a.GetThreadId()));
 
   thread_a.Stop();
-  EXPECT_STREQ("", manager->GetName(thread_a.thread_id()));
+  EXPECT_STREQ("", manager->GetName(thread_a.GetThreadId()));
 }
 
 TEST_F(ThreadIdNameManagerTest, RestartThread) {
@@ -52,13 +52,13 @@ TEST_F(ThreadIdNameManagerTest, RestartThread) {
   base::Thread thread_a(kAThread);
 
   thread_a.StartAndWaitForTesting();
-  base::PlatformThreadId a_id = thread_a.thread_id();
+  base::PlatformThreadId a_id = thread_a.GetThreadId();
   EXPECT_STREQ(kAThread, manager->GetName(a_id));
   thread_a.Stop();
 
   thread_a.StartAndWaitForTesting();
   EXPECT_STREQ("", manager->GetName(a_id));
-  EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id()));
+  EXPECT_STREQ(kAThread, manager->GetName(thread_a.GetThreadId()));
   thread_a.Stop();
 }
 
diff --git a/base/threading/thread_unittest.cc b/base/threading/thread_unittest.cc
index 3c35416..9422081 100644
--- a/base/threading/thread_unittest.cc
+++ b/base/threading/thread_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
@@ -105,6 +106,15 @@ void RegisterDestructionObserver(
   base::MessageLoop::current()->AddDestructionObserver(observer);
 }
 
+// Task that calls GetThreadId() of |thread|, stores the result into |id|, then
+// signal |event|.
+void ReturnThreadId(base::Thread* thread,
+                    base::PlatformThreadId* id,
+                    base::WaitableEvent* event) {
+  *id = thread->GetThreadId();
+  event->Signal();
+}
+
 }  // namespace
 
 TEST_F(ThreadTest, Restart) {
@@ -194,6 +204,47 @@ TEST_F(ThreadTest, ThreadName) {
   EXPECT_EQ("ThreadName", a.thread_name());
 }
 
+TEST_F(ThreadTest, ThreadId) {
+  Thread a("ThreadId0");
+  Thread b("ThreadId1");
+  a.Start();
+  b.Start();
+
+  // Post a task that calls GetThreadId() on the created thread.
+  base::WaitableEvent event(false, false);
+  base::PlatformThreadId id_from_new_thread;
+  a.task_runner()->PostTask(
+      FROM_HERE, base::Bind(ReturnThreadId, &a, &id_from_new_thread, &event));
+
+  // Call GetThreadId() on the current thread before calling event.Wait() so
+  // that this test can find a race issue with TSAN.
+  base::PlatformThreadId id_from_current_thread = a.GetThreadId();
+
+  // Check if GetThreadId() returns consistent value in both threads.
+  event.Wait();
+  EXPECT_EQ(id_from_current_thread, id_from_new_thread);
+
+  // A started thread should have a valid ID.
+  EXPECT_NE(base::kInvalidThreadId, a.GetThreadId());
+  EXPECT_NE(base::kInvalidThreadId, b.GetThreadId());
+
+  // Each thread should have a different thread ID.
+  EXPECT_NE(a.GetThreadId(), b.GetThreadId());
+}
+
+TEST_F(ThreadTest, ThreadIdWithRestart) {
+  Thread a("ThreadIdWithRestart");
+  base::PlatformThreadId previous_id = base::kInvalidThreadId;
+
+  for (size_t i = 0; i < 16; ++i) {
+    EXPECT_TRUE(a.Start());
+    base::PlatformThreadId current_id = a.GetThreadId();
+    EXPECT_NE(previous_id, current_id);
+    previous_id = current_id;
+    a.Stop();
+  }
+}
+
 // Make sure Init() is called after Start() and before
 // WaitUntilThreadInitialized() returns.
 TEST_F(ThreadTest, SleepInsideInit) {
diff --git a/base/trace_event/trace_event_unittest.cc b/base/trace_event/trace_event_unittest.cc
index afee8a9..bf389d6 100644
--- a/base/trace_event/trace_event_unittest.cc
+++ b/base/trace_event/trace_event_unittest.cc
@@ -1549,7 +1549,7 @@ TEST_F(TraceEventTestFixture, ThreadNames) {
   for (int i = 0; i < kNumThreads; i++) {
     task_complete_events[i] = new WaitableEvent(false, false);
     threads[i]->Start();
-    thread_ids[i] = threads[i]->thread_id();
+    thread_ids[i] = threads[i]->GetThreadId();
     threads[i]->task_runner()->PostTask(
         FROM_HERE, base::Bind(&TraceManyInstantEvents, i, kNumEvents,
                               task_complete_events[i]));