Avoid threading races on TraceSamplingThread's members

The original CL was landed in https://codereview.chromium.org/26541005/
but the CL caused a dead lock. This CL fixed the issue.

NOTRY=true

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

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

base/debug/trace_event_impl.cc

@@ -838,7 +838,7 @@ class TraceSamplingThread : public PlatformThread::Delegate {
   static void DefaultSamplingCallback(TraceBucketData* bucekt_data);
 
   void Stop();
-  void InstallWaitableEventForSamplingTesting(WaitableEvent* waitable_event);
+  void WaitSamplingEventForTesting();
 
  private:
   friend class TraceLog;
@@ -855,14 +855,14 @@ class TraceSamplingThread : public PlatformThread::Delegate {
                                      const char** name);
   std::vector<TraceBucketData> sample_buckets_;
   bool thread_running_;
-  scoped_ptr<CancellationFlag> cancellation_flag_;
-  scoped_ptr<WaitableEvent> waitable_event_for_testing_;
+  CancellationFlag cancellation_flag_;
+  WaitableEvent waitable_event_for_testing_;
 };
 
 
 TraceSamplingThread::TraceSamplingThread()
-    : thread_running_(false) {
-  cancellation_flag_.reset(new CancellationFlag);
+    : thread_running_(false),
+      waitable_event_for_testing_(false, false) {
 }
 
 TraceSamplingThread::~TraceSamplingThread() {
@@ -872,12 +872,11 @@ void TraceSamplingThread::ThreadMain() {
   PlatformThread::SetName("Sampling Thread");
   thread_running_ = true;
   const int kSamplingFrequencyMicroseconds = 1000;
-  while (!cancellation_flag_->IsSet()) {
+  while (!cancellation_flag_.IsSet()) {
     PlatformThread::Sleep(
         TimeDelta::FromMicroseconds(kSamplingFrequencyMicroseconds));
     GetSamples();
-    if (waitable_event_for_testing_.get())
-      waitable_event_for_testing_->Signal();
+    waitable_event_for_testing_.Signal();
   }
 }
 
@@ -909,6 +908,9 @@ void TraceSamplingThread::RegisterSampleBucket(
     TRACE_EVENT_API_ATOMIC_WORD* bucket,
     const char* const name,
     TraceSampleCallback callback) {
+  // Access to sample_buckets_ doesn't cause races with the sampling thread
+  // that uses the sample_buckets_, because it is guaranteed that
+  // RegisterSampleBucket is called before the sampling thread is created.
   DCHECK(!thread_running_);
   sample_buckets_.push_back(TraceBucketData(bucket, name, callback));
 }
@@ -922,12 +924,11 @@ void TraceSamplingThread::ExtractCategoryAndName(const char* combined,
 }
 
 void TraceSamplingThread::Stop() {
-  cancellation_flag_->Set();
+  cancellation_flag_.Set();
 }
 
-void TraceSamplingThread::InstallWaitableEventForSamplingTesting(
-    WaitableEvent* waitable_event) {
-  waitable_event_for_testing_.reset(waitable_event);
+void TraceSamplingThread::WaitSamplingEventForTesting() {
+  waitable_event_for_testing_.Wait();
 }
 
 TraceBucketData::TraceBucketData(base::subtle::AtomicWord* bucket,
@@ -2027,11 +2028,10 @@ void TraceLog::AddMetadataEventsWhileLocked() {
   }
 }
 
-void TraceLog::InstallWaitableEventForSamplingTesting(
-    WaitableEvent* waitable_event) {
+void TraceLog::WaitSamplingEventForTesting() {
   if (!sampling_thread_)
     return;
-  sampling_thread_->InstallWaitableEventForSamplingTesting(waitable_event);
+  sampling_thread_->WaitSamplingEventForTesting();
 }
 
 void TraceLog::DeleteForTesting() {

base/debug/trace_event_impl.h

@@ -540,7 +540,7 @@ class BASE_EXPORT TraceLog {
 
   // Exposed for unittesting:
 
-  void InstallWaitableEventForSamplingTesting(WaitableEvent* waitable_event);
+  void WaitSamplingEventForTesting();
 
   // Allows deleting our singleton instance.
   static void DeleteForTesting();

base/debug/trace_event_unittest.cc

@@ -1756,13 +1756,10 @@ TEST_F(TraceEventTestFixture, TraceSampling) {
       TraceLog::Options(TraceLog::RECORD_UNTIL_FULL |
                         TraceLog::ENABLE_SAMPLING));
 
-  WaitableEvent* sampled = new WaitableEvent(false, false);
-  TraceLog::GetInstance()->InstallWaitableEventForSamplingTesting(sampled);
-
   TRACE_EVENT_SET_SAMPLING_STATE_FOR_BUCKET(1, "cc", "Stuff");
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
   TRACE_EVENT_SET_SAMPLING_STATE_FOR_BUCKET(1, "cc", "Things");
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
 
   EndTraceAndFlush();
 
@@ -1778,32 +1775,29 @@ TEST_F(TraceEventTestFixture, TraceSamplingScope) {
     TraceLog::Options(TraceLog::RECORD_UNTIL_FULL |
                       TraceLog::ENABLE_SAMPLING));
 
-  WaitableEvent* sampled = new WaitableEvent(false, false);
-  TraceLog::GetInstance()->InstallWaitableEventForSamplingTesting(sampled);
-
   TRACE_EVENT_SCOPED_SAMPLING_STATE("AAA", "name");
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
   {
     EXPECT_STREQ(TRACE_EVENT_GET_SAMPLING_STATE(), "AAA");
     TRACE_EVENT_SCOPED_SAMPLING_STATE("BBB", "name");
-    sampled->Wait();
+    TraceLog::GetInstance()->WaitSamplingEventForTesting();
     EXPECT_STREQ(TRACE_EVENT_GET_SAMPLING_STATE(), "BBB");
   }
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
   {
     EXPECT_STREQ(TRACE_EVENT_GET_SAMPLING_STATE(), "AAA");
     TRACE_EVENT_SCOPED_SAMPLING_STATE("CCC", "name");
-    sampled->Wait();
+    TraceLog::GetInstance()->WaitSamplingEventForTesting();
     EXPECT_STREQ(TRACE_EVENT_GET_SAMPLING_STATE(), "CCC");
   }
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
   {
     EXPECT_STREQ(TRACE_EVENT_GET_SAMPLING_STATE(), "AAA");
     TRACE_EVENT_SET_SAMPLING_STATE("DDD", "name");
-    sampled->Wait();
+    TraceLog::GetInstance()->WaitSamplingEventForTesting();
     EXPECT_STREQ(TRACE_EVENT_GET_SAMPLING_STATE(), "DDD");
   }
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
   EXPECT_STREQ(TRACE_EVENT_GET_SAMPLING_STATE(), "DDD");
 
   EndTraceAndFlush();
@@ -1815,14 +1809,10 @@ TEST_F(TraceEventTestFixture, TraceContinuousSampling) {
       CategoryFilter("*"),
       TraceLog::Options(TraceLog::MONITOR_SAMPLING));
 
-  WaitableEvent* sampled = new WaitableEvent(false, false);
-  TraceLog::GetInstance()->InstallWaitableEventForSamplingTesting(
-      sampled);
-
   TRACE_EVENT_SET_SAMPLING_STATE_FOR_BUCKET(1, "category", "AAA");
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
   TRACE_EVENT_SET_SAMPLING_STATE_FOR_BUCKET(1, "category", "BBB");
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
 
   FlushMonitoring();
 
@@ -1831,12 +1821,12 @@ TEST_F(TraceEventTestFixture, TraceContinuousSampling) {
   EXPECT_TRUE(FindNamePhase("BBB", "P"));
 
   Clear();
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
 
   TRACE_EVENT_SET_SAMPLING_STATE_FOR_BUCKET(1, "category", "CCC");
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
   TRACE_EVENT_SET_SAMPLING_STATE_FOR_BUCKET(1, "category", "DDD");
-  sampled->Wait();
+  TraceLog::GetInstance()->WaitSamplingEventForTesting();
 
   FlushMonitoring();
 

tools/valgrind/tsan_v2/suppressions.txt

@@ -59,9 +59,6 @@ race:uprv_realloc_46
 # http://crbug.com/223955
 race:PassRefPtr
 
-# http://crbug.com/224617
-race:base::debug::TraceEventTestFixture_TraceSampling_Test::TestBody
-
 # http://crbug.com/239359
 race:media::TestInputCallback::OnData
 
@@ -135,8 +132,5 @@ race:PostponeInterruptsScope
 # http://crbug.com/296883
 race:net::URLFetcherCore::Stop
 
-# http://crbug.com/305459
-race:base::debug::TraceEventTestFixture_TraceContinuousSampling_Test::TestBody
-
 # http://crbug.com/310851
 race:net::ProxyResolverV8Tracing::Job::~Job