Bug 1258183 - TSan: data race toolkit/components/telemetry/Telemetry.cpp in CanRecordBase (part 2, derace). r=chutten.

--HG--
extra : rebase_source : 403c8ec419ee8ac2ece248a8395480dbd3018c74

ipc/chromium/src/base/histogram.cc

@@ -29,8 +29,6 @@ namespace base {
 typedef ::Lock Lock;
 typedef ::AutoLock AutoLock;
 
-using mozilla::OffTheBooksMutexAutoLock;
-
 // Static table of checksums for all possible 8 bit bytes.
 const uint32_t Histogram::kCrcTable[256] = {0x0, 0x77073096L, 0xee0e612cL,
 0x990951baL, 0x76dc419L, 0x706af48fL, 0xe963a535L, 0x9e6495a3L, 0xedb8832L,
@@ -177,24 +175,20 @@ void Histogram::WriteAscii(bool graph_it, const std::string& newline,
   SampleSet snapshot;
   SnapshotSample(&snapshot);
 
-  // For the rest of the routine, we hold |snapshot|'s lock so as to
+  Count sample_count = snapshot.TotalCount();
-  // be able to examine it atomically.
-  OffTheBooksMutexAutoLock locker(snapshot.mutex());
 
-  Count sample_count = snapshot.TotalCount(locker);
+  WriteAsciiHeader(snapshot, sample_count, output);
-
-  WriteAsciiHeader(snapshot, locker, sample_count, output);
   output->append(newline);
 
   // Prepare to normalize graphical rendering of bucket contents.
   double max_size = 0;
   if (graph_it)
-    max_size = GetPeakBucketSize(snapshot, locker);
+    max_size = GetPeakBucketSize(snapshot);
 
   // Calculate space needed to print bucket range numbers.  Leave room to print
   // nearly the largest bucket range without sliding over the histogram.
   size_t largest_non_empty_bucket = bucket_count() - 1;
-  while (0 == snapshot.counts(locker, largest_non_empty_bucket)) {
+  while (0 == snapshot.counts(largest_non_empty_bucket)) {
     if (0 == largest_non_empty_bucket)
       break;  // All buckets are empty.
     --largest_non_empty_bucket;
@@ -203,7 +197,7 @@ void Histogram::WriteAscii(bool graph_it, const std::string& newline,
   // Calculate largest print width needed for any of our bucket range displays.
   size_t print_width = 1;
   for (size_t i = 0; i < bucket_count(); ++i) {
-    if (snapshot.counts(locker, i)) {
+    if (snapshot.counts(i)) {
       size_t width = GetAsciiBucketRange(i).size() + 1;
       if (width > print_width)
         print_width = width;
@@ -214,7 +208,7 @@ void Histogram::WriteAscii(bool graph_it, const std::string& newline,
   int64_t past = 0;
   // Output the actual histogram graph.
   for (size_t i = 0; i < bucket_count(); ++i) {
-    Count current = snapshot.counts(locker, i);
+    Count current = snapshot.counts(i);
     if (!current && !PrintEmptyBucket(i))
       continue;
     remaining -= current;
@@ -223,8 +217,8 @@ void Histogram::WriteAscii(bool graph_it, const std::string& newline,
     for (size_t j = 0; range.size() + j < print_width + 1; ++j)
       output->push_back(' ');
     if (0 == current &&
-        i < bucket_count() - 1 && 0 == snapshot.counts(locker, i + 1)) {
+        i < bucket_count() - 1 && 0 == snapshot.counts(i + 1)) {
-      while (i < bucket_count() - 1 && 0 == snapshot.counts(locker, i + 1))
+      while (i < bucket_count() - 1 && 0 == snapshot.counts(i + 1))
         ++i;
       output->append("... ");
       output->append(newline);
@@ -244,14 +238,14 @@ void Histogram::WriteAscii(bool graph_it, const std::string& newline,
 // Methods for the validating a sample and a related histogram.
 //------------------------------------------------------------------------------
 
-Histogram::Inconsistencies Histogram::FindCorruption(
+Histogram::Inconsistencies
-    const SampleSet& snapshot,
+Histogram::FindCorruption(const SampleSet& snapshot) const
-    const OffTheBooksMutexAutoLock& snapshotLockEvidence) const {
+{
   int inconsistencies = NO_INCONSISTENCIES;
   Sample previous_range = -1;  // Bottom range is always 0.
   int64_t count = 0;
   for (size_t index = 0; index < bucket_count(); ++index) {
-    count += snapshot.counts(snapshotLockEvidence, index);
+    count += snapshot.counts(index);
     int new_range = ranges(index);
     if (previous_range >= new_range)
       inconsistencies |= BUCKET_ORDER_ERROR;
@@ -261,7 +255,7 @@ Histogram::Inconsistencies Histogram::FindCorruption(
   if (!HasValidRangeChecksum())
     inconsistencies |= RANGE_CHECKSUM_ERROR;
 
-  int64_t delta64 = snapshot.redundant_count(snapshotLockEvidence) - count;
+  int64_t delta64 = snapshot.redundant_count() - count;
   if (delta64 != 0) {
     int delta = static_cast<int>(delta64);
     if (delta != delta64)
@@ -302,7 +296,6 @@ size_t Histogram::bucket_count() const {
 }
 
 void Histogram::SnapshotSample(SampleSet* sample) const {
-  OffTheBooksMutexAutoLock locker(sample_.mutex());
   *sample = sample_;
 }
 
@@ -336,9 +329,9 @@ size_t Histogram::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)
   return n;
 }
 
-size_t Histogram::SampleSet::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)
+size_t
+Histogram::SampleSet::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)
 {
-  OffTheBooksMutexAutoLock locker(mutex_);
   // We're not allowed to do deep dives into STL data structures.  This
   // is as close as we can get to measuring this array.
   return aMallocSizeOf(&counts_[0]);
@@ -556,13 +549,11 @@ uint32_t Histogram::Crc32(uint32_t sum, Histogram::Sample range) {
 //------------------------------------------------------------------------------
 // Private methods
 
-double Histogram::GetPeakBucketSize(const SampleSet& snapshot,
+double Histogram::GetPeakBucketSize(const SampleSet& snapshot) const {
-                                    const OffTheBooksMutexAutoLock&
-                                          snapshotLockEvidence) const {
   double max = 0;
   for (size_t i = 0; i < bucket_count() ; ++i) {
     double current_size
-        = GetBucketSize(snapshot.counts(snapshotLockEvidence, i), i);
+        = GetBucketSize(snapshot.counts(i), i);
     if (current_size > max)
       max = current_size;
   }
@@ -570,15 +561,13 @@ double Histogram::GetPeakBucketSize(const SampleSet& snapshot,
 }
 
 void Histogram::WriteAsciiHeader(const SampleSet& snapshot,
-                                 const OffTheBooksMutexAutoLock&
-                                       snapshotLockEvidence,
                                  Count sample_count,
                                  std::string* output) const {
   StringAppendF(output,
                 "Histogram: %s recorded %d samples",
                 histogram_name().c_str(),
                 sample_count);
-  int64_t snapshot_sum = snapshot.sum(snapshotLockEvidence);
+  int64_t snapshot_sum = snapshot.sum();
   if (0 == sample_count) {
     DCHECK_EQ(snapshot_sum, 0);
   } else {
@@ -629,20 +618,17 @@ void Histogram::WriteAsciiBucketGraph(double current_size, double max_size,
 Histogram::SampleSet::SampleSet()
     : counts_(),
       sum_(0),
-      redundant_count_(0),
+      redundant_count_(0) {
-      mutex_("Histogram::SampleSet::SampleSet") {
 }
 
 Histogram::SampleSet::~SampleSet() {
 }
 
 void Histogram::SampleSet::Resize(const Histogram& histogram) {
-  OffTheBooksMutexAutoLock locker(mutex_);
   counts_.resize(histogram.bucket_count(), 0);
 }
 
-void Histogram::SampleSet::Accumulate(const OffTheBooksMutexAutoLock& ev,
+void Histogram::SampleSet::Accumulate(Sample value, Count count,
-                                      Sample value, Count count,
                                       size_t index) {
   DCHECK(count == 1 || count == -1);
   counts_[index] += count;
@@ -653,15 +639,7 @@ void Histogram::SampleSet::Accumulate(const OffTheBooksMutexAutoLock& ev,
   DCHECK_GE(redundant_count_, 0);
 }
 
-void Histogram::SampleSet::Accumulate(Sample value,
+Count Histogram::SampleSet::TotalCount() const {
-                                      Count count,
-                                      size_t index) {
-  OffTheBooksMutexAutoLock locker(mutex_);
-  Accumulate(locker, value, count, index);
-}
-
-Count Histogram::SampleSet::TotalCount(const OffTheBooksMutexAutoLock& ev)
-                                                                       const {
   Count total = 0;
   for (Counts::const_iterator it = counts_.begin();
        it != counts_.end();
@@ -672,7 +650,6 @@ Count Histogram::SampleSet::TotalCount(const OffTheBooksMutexAutoLock& ev)
 }
 
 void Histogram::SampleSet::Add(const SampleSet& other) {
-  OffTheBooksMutexAutoLock locker(mutex_);
   DCHECK_EQ(counts_.size(), other.counts_.size());
   sum_ += other.sum_;
   redundant_count_ += other.redundant_count_;
@@ -874,8 +851,7 @@ FlagHistogram::Accumulate(Sample value, Count count, size_t index)
 
 void
 FlagHistogram::AddSampleSet(const SampleSet& sample) {
-  OffTheBooksMutexAutoLock locker(sample.mutex());
+  DCHECK_EQ(bucket_count(), sample.size());
-  DCHECK_EQ(bucket_count(), sample.size(locker));
   // We can't be sure the SampleSet provided came from another FlagHistogram,
   // so we take the following steps:
   //  - If our flag has already been set do nothing.
@@ -889,12 +865,12 @@ FlagHistogram::AddSampleSet(const SampleSet& sample) {
     return;
   }
 
-  if (sample.sum(locker) != 1) {
+  if (sample.sum() != 1) {
     return;
   }
 
   size_t one_index = BucketIndex(1);
-  if (sample.counts(locker, one_index) == 1) {
+  if (sample.counts(one_index) == 1) {
     Accumulate(1, 1, one_index);
   }
 }
@@ -946,20 +922,18 @@ CountHistogram::Accumulate(Sample value, Count count, size_t index)
 
 void
 CountHistogram::AddSampleSet(const SampleSet& sample) {
-  OffTheBooksMutexAutoLock locker(sample.mutex());
+  DCHECK_EQ(bucket_count(), sample.size());
-  DCHECK_EQ(bucket_count(), sample.size(locker));
   // We can't be sure the SampleSet provided came from another CountHistogram,
   // so we at least check that the unused buckets are empty.
 
   const size_t indices[] = { BucketIndex(0), BucketIndex(1), BucketIndex(2) };
 
-  if (sample.counts(locker, indices[1]) != 0 ||
+  if (sample.counts(indices[1]) != 0 || sample.counts(indices[2]) != 0) {
-      sample.counts(locker, indices[2]) != 0) {
     return;
   }
 
-  if (sample.counts(locker, indices[0]) != 0) {
+  if (sample.counts(indices[0]) != 0) {
-    Accumulate(1, sample.counts(locker, indices[0]), indices[0]);
+    Accumulate(1, sample.counts(indices[0]), indices[0]);
   }
 }
 

ipc/chromium/src/base/histogram.h

@@ -45,7 +45,6 @@
 
 #include "mozilla/Atomics.h"
 #include "mozilla/MemoryReporting.h"
-#include "mozilla/Mutex.h"
 
 #include <map>
 #include <string>
@@ -56,9 +55,6 @@
 
 namespace base {
 
-using mozilla::OffTheBooksMutex;
-using mozilla::OffTheBooksMutexAutoLock;
-
 //------------------------------------------------------------------------------
 // Provide easy general purpose histogram in a macro, just like stats counters.
 // The first four macros use 50 buckets.
@@ -327,20 +323,8 @@ class Histogram {
     explicit SampleSet();
     ~SampleSet();
 
-    // This class contains a mozilla::OffTheBooksMutex, |mutex_|.
+    // None of the methods in this class are thread-safe.  Callers
-    // Most of the methods are thread-safe: they acquire and release
+    // must deal with locking themselves.
-    // the mutex themselves.  A few are not thread-safe, and require
-    // the caller to provide evidence that the object is locked, by
-    // supplying a const OffTheBooksMutexAutoLock& parameter.  The
-    // parameter is ignored but must be present.  |mutex_| must be an
-    // OffTheBooks variant because some of the containing SampleSet
-    // objects are leaked until shutdown, so a standard Mutex can't be
-    // used, since that does leak checking, and causes test failures.
-
-    //---------------- THREAD SAFE METHODS ----------------//
-    //
-    // The caller must not already hold |this.mutex_|, otherwise we
-    // will end up deadlocking.
 
     // Adjust size of counts_ for use with given histogram.
     void Resize(const Histogram& histogram);
@@ -353,38 +337,20 @@ class Histogram {
 
     size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf);
 
-    //---------------- THREAD UNSAFE METHODS ----------------//
+    Count counts(size_t i) const {
-    //
-    // The caller must hold |this.mutex_|, and must supply evidence by passing
-    // a const reference to the relevant OffTheBooksMutexAutoLock used.
-
-    Count counts(const OffTheBooksMutexAutoLock& ev, size_t i) const {
        return counts_[i];
     }
-    Count TotalCount(const OffTheBooksMutexAutoLock& ev) const;
+    Count TotalCount() const;
-    int64_t sum(const OffTheBooksMutexAutoLock& ev) const {
+    int64_t sum() const {
        return sum_;
     }
-    int64_t redundant_count(const OffTheBooksMutexAutoLock& ev) const {
+    int64_t redundant_count() const {
        return redundant_count_;
     }
-    size_t size(const OffTheBooksMutexAutoLock& ev) const {
+    size_t size() const {
        return counts_.size();
     }
 
-    // An assignment operator.  The presence of mozilla::OffTheBooksMutex
-    // in this class causes the default assignment operator to be deleted.
-    const SampleSet& operator=(const SampleSet& other) {
-       counts_          = other.counts_;
-       sum_             = other.sum_;
-       redundant_count_ = other.redundant_count_;
-       return *this;
-    }
-
-   private:
-    void Accumulate(const OffTheBooksMutexAutoLock& ev,
-                    Sample value, Count count, size_t index);
-
    protected:
     // Actual histogram data is stored in buckets, showing the count of values
     // that fit into each bucket.
@@ -402,13 +368,6 @@ class Histogram {
     // and also the snapshotting code may asynchronously get a mismatch (though
     // generally either race based mismatch cause is VERY rare).
     int64_t redundant_count_;
-
-   private:
-    // Protects all data fields.
-    mutable OffTheBooksMutex mutex_;
-
-   public:
-    OffTheBooksMutex& mutex() const { return mutex_; }
   };
 
   //----------------------------------------------------------------------------
@@ -466,9 +425,7 @@ class Histogram {
   // produce a false-alarm if a race occurred in the reading of the data during
   // a SnapShot process, but should otherwise be false at all times (unless we
   // have memory over-writes, or DRAM failures).
-  virtual Inconsistencies FindCorruption(const SampleSet& snapshot,
+  virtual Inconsistencies FindCorruption(const SampleSet& snapshot) const;
-                                         const OffTheBooksMutexAutoLock&
-                                               snapshotLockEvidence) const;
 
   //----------------------------------------------------------------------------
   // Accessors for factory constuction, serialization and testing.
@@ -483,7 +440,7 @@ class Histogram {
 
   // Do a safe atomic snapshot of sample data.  The caller is assumed to
   // have exclusive access to the destination, |*sample|, and no locking
-  // of it is done here.  This routine does lock the source sample though.
+  // of it is done here.
   virtual void SnapshotSample(SampleSet* sample) const;
 
   virtual bool HasConstructorArguments(Sample minimum, Sample maximum,
@@ -559,13 +516,10 @@ class Histogram {
   // Helpers for emitting Ascii graphic.  Each method appends data to output.
 
   // Find out how large the (graphically) the largest bucket will appear to be.
-  double GetPeakBucketSize(const SampleSet& snapshot,
+  double GetPeakBucketSize(const SampleSet& snapshot) const;
-                           const OffTheBooksMutexAutoLock&
-                                 snapshotLockEvidence) const;
 
   // Write a common header message describing this histogram.
   void WriteAsciiHeader(const SampleSet& snapshot,
-                        const OffTheBooksMutexAutoLock& snapshotLockEvidence,
                         Count sample_count, std::string* output) const;
 
   // Write information about previous, current, and next buckets.

toolkit/components/telemetry/Telemetry.cpp

@@ -2906,5 +2906,15 @@ SetProfileDir(nsIFile* aProfD)
   sTelemetryIOObserver->AddPath(profDirPath, NS_LITERAL_STRING("{profile}"));
 }
 
+void CreateStatisticsRecorder()
+{
+  TelemetryHistogram::CreateStatisticsRecorder();
+}
+
+void DestroyStatisticsRecorder()
+{
+  TelemetryHistogram::DestroyStatisticsRecorder();
+}
+
 } // namespace Telemetry
 } // namespace mozilla

toolkit/components/telemetry/Telemetry.h

@@ -37,6 +37,13 @@ enum TimerResolution {
   Microsecond
 };
 
+/**
+ * Create and destroy the underlying base::StatisticsRecorder singleton.
+ * Creation has to be done very early in the startup sequence.
+ */
+void CreateStatisticsRecorder();
+void DestroyStatisticsRecorder();
+
 /**
  * Initialize the Telemetry service on the main thread at startup.
  */

toolkit/components/telemetry/TelemetryHistogram.h

@@ -15,6 +15,9 @@
 
 namespace TelemetryHistogram {
 
+void CreateStatisticsRecorder();
+void DestroyStatisticsRecorder();
+
 void InitializeGlobalState(bool canRecordBase, bool canRecordExtended);
 void DeInitializeGlobalState();
 #ifdef DEBUG

toolkit/xre/nsAppRunner.cpp

@@ -94,8 +94,6 @@
 #include "mozilla/scache/StartupCache.h"
 #include "gfxPrefs.h"
 
-#include "base/histogram.h"
-
 #include "mozilla/unused.h"
 
 #ifdef XP_WIN
@@ -4586,12 +4584,9 @@ XRE_StopLateWriteChecks(void) {
 void
 XRE_CreateStatsObject()
 {
-  // A initializer to initialize histogram collection, a chromium
+  // Initialize global variables used by histogram collection
-  // thing used by Telemetry (and effectively a global; it's all static).
+  // machinery that is used by by Telemetry.  Note: is never de-initialised.
-  // Note: purposely leaked
+  Telemetry::CreateStatisticsRecorder();
-  base::StatisticsRecorder* statistics_recorder = new base::StatisticsRecorder();
-  MOZ_LSAN_INTENTIONALLY_LEAK_OBJECT(statistics_recorder);
-  Unused << statistics_recorder;
 }
 
 int

toolkit/xre/nsEmbedFunctions.cpp

@@ -76,7 +76,7 @@
 
 #include "GeckoProfiler.h"
 
- #include "base/histogram.h"
+#include "mozilla/Telemetry.h"
 
 #if defined(MOZ_SANDBOX) && defined(XP_WIN)
 #include "mozilla/sandboxTarget.h"
@@ -318,8 +318,7 @@ XRE_InitChildProcess(int aArgc,
 #endif
 
   // This is needed by Telemetry to initialize histogram collection.
-  UniquePtr<base::StatisticsRecorder> statisticsRecorder =
+  Telemetry::CreateStatisticsRecorder();
-    MakeUnique<base::StatisticsRecorder>();
 
 #if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_GONK)
   // On non-Fennec Gecko, the GMPLoader code resides in plugin-container,
@@ -668,7 +667,7 @@ XRE_InitChildProcess(int aArgc,
     }
   }
 
-  statisticsRecorder = nullptr;
+  Telemetry::DestroyStatisticsRecorder();
   profiler_shutdown();
   NS_LogTerm();
   return XRE_DeinitCommandLine();