Avoid threading races on TraceSamplingThread's members.

base/debug/trace_event_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/debug/trace_event_impl.h"
 
 #include <algorithm>
 
 #include "base/base_switches.h"
 #include "base/bind.h"
@@ skipping 695 matching lines @@
   void GetSamples();
   // Not thread-safe. Once the ThreadMain has been called, this can no longer
   // be called.
   void RegisterSampleBucket(TRACE_EVENT_API_ATOMIC_WORD* bucket,
                             const char* const name,
                             TraceSampleCallback callback);
   // Splits a combined "category\0name" into the two component parts.
   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_;
   scoped_ptr<WaitableEvent> waitable_event_for_testing_;
 };
 
 
 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:

 }
 
 TraceSamplingThread::~TraceSamplingThread() {
 }
 
 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();
   }
 }
 
 // static
 void TraceSamplingThread::DefaultSamplingCallback(
     TraceBucketData* bucket_data) {
   TRACE_EVENT_API_ATOMIC_WORD category_and_name =
       TRACE_EVENT_API_ATOMIC_LOAD(*bucket_data->bucket);
   if (!category_and_name)
     return;
   const char* const combined =
       reinterpret_cast<const char* const>(category_and_name);
   const char* category_group;
   const char* name;
   ExtractCategoryAndName(combined, &category_group, &name);
   TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_SAMPLE,
       TraceLog::GetCategoryGroupEnabled(category_group),
       name, 0, 0, NULL, NULL, NULL, NULL, 0);
 }
 
 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.

   }
 }
 
 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));
 }
 
 // static
 void TraceSamplingThread::ExtractCategoryAndName(const char* combined,
                                                  const char** category,
                                                  const char** name) {
   *category = combined;
   *name = &combined[strlen(combined) + 1];
 }
 
 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);
 }
 
 TraceBucketData::TraceBucketData(base::subtle::AtomicWord* bucket,
                                  const char* name,
                                  TraceSampleCallback callback)
     : bucket(bucket),
       bucket_name(name),
       callback(callback) {
 }
@@ skipping 1268 matching lines @@
         0,                       // num_args
         NULL,                    // arg_names
         NULL,                    // arg_types
         NULL,                    // arg_values
         NULL,                    // convertable values
         TRACE_EVENT_FLAG_NONE);  // flags
   }
 }
 
 }  // namespace trace_event_internal