Avoid threading races on TraceSamplingThread's members.

base/debug/trace_event_impl.cc

Index: base/debug/trace_event_impl.cc
diff --git a/base/debug/trace_event_impl.cc b/base/debug/trace_event_impl.cc
index 4a365a773dc37a6d776eff5785ed1eb8d1ad989e..d96602502a9d7b85a8b60d44092258ff28661646 100644
--- a/base/debug/trace_event_impl.cc
+++ b/base/debug/trace_event_impl.cc
@@ -713,6 +713,7 @@ class TraceSamplingThread : public PlatformThread::Delegate {
   static void ExtractCategoryAndName(const char* combined,
                                      const char** category,
                                      const char** name);
+  Lock lock_;
   std::vector<TraceBucketData> sample_buckets_;
   bool thread_running_;
   scoped_ptr<CancellationFlag> cancellation_flag_;
@@ -722,6 +723,7 @@ class TraceSamplingThread : public PlatformThread::Delegate {
 
 TraceSamplingThread::TraceSamplingThread()
     : thread_running_(false) {
+  AutoLock lock(lock_);
   cancellation_flag_.reset(new CancellationFlag);

[Xianzhu, 2013/10/17 16:26:12]

It looks weird that CancellationFlag and WaitableEvent are used along with a
lock. Both of the are thread-safe. I think you can avoid the lock by using
CancellationFlag directly not through a scoped_ptr.

I think WaitableEvent is the same if the tests allocate it on stack and this
class just holds a non-owning pointer:

 }
 
@@ -732,10 +734,14 @@ void TraceSamplingThread::ThreadMain() {
   PlatformThread::SetName("Sampling Thread");
   thread_running_ = true;
   const int kSamplingFrequencyMicroseconds = 1000;
-  while (!cancellation_flag_->IsSet()) {
+  while (true) {
     PlatformThread::Sleep(
         TimeDelta::FromMicroseconds(kSamplingFrequencyMicroseconds));
     GetSamples();
+
+    AutoLock lock(lock_);
+    if (cancellation_flag_->IsSet())
+      break;
     if (waitable_event_for_testing_.get())
       waitable_event_for_testing_->Signal();
   }
@@ -759,6 +765,7 @@ void TraceSamplingThread::DefaultSamplingCallback(
 }
 
 void TraceSamplingThread::GetSamples() {
+  AutoLock lock(lock_);
   for (size_t i = 0; i < sample_buckets_.size(); ++i) {
     TraceBucketData* bucket_data = &sample_buckets_[i];
     bucket_data->callback.Run(bucket_data);

[haraken, 2013/10/17 01:15:51]

The only suspicious place is this one. We're calling a callback while holding a
lock. However, this wouldn't be a problem because the callback
(DefautSamplingCallback) doesn't acquire the lock...

[Xianzhu, 2013/10/17 16:26:12]

Please add comments somewhere to state the above restriction.

@@ -769,6 +776,7 @@ void TraceSamplingThread::RegisterSampleBucket(
     TRACE_EVENT_API_ATOMIC_WORD* bucket,
     const char* const name,
     TraceSampleCallback callback) {
+  AutoLock lock(lock_);
   DCHECK(!thread_running_);
   sample_buckets_.push_back(TraceBucketData(bucket, name, callback));
 }
@@ -782,11 +790,13 @@ void TraceSamplingThread::ExtractCategoryAndName(const char* combined,
 }
 
 void TraceSamplingThread::Stop() {
+  AutoLock lock(lock_);
   cancellation_flag_->Set();
 }
 
 void TraceSamplingThread::InstallWaitableEventForSamplingTesting(
     WaitableEvent* waitable_event) {
+  AutoLock lock(lock_);
   waitable_event_for_testing_.reset(waitable_event);
 }