Port last remaining test case in base/stats_table_unittest.cc, and

fix the bug it exposes on posix in StatsTable, i.e.
UnregisterThread() breaks when called inside SlotReturnFunction()
on posix because posix clears tls data before calling destructor.

Review URL: http://codereview.chromium.org/8751

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@4309 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 45 insertions, 39 deletions

diff --git a/base/stats_table.cc b/base/stats_table.cc
index 7c55204..47bc533 100644
--- a/base/stats_table.cc
+++ b/base/stats_table.cc
@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/platform_thread.h"
+#include "base/process_util.h"
 #include "base/shared_memory.h"
 #include "base/string_util.h"
 #include "base/thread_local_storage.h"
@@ -91,16 +92,6 @@ struct StatsTableTLSData {
   int slot;
 };
 
-// The SlotReturnFunction is called at thread exit for each thread
-// which used the StatsTable.
-static void SlotReturnFunction(void* data) {
-  StatsTableTLSData* tls_data = static_cast<StatsTableTLSData*>(data);
-  if (tls_data) {
-    DCHECK(tls_data->table);
-    tls_data->table->UnregisterThread();
-  }
-}
-
 }  // namespace
 
 // The StatsTablePrivate maintains convenience pointers into the
@@ -313,12 +304,7 @@ int StatsTable::RegisterThread(const std::wstring& name) {
     base::wcslcpy(impl_->thread_name(slot), thread_name.c_str(),
                   kMaxThreadNameLength);
     *(impl_->thread_tid(slot)) = PlatformThread::CurrentId();
-    // TODO(pinkerton): these should go into process_utils when it's ported
-#if defined(OS_WIN)
-    *(impl_->thread_pid(slot)) = GetCurrentProcessId();
-#elif defined(OS_POSIX)
-    *(impl_->thread_pid(slot)) = getpid();
-#endif  
+    *(impl_->thread_pid(slot)) = process_util::GetCurrentProcId();
   }
 
   // Set our thread local storage.
@@ -341,7 +327,10 @@ StatsTableTLSData* StatsTable::GetTLSData() const {
 }
 
 void StatsTable::UnregisterThread() {
-  StatsTableTLSData* data = GetTLSData();
+  UnregisterThread(GetTLSData());
+}
+
+void StatsTable::UnregisterThread(StatsTableTLSData* data) {
   if (!data)
     return;
   DCHECK(impl_);
@@ -355,6 +344,14 @@ void StatsTable::UnregisterThread() {
   delete data;
 }
 
+void StatsTable::SlotReturnFunction(void* data) {
+  StatsTableTLSData* tls_data = static_cast<StatsTableTLSData*>(data);
+  if (tls_data) {
+    DCHECK(tls_data->table);
+    tls_data->table->UnregisterThread(tls_data);
+  }
+}
+
 int StatsTable::CountThreadsRegistered() const {
   if (!impl_)
     return 0;
diff --git a/base/stats_table.h b/base/stats_table.h
index 8ef529c..48da71a 100644
--- a/base/stats_table.h
+++ b/base/stats_table.h
@@ -76,11 +76,6 @@ class StatsTable {
   // returns 0.
   int RegisterThread(const std::wstring& name);
 
-  // Returns the space occupied by a thread in the table.  Generally used
-  // if a thread terminates but the process continues.  This function
-  // does not zero out the thread's counters.
-  void UnregisterThread();
-
   // Returns the number of threads currently registered.  This is really not
   // useful except for diagnostics and debugging.
   int CountThreadsRegistered() const;
@@ -137,6 +132,20 @@ class StatsTable {
   static int* FindLocation(const wchar_t *name);
 
  private:
+  // Returns the space occupied by a thread in the table.  Generally used
+  // if a thread terminates but the process continues.  This function
+  // does not zero out the thread's counters.
+  // Cannot be used inside a posix tls destructor.
+  void UnregisterThread();
+
+  // This variant expects the tls data to be passed in, so it is safe to
+  // call from inside a posix tls destructor (see doc for pthread_key_create).
+  void UnregisterThread(StatsTableTLSData* tls_data);
+
+  // The SlotReturnFunction is called at thread exit for each thread
+  // which used the StatsTable.
+  static void SlotReturnFunction(void* data);
+
   // Locates a free slot in the table.  Returns a number > 0 on success,
   // or 0 on failure.  The caller must hold the shared_memory lock when
   // calling this function.
diff --git a/base/stats_table_unittest.cc b/base/stats_table_unittest.cc
index aa7b2cb..87ad47f 100644
--- a/base/stats_table_unittest.cc
+++ b/base/stats_table_unittest.cc
@@ -66,13 +66,17 @@ const std::wstring kCounterMixed = L"CounterMixed";
 // The number of thread loops that we will do.
 const int kThreadLoops = 1000;
 
-// TODO(estade): port this test
-#if defined(OS_WIN)
-unsigned __stdcall StatsTableMultipleThreadMain(void* param) {
+class StatsTableThread : public PlatformThread::Delegate {
+public:
+  void ThreadMain();
+  PlatformThreadHandle thread_;
+  int id_;
+};
+
+void StatsTableThread::ThreadMain() {
   // Each thread will open the shared memory and set counters
   // concurrently in a loop.  We'll use some pauses to
   // mixup the thread scheduling.
-  int16 id = reinterpret_cast<int16>(param);
 
   StatsCounter zero_counter(kCounterZero);
   StatsCounter lucky13_counter(kCounter1313);
@@ -84,13 +88,12 @@ unsigned __stdcall StatsTableMultipleThreadMain(void* param) {
     lucky13_counter.Set(1313);
     increment_counter.Increment();
     decrement_counter.Decrement();
-    if (id % 2)
+    if (id_ % 2)
       mixed_counter.Decrement();
     else
       mixed_counter.Increment();
     PlatformThread::Sleep(index % 10);   // short wait
   }
-  return 0;
 }
 
 // Create a few threads and have them poke on their counters.
@@ -107,22 +110,20 @@ TEST_F(StatsTableTest, MultipleThreads) {
   // Spin up a set of threads to go bang on the various counters.
   // After we join the threads, we'll make sure the counters
   // contain the values we expected.
-  HANDLE threads[kMaxThreads];
+  StatsTableThread threads[kMaxThreads];
 
   // Spawn the threads.
-  for (int16 index = 0; index < kMaxThreads; index++) {
-    void* argument = reinterpret_cast<void*>(index);
-    unsigned thread_id;
-    threads[index] = reinterpret_cast<HANDLE>(
-      _beginthreadex(NULL, 0, StatsTableMultipleThreadMain, argument, 0,
-        &thread_id));
-    EXPECT_NE((HANDLE)NULL, threads[index]);
+  for (int index = 0; index < kMaxThreads; index++) {
+    threads[index].id_ = index; 
+    bool created = 
+        PlatformThread::Create(0, &threads[index], &threads[index].thread_);
+    EXPECT_EQ(true, created);
+    EXPECT_NE(static_cast<PlatformThreadHandle>(0), threads[index].thread_);
   }
 
   // Wait for the threads to finish.
   for (int index = 0; index < kMaxThreads; index++) {
-    DWORD rv = WaitForSingleObject(threads[index], 60 * 1000);
-    EXPECT_EQ(rv, WAIT_OBJECT_0);  // verify all threads finished
+    PlatformThread::Join(threads[index].thread_);
   }
   StatsCounter zero_counter(kCounterZero);
   StatsCounter lucky13_counter(kCounter1313);
@@ -148,7 +149,6 @@ TEST_F(StatsTableTest, MultipleThreads) {
       table.GetCounterValue(name));
   EXPECT_EQ(0, table.CountThreadsRegistered());
 }
-#endif  // defined(OS_WIN)
 
 const std::wstring kTableName = L"MultipleProcessStatTable";