Use lock-free lazy initialization for static histogram references

Make all histogram macros thread safe, and fast by again
using statics to achieve performance.

...at the cost of:
Leak all histograms to avoid races at shutdown.

Also included leak suppression for valgrind.

r=rtenneti
BUG=78207
Review URL: http://codereview.chromium.org/6780035

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

11 files changed, 83 insertions, 81 deletions

diff --git a/net/base/connection_type_histograms.cc b/net/base/connection_type_histograms.cc
index 06cdead..e64619c 100644
--- a/net/base/connection_type_histograms.cc
+++ b/net/base/connection_type_histograms.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -21,12 +21,8 @@ namespace net {
 // Each histogram has an unused bucket at the end to allow seamless future
 // expansion.
 void UpdateConnectionTypeHistograms(ConnectionType type) {
-  // TODO(wtc): Bug 74467 Move these stats up to a higher level, where the
-  // explicit static (shown below) and the implicit statics (inside the
-  // HISTOGRAM macros) will be thread safe.
-#if 0  // Don't do anything for now.
-
-  static bool had_connection_type[NUM_OF_CONNECTION_TYPES];
+  // TODO(wtc): Bug 74467 Move these stats up to a higher level.
+  static bool had_connection_type[NUM_OF_CONNECTION_TYPES];  // Default false.
 
   if (type >= 0 && type < NUM_OF_CONNECTION_TYPES) {
     if (!had_connection_type[type]) {
@@ -40,7 +36,6 @@ void UpdateConnectionTypeHistograms(ConnectionType type) {
   } else {
     NOTREACHED();  // Someone's logging an invalid type!
   }
-#endif  // 0
 }
 
 }  // namespace net
diff --git a/net/base/cookie_monster.h b/net/base/cookie_monster.h
index 8a0d591..92cc4f8 100644
--- a/net/base/cookie_monster.h
+++ b/net/base/cookie_monster.h
@@ -469,17 +469,17 @@ class CookieMonster : public CookieStore {
 
   // Histogram variables; see CookieMonster::InitializeHistograms() in
   // cookie_monster.cc for details.
-  scoped_refptr<base::Histogram> histogram_expiration_duration_minutes_;
-  scoped_refptr<base::Histogram> histogram_between_access_interval_minutes_;
-  scoped_refptr<base::Histogram> histogram_evicted_last_access_minutes_;
-  scoped_refptr<base::Histogram> histogram_count_;
-  scoped_refptr<base::Histogram> histogram_domain_count_;
-  scoped_refptr<base::Histogram> histogram_etldp1_count_;
-  scoped_refptr<base::Histogram> histogram_domain_per_etldp1_count_;
-  scoped_refptr<base::Histogram> histogram_number_duplicate_db_cookies_;
-  scoped_refptr<base::Histogram> histogram_cookie_deletion_cause_;
-  scoped_refptr<base::Histogram> histogram_time_get_;
-  scoped_refptr<base::Histogram> histogram_time_load_;
+  base::Histogram* histogram_expiration_duration_minutes_;
+  base::Histogram* histogram_between_access_interval_minutes_;
+  base::Histogram* histogram_evicted_last_access_minutes_;
+  base::Histogram* histogram_count_;
+  base::Histogram* histogram_domain_count_;
+  base::Histogram* histogram_etldp1_count_;
+  base::Histogram* histogram_domain_per_etldp1_count_;
+  base::Histogram* histogram_number_duplicate_db_cookies_;
+  base::Histogram* histogram_cookie_deletion_cause_;
+  base::Histogram* histogram_time_get_;
+  base::Histogram* histogram_time_load_;
 
   CookieMap cookies_;
 
diff --git a/net/base/cookie_monster_unittest.cc b/net/base/cookie_monster_unittest.cc
index a430833..abaf84c 100644
--- a/net/base/cookie_monster_unittest.cc
+++ b/net/base/cookie_monster_unittest.cc
@@ -2238,7 +2238,7 @@ TEST(CookieMonsterTest, HistogramCheck) {
   // Should match call in InitializeHistograms, but doesn't really matter
   // since the histogram should have been initialized by the CM construction
   // above.
-  scoped_refptr<base::Histogram> expired_histogram =
+  base::Histogram* expired_histogram =
       base::Histogram::FactoryGet(
           "Cookie.ExpirationDurationMinutes", 1, 10 * 365 * 24 * 60, 50,
           base::Histogram::kUmaTargetedHistogramFlag);
diff --git a/net/base/mime_sniffer.cc b/net/base/mime_sniffer.cc
index 25b94ec..b0b6d03 100644
--- a/net/base/mime_sniffer.cc
+++ b/net/base/mime_sniffer.cc
@@ -209,9 +209,9 @@ static const MagicNumber kSniffableTags[] = {
   MAGIC_HTML_TAG("p")  // Mozilla
 };
 
-static scoped_refptr<base::Histogram> UMASnifferHistogramGet(const char* name,
-                                                             int array_size) {
-  scoped_refptr<base::Histogram> counter =
+static base::Histogram* UMASnifferHistogramGet(const char* name,
+                                               int array_size) {
+  base::Histogram* counter =
       base::LinearHistogram::FactoryGet(name, 1, array_size - 1, array_size,
       base::Histogram::kUmaTargetedHistogramFlag);
   return counter;
@@ -308,13 +308,14 @@ static bool SniffForHTML(const char* content,
     if (!IsAsciiWhitespace(*pos))
       break;
   }
-  scoped_refptr<base::Histogram> counter =
-      UMASnifferHistogramGet("mime_sniffer.kSniffableTags2",
-                             arraysize(kSniffableTags));
+  static base::Histogram* counter(NULL);
+  if (!counter)
+    counter = UMASnifferHistogramGet("mime_sniffer.kSniffableTags2",
+                                     arraysize(kSniffableTags));
   // |pos| now points to first non-whitespace character (or at end).
   return CheckForMagicNumbers(pos, end - pos,
                               kSniffableTags, arraysize(kSniffableTags),
-                              counter.get(), result);
+                              counter, result);
 }
 
 // Returns true and sets result if the content matches any of kMagicNumbers.
@@ -326,12 +327,13 @@ static bool SniffForMagicNumbers(const char* content,
   *have_enough_content &= TruncateSize(kBytesRequiredForMagic, &size);
 
   // Check our big table of Magic Numbers
-  scoped_refptr<base::Histogram> counter =
-      UMASnifferHistogramGet("mime_sniffer.kMagicNumbers2",
-                             arraysize(kMagicNumbers));
+  static base::Histogram* counter(NULL);
+  if (!counter)
+    counter = UMASnifferHistogramGet("mime_sniffer.kMagicNumbers2",
+                                     arraysize(kMagicNumbers));
   return CheckForMagicNumbers(content, size,
                               kMagicNumbers, arraysize(kMagicNumbers),
-                              counter.get(), result);
+                              counter, result);
 }
 
 // Byte order marks
@@ -367,9 +369,10 @@ static bool SniffXML(const char* content,
   // We want to skip XML processing instructions (of the form "<?xml ...")
   // and stop at the first "plain" tag, then make a decision on the mime-type
   // based on the name (or possibly attributes) of that tag.
-  scoped_refptr<base::Histogram> counter =
-      UMASnifferHistogramGet("mime_sniffer.kMagicXML2",
-                             arraysize(kMagicXML));
+  static base::Histogram* counter(NULL);
+  if (!counter)
+    counter = UMASnifferHistogramGet("mime_sniffer.kMagicXML2",
+                                     arraysize(kMagicXML));
   const int kMaxTagIterations = 5;
   for (int i = 0; i < kMaxTagIterations && pos < end; ++i) {
     pos = reinterpret_cast<const char*>(memchr(pos, '<', end - pos));
@@ -389,7 +392,7 @@ static bool SniffXML(const char* content,
 
     if (CheckForMagicNumbers(pos, end - pos,
                              kMagicXML, arraysize(kMagicXML),
-                             counter.get(), result))
+                             counter, result))
       return true;
 
     // TODO(evanm): handle RSS 1.0, which is an RDF format and more difficult
@@ -451,13 +454,14 @@ static bool SniffBinary(const char* content,
   const bool is_truncated = TruncateSize(kMaxBytesToSniff, &size);
 
   // First, we look for a BOM.
-  scoped_refptr<base::Histogram> counter =
-      UMASnifferHistogramGet("mime_sniffer.kByteOrderMark2",
-                             arraysize(kByteOrderMark));
+  static base::Histogram* counter(NULL);
+  if (!counter)
+    counter = UMASnifferHistogramGet("mime_sniffer.kByteOrderMark2",
+                                     arraysize(kByteOrderMark));
   std::string unused;
   if (CheckForMagicNumbers(content, size,
                            kByteOrderMark, arraysize(kByteOrderMark),
-                           counter.get(), &unused)) {
+                           counter, &unused)) {
     // If there is BOM, we think the buffer is not binary.
     result->assign("text/plain");
     return false;
@@ -493,9 +497,10 @@ static bool IsUnknownMimeType(const std::string& mime_type) {
     // Firefox rejects a mime type if it is exactly */*
     "*/*",
   };
-  scoped_refptr<base::Histogram> counter =
-      UMASnifferHistogramGet("mime_sniffer.kUnknownMimeTypes2",
-                             arraysize(kUnknownMimeTypes) + 1);
+  static base::Histogram* counter(NULL);
+  if (!counter)
+    counter = UMASnifferHistogramGet("mime_sniffer.kUnknownMimeTypes2",
+                                     arraysize(kUnknownMimeTypes) + 1);
   for (size_t i = 0; i < arraysize(kUnknownMimeTypes); ++i) {
     if (mime_type == kUnknownMimeTypes[i]) {
       counter->Add(i);
@@ -519,8 +524,9 @@ static bool SniffCRX(const char* content,
                      const std::string& type_hint,
                      bool* have_enough_content,
                      std::string* result) {
-  scoped_refptr<base::Histogram> counter =
-      UMASnifferHistogramGet("mime_sniffer.kSniffCRX", 3);
+  static base::Histogram* counter(NULL);
+  if (!counter)
+    counter = UMASnifferHistogramGet("mime_sniffer.kSniffCRX", 3);
 
   // Technically, the crx magic number is just Cr24, but the bytes after that
   // are a version number which changes infrequently. Including it in the
@@ -557,8 +563,10 @@ static bool SniffCRX(const char* content,
 }
 
 bool ShouldSniffMimeType(const GURL& url, const std::string& mime_type) {
-  scoped_refptr<base::Histogram> should_sniff_counter =
-      UMASnifferHistogramGet("mime_sniffer.ShouldSniffMimeType2", 3);
+  static base::Histogram* should_sniff_counter(NULL);
+  if (!should_sniff_counter)
+    should_sniff_counter =
+        UMASnifferHistogramGet("mime_sniffer.ShouldSniffMimeType2", 3);
   // We are willing to sniff the mime type for HTTP, HTTPS, and FTP
   bool sniffable_scheme = url.is_empty() ||
                           url.SchemeIs("http") ||
@@ -582,9 +590,10 @@ bool ShouldSniffMimeType(const GURL& url, const std::string& mime_type) {
     "text/xml",
     "application/xml",
   };
-  scoped_refptr<base::Histogram> counter =
-      UMASnifferHistogramGet("mime_sniffer.kSniffableTypes2",
-                             arraysize(kSniffableTypes) + 1);
+  static base::Histogram* counter(NULL);
+  if (!counter)
+    counter = UMASnifferHistogramGet("mime_sniffer.kSniffableTypes2",
+                                     arraysize(kSniffableTypes) + 1);
   for (size_t i = 0; i < arraysize(kSniffableTypes); ++i) {
     if (mime_type == kSniffableTypes[i]) {
       counter->Add(i);
diff --git a/net/disk_cache/histogram_macros.h b/net/disk_cache/histogram_macros.h
index e238599..0dc12c5 100644
--- a/net/disk_cache/histogram_macros.h
+++ b/net/disk_cache/histogram_macros.h
@@ -17,10 +17,12 @@
 // These histograms follow the definition of UMA_HISTOGRAMN_XXX except that
 // whenever the name changes (the experiment group changes), the histrogram
 // object is re-created.
+// Note: These macros are only run on one thread, so the declarations of
+// |counter| was made static (i.e., there will be no race for reinitialization).
 
 #define CACHE_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \
     do { \
-      scoped_refptr<base::Histogram> counter; \
+      static base::Histogram* counter(NULL); \
       if (!counter || name != counter->histogram_name()) \
         counter = base::Histogram::FactoryGet( \
             name, min, max, bucket_count, \
@@ -39,7 +41,7 @@
 
 #define CACHE_HISTOGRAM_CUSTOM_TIMES(name, sample, min, max, bucket_count) \
     do { \
-      scoped_refptr<base::Histogram> counter; \
+      static base::Histogram* counter(NULL); \
       if (!counter || name != counter->histogram_name()) \
         counter = base::Histogram::FactoryTimeGet( \
             name, min, max, bucket_count, \
@@ -52,7 +54,7 @@
     base::TimeDelta::FromSeconds(10), 50)
 
 #define CACHE_HISTOGRAM_ENUMERATION(name, sample, boundary_value) do { \
-    scoped_refptr<base::Histogram> counter; \
+    static base::Histogram* counter(NULL); \
     if (!counter || name != counter->histogram_name()) \
       counter = base::LinearHistogram::FactoryGet( \
                     name, 1, boundary_value, boundary_value + 1, \
diff --git a/net/disk_cache/stats.cc b/net/disk_cache/stats.cc
index d9a9d12..23991c0 100644
--- a/net/disk_cache/stats.cc
+++ b/net/disk_cache/stats.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -116,7 +116,7 @@ bool CreateStats(BackendImpl* backend, Addr* address, OnDiskStats* stats) {
   return StoreStats(backend, *address, stats);
 }
 
-Stats::Stats() : backend_(NULL) {
+Stats::Stats() : backend_(NULL), size_histogram_(NULL) {
 }
 
 Stats::~Stats() {
@@ -146,7 +146,7 @@ bool Stats::Init(BackendImpl* backend, uint32* storage_addr) {
   if (first_time) {
     first_time = false;
     // ShouldReportAgain() will re-enter this object.
-    if (!size_histogram_.get() && backend->cache_type() == net::DISK_CACHE &&
+    if (!size_histogram_ && backend->cache_type() == net::DISK_CACHE &&
         backend->ShouldReportAgain()) {
       // Stats may be reused when the cache is re-created, but we want only one
       // histogram at any given time.
diff --git a/net/disk_cache/stats.h b/net/disk_cache/stats.h
index ba0d7dd..1f5aa59 100644
--- a/net/disk_cache/stats.h
+++ b/net/disk_cache/stats.h
@@ -86,7 +86,7 @@ class Stats {
   uint32 storage_addr_;
   int data_sizes_[kDataSizesLength];
   int64 counters_[MAX_COUNTER];
-  scoped_refptr<StatsHistogram> size_histogram_;
+  StatsHistogram* size_histogram_;
 
   DISALLOW_COPY_AND_ASSIGN(Stats);
 };
diff --git a/net/disk_cache/stats_histogram.cc b/net/disk_cache/stats_histogram.cc
index e9d7696..6d3097a 100644
--- a/net/disk_cache/stats_histogram.cc
+++ b/net/disk_cache/stats_histogram.cc
@@ -21,23 +21,23 @@ StatsHistogram::~StatsHistogram() {
     stats_ = NULL;
 }
 
-scoped_refptr<StatsHistogram> StatsHistogram::StatsHistogramFactoryGet(
+StatsHistogram* StatsHistogram::StatsHistogramFactoryGet(
     const std::string& name) {
-  scoped_refptr<Histogram> histogram(NULL);
+  Histogram* histogram(NULL);
 
   Sample minimum = 1;
   Sample maximum = disk_cache::Stats::kDataSizesLength - 1;
   size_t bucket_count = disk_cache::Stats::kDataSizesLength;
 
   if (StatisticsRecorder::FindHistogram(name, &histogram)) {
-    DCHECK(histogram.get() != NULL);
+    DCHECK(histogram != NULL);
   } else {
-    StatsHistogram* stats_histogram = new StatsHistogram(name, minimum, maximum,
-                                                         bucket_count);
+    // To avoid racy destruction at shutdown, the following will be leaked.
+    StatsHistogram* stats_histogram =
+        new StatsHistogram(name, minimum, maximum, bucket_count);
     stats_histogram->InitializeBucketRange();
-    histogram = stats_histogram;
-    histogram->SetFlags(kUmaTargetedHistogramFlag);
-    StatisticsRecorder::RegisterOrDiscardDuplicate(&histogram);
+    stats_histogram->SetFlags(kUmaTargetedHistogramFlag);
+    histogram = StatisticsRecorder::RegisterOrDeleteDuplicate(stats_histogram);
   }
 
   DCHECK(HISTOGRAM == histogram->histogram_type());
@@ -45,13 +45,10 @@ scoped_refptr<StatsHistogram> StatsHistogram::StatsHistogramFactoryGet(
 
   // We're preparing for an otherwise unsafe upcast by ensuring we have the
   // proper class type.
-  Histogram* temp_histogram = histogram.get();
-  StatsHistogram* temp_stats_histogram =
-      static_cast<StatsHistogram*>(temp_histogram);
+  StatsHistogram* return_histogram = static_cast<StatsHistogram*>(histogram);
   // Validate upcast by seeing that we're probably providing the checksum.
-  CHECK_EQ(temp_stats_histogram->StatsHistogram::CalculateRangeChecksum(),
-           temp_stats_histogram->CalculateRangeChecksum());
-  scoped_refptr<StatsHistogram> return_histogram(temp_stats_histogram);
+  CHECK_EQ(return_histogram->StatsHistogram::CalculateRangeChecksum(),
+           return_histogram->CalculateRangeChecksum());
   return return_histogram;
 }
 
diff --git a/net/disk_cache/stats_histogram.h b/net/disk_cache/stats_histogram.h
index 249327c..83d359c 100644
--- a/net/disk_cache/stats_histogram.h
+++ b/net/disk_cache/stats_histogram.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -35,8 +35,7 @@ class StatsHistogram : public base::Histogram {
       : Histogram(name, minimum, maximum, bucket_count), init_(false) {}
   ~StatsHistogram();
 
-  static scoped_refptr<StatsHistogram>
-      StatsHistogramFactoryGet(const std::string& name);
+  static StatsHistogram* StatsHistogramFactoryGet(const std::string& name);
 
   // We'll be reporting data from the given set of cache stats.
   bool Init(const Stats* stats);
diff --git a/net/socket/client_socket_pool_histograms.h b/net/socket/client_socket_pool_histograms.h
index 493d34a..0e5afff 100644
--- a/net/socket/client_socket_pool_histograms.h
+++ b/net/socket/client_socket_pool_histograms.h
@@ -28,10 +28,10 @@ class ClientSocketPoolHistograms {
   void AddReusedIdleTime(base::TimeDelta time) const;
 
  private:
-  scoped_refptr<base::Histogram> socket_type_;
-  scoped_refptr<base::Histogram> request_time_;
-  scoped_refptr<base::Histogram> unused_idle_time_;
-  scoped_refptr<base::Histogram> reused_idle_time_;
+  base::Histogram* socket_type_;
+  base::Histogram* request_time_;
+  base::Histogram* unused_idle_time_;
+  base::Histogram* reused_idle_time_;
 
   bool is_http_proxy_connection_;
   bool is_socks_connection_;
diff --git a/net/socket_stream/socket_stream_metrics_unittest.cc b/net/socket_stream/socket_stream_metrics_unittest.cc
index 72ae142..23f227d 100644
--- a/net/socket_stream/socket_stream_metrics_unittest.cc
+++ b/net/socket_stream/socket_stream_metrics_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -25,7 +25,7 @@ TEST(SocketStreamMetricsTest, Initialize) {
 }
 
 TEST(SocketStreamMetricsTest, ProtocolType) {
-  scoped_refptr<Histogram> histogram;
+  Histogram* histogram;
 
   // First we'll preserve the original values. We need to do this
   // as histograms can get affected by other tests. In particular,
@@ -57,7 +57,7 @@ TEST(SocketStreamMetricsTest, ProtocolType) {
 }
 
 TEST(SocketStreamMetricsTest, ConnectionType) {
-  scoped_refptr<Histogram> histogram;
+  Histogram* histogram;
 
   // First we'll preserve the original values.
   Histogram::SampleSet original;
@@ -91,7 +91,7 @@ TEST(SocketStreamMetricsTest, ConnectionType) {
 }
 
 TEST(SocketStreamMetricsTest, OtherNumbers) {
-  scoped_refptr<Histogram> histogram;
+  Histogram* histogram;
 
   // First we'll preserve the original values.
   int64 original_received_bytes = 0;