Collect information about failing histogram factory calls.

Some tests need to be disabled because they create the same histogram with different parameters, a condition which does not always crash in the calling code but does with this CL.

This CL will be reverted once the necessary information has been collected, including re-enabling of any tests.

SHERIFFS: If a test breaks in the waterfall, you can either revert this CL or disable the test.  I'll re-enable it when reverting this after the necessary data has been collected.

BUG=588946

Committed: https://crrev.com/ce1315b94863f40d6323fa488490ed866aae4ed4
Cr-Commit-Position: refs/heads/master@{#378957}

Committed: https://crrev.com/63f1c5d846f6dac2cf148b81961c38b88ff1bd86
Cr-Commit-Position: refs/heads/master@{#379323}

Committed: https://crrev.com/89dca78efa00acb8fc8749adcc1a08a24eb105d5
Cr-Commit-Position: refs/heads/master@{#379489}

Committed: https://crrev.com/1375777f66fcf8edd3f393dadd9d9de6a7409030
Cr-Commit-Position: refs/heads/master@{#379690}

Committed: https://crrev.com/0abc5c2d2cb7ac70e26eab73d7e3f61f0472461e
Cr-Commit-Position: refs/heads/master@{#380070}

Committed: https://crrev.com/1d50ab3eec1dab7d3a6c70ec85b8937d5073b66d
Cr-Commit-Position: refs/heads/master@{#380474}

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

Cr-Commit-Position: refs/heads/master@{#380889}

6 files changed, 147 insertions, 15 deletions

diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index ea4f816..7feacdb 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -154,10 +154,22 @@ HistogramBase* Histogram::Factory::Build() {
   ImportPersistentHistograms();
 
   HistogramBase* histogram = StatisticsRecorder::FindHistogram(name_);
+
+  // crbug.com/588946 debugging. See comment at end of function.
+  const BucketRanges* created_ranges =
+      reinterpret_cast<const BucketRanges*>(0xDEADBEEF);
+  const BucketRanges* registered_ranges =
+      reinterpret_cast<const BucketRanges*>(0xDEADBEEF);
+  HistogramBase* tentative_histogram =
+      reinterpret_cast<HistogramBase*>(0xDEADBEEF);
+  PersistentMemoryAllocator* allocator =
+      reinterpret_cast<PersistentMemoryAllocator*>(0xDEADBEEF);
+  PersistentMemoryAllocator::Reference histogram_ref = 0xDEADBEEF;
+
   if (!histogram) {
     // To avoid racy destruction at shutdown, the following will be leaked.
-    const BucketRanges* created_ranges = CreateRanges();
-    const BucketRanges* registered_ranges =
+    created_ranges = CreateRanges();
+    registered_ranges =
         StatisticsRecorder::RegisterOrDeleteDuplicateRanges(created_ranges);
 
     // In most cases, the bucket-count, minimum, and maximum values are known
@@ -170,15 +182,17 @@ HistogramBase* Histogram::Factory::Build() {
       minimum_ = registered_ranges->range(1);
       maximum_ = registered_ranges->range(bucket_count_ - 1);
     }
+    CHECK_LT(0, minimum_);
+    CHECK_LT(0, maximum_);
 
     // Try to create the histogram using a "persistent" allocator. As of
     // 2015-01-14, the availability of such is controlled by a base::Feature
     // that is off by default. If the allocator doesn't exist or if
     // allocating from it fails, code below will allocate the histogram from
     // the process heap.
-    PersistentMemoryAllocator::Reference histogram_ref = 0;
-    HistogramBase* tentative_histogram = nullptr;
-    PersistentMemoryAllocator* allocator =
+    histogram_ref = 0;
+    tentative_histogram = nullptr;
+    allocator =
         GetPersistentHistogramMemoryAllocator();
     if (allocator) {
       flags_ |= HistogramBase::kIsPersistent;
@@ -191,6 +205,12 @@ HistogramBase* Histogram::Factory::Build() {
           registered_ranges,
           flags_,
           &histogram_ref);
+      CHECK_LT(0, minimum_);
+      CHECK_LT(0, maximum_);
+      CHECK_EQ(minimum_,
+               static_cast<Histogram*>(tentative_histogram)->declared_min_);
+      CHECK_EQ(maximum_,
+               static_cast<Histogram*>(tentative_histogram)->declared_max_);
     }
 
     // Handle the case where no persistent allocator is present or the
@@ -200,6 +220,12 @@ HistogramBase* Histogram::Factory::Build() {
       DCHECK(!allocator);  // Shouldn't have failed.
       flags_ &= ~HistogramBase::kIsPersistent;
       tentative_histogram = HeapAlloc(registered_ranges);
+      CHECK_LT(0, minimum_);
+      CHECK_LT(0, maximum_);
+      CHECK_EQ(minimum_,
+               static_cast<Histogram*>(tentative_histogram)->declared_min_);
+      CHECK_EQ(maximum_,
+               static_cast<Histogram*>(tentative_histogram)->declared_max_);
     }
 
     FillHistogram(tentative_histogram);
@@ -214,6 +240,15 @@ HistogramBase* Histogram::Factory::Build() {
   }
 
   DCHECK_EQ(histogram_type_, histogram->GetHistogramType());
+  bool bad_args = false;
+  HistogramBase* existing_histogram = histogram;
+  HistogramType existing_type = histogram->GetHistogramType();
+  const char* existing_name = histogram->histogram_name().c_str();
+  Sample existing_minimum = static_cast<Histogram*>(histogram)->declared_min_;
+  Sample existing_maximum = static_cast<Histogram*>(histogram)->declared_max_;
+  uint32_t existing_bucket_count =
+    static_cast<Histogram*>(histogram)->bucket_count();
+
   if (bucket_count_ != 0 &&
       !histogram->HasConstructionArguments(minimum_, maximum_, bucket_count_)) {
     // The construction arguments do not match the existing histogram.  This can
@@ -223,8 +258,45 @@ HistogramBase* Histogram::Factory::Build() {
     // on dereference, but extension/Pepper APIs will guard against NULL and not
     // crash.
     DLOG(ERROR) << "Histogram " << name_ << " has bad construction arguments";
-    return nullptr;
+    bad_args = true;
+    histogram = nullptr;
+  }
+
+#if !DCHECK_IS_ON()  // Don't affect tests, only release builds.
+  // For the moment, crash here so that collected crash reports have access
+  // to the construction values in order to figure out why this is failing.
+  // TODO(bcwhite): Remove this once crbug.com/588946 is resolved. Also remove
+  // from beta-branch because we don't want crashes due to misbehaving
+  // extensions (see comment above).
+  if (!histogram) {
+    HistogramType histogram_type = histogram_type_;
+    HistogramBase::Sample minimum = minimum_;
+    HistogramBase::Sample maximum = maximum_;
+    uint32_t bucket_count = bucket_count_;
+    int32_t flags = flags_;
+    CHECK(histogram) << name_ << ": bad-args=" << bad_args;
+    base::debug::Alias(&histogram_type);
+    base::debug::Alias(&minimum);
+    base::debug::Alias(&maximum);
+    base::debug::Alias(&bucket_count);
+    base::debug::Alias(&flags);
+    base::debug::Alias(&created_ranges);
+    base::debug::Alias(&registered_ranges);
+    base::debug::Alias(&histogram_ref);
+    base::debug::Alias(&tentative_histogram);
+    base::debug::Alias(&allocator);
+    base::debug::Alias(&tentative_histogram);
   }
+#endif
+
+  // Down here so vars are always "used".
+  base::debug::Alias(&bad_args);
+  base::debug::Alias(&existing_histogram);
+  base::debug::Alias(&existing_type);
+  base::debug::Alias(&existing_name);
+  base::debug::Alias(&existing_minimum);
+  base::debug::Alias(&existing_maximum);
+  base::debug::Alias(&existing_bucket_count);
   return histogram;
 }
 
@@ -486,8 +558,12 @@ Histogram::Histogram(const std::string& name,
     bucket_ranges_(ranges),
     declared_min_(minimum),
     declared_max_(maximum) {
+  CHECK_LT(0, minimum);
+  CHECK_LT(0, maximum);
   if (ranges)
     samples_.reset(new SampleVector(HashMetricName(name), ranges));
+  CHECK_EQ(minimum, declared_min_);
+  CHECK_EQ(maximum, declared_max_);
 }
 
 Histogram::Histogram(const std::string& name,
@@ -503,12 +579,16 @@ Histogram::Histogram(const std::string& name,
     bucket_ranges_(ranges),
     declared_min_(minimum),
     declared_max_(maximum) {
+  CHECK_LT(0, minimum);
+  CHECK_LT(0, maximum);
   if (ranges) {
     samples_.reset(new SampleVector(HashMetricName(name),
                                     counts, counts_size, meta, ranges));
     logged_samples_.reset(new SampleVector(samples_->id(), logged_counts,
                                            counts_size, logged_meta, ranges));
   }
+  CHECK_EQ(minimum, declared_min_);
+  CHECK_EQ(maximum, declared_max_);
 }
 
 Histogram::~Histogram() {
diff --git a/base/metrics/histogram_persistence.cc b/base/metrics/histogram_persistence.cc
index f18d175..34b3100 100644
--- a/base/metrics/histogram_persistence.cc
+++ b/base/metrics/histogram_persistence.cc
@@ -244,6 +244,12 @@ HistogramBase* CreatePersistentHistogram(
   // validated below; the local copy is to ensure that the contents cannot
   // be externally changed between validation and use.
   PersistentHistogramData histogram_data = *histogram_data_ptr;
+  CHECK_EQ(histogram_data.histogram_type, histogram_data_ptr->histogram_type);
+  CHECK_EQ(histogram_data.flags, histogram_data_ptr->flags);
+  CHECK_EQ(histogram_data.minimum, histogram_data_ptr->minimum);
+  CHECK_EQ(histogram_data.maximum, histogram_data_ptr->maximum);
+  CHECK_EQ(histogram_data.bucket_count, histogram_data_ptr->bucket_count);
+  CHECK_EQ(histogram_data.ranges_checksum, histogram_data_ptr->ranges_checksum);
 
   HistogramBase::Sample* ranges_data =
       allocator->GetAsObject<HistogramBase::Sample>(histogram_data.ranges_ref,
@@ -288,6 +294,8 @@ HistogramBase* CreatePersistentHistogram(
   HistogramBase::AtomicCount* logged_data =
       counts_data + histogram_data.bucket_count;
 
+  CHECK_LT(0, histogram_data.minimum);
+  CHECK_LT(0, histogram_data.maximum);
   std::string name(histogram_data_ptr->name);
   HistogramBase* histogram = nullptr;
   switch (histogram_data.histogram_type) {
diff --git a/base/metrics/histogram_unittest.cc b/base/metrics/histogram_unittest.cc
index beb6a71..e6aefd3 100644
--- a/base/metrics/histogram_unittest.cc
+++ b/base/metrics/histogram_unittest.cc
@@ -668,7 +668,7 @@ TEST_P(HistogramTest, CustomHistogramSerializeInfo) {
   EXPECT_FALSE(iter.SkipBytes(1));
 }
 
-TEST_P(HistogramTest, BadConstruction) {
+TEST_P(HistogramTest, DISABLED_BadConstruction) {
   HistogramBase* histogram = Histogram::FactoryGet(
       "BadConstruction", 0, 100, 8, HistogramBase::kNoFlags);
   EXPECT_TRUE(histogram->HasConstructionArguments(1, 100, 8));
diff --git a/base/metrics/metrics_hashes.cc b/base/metrics/metrics_hashes.cc
index 5672b06..9b60401 100644
--- a/base/metrics/metrics_hashes.cc
+++ b/base/metrics/metrics_hashes.cc
@@ -4,6 +4,7 @@
 
 #include "base/metrics/metrics_hashes.h"
 
+#include "base/debug/alias.h"
 #include "base/logging.h"
 #include "base/md5.h"
 #include "base/sys_byteorder.h"
@@ -15,16 +16,42 @@ namespace {
 // Converts the 8-byte prefix of an MD5 hash into a uint64_t value.
 inline uint64_t DigestToUInt64(const base::MD5Digest& digest) {
   uint64_t value;
-  DCHECK_GE(sizeof(digest.a), sizeof(value));
+  CHECK_GE(sizeof(digest.a), sizeof(value));
   memcpy(&value, digest.a, sizeof(value));
-  return base::NetToHost64(value);
+  uint64_t hash = base::NetToHost64(value);
+  CHECK_NE(0U, hash);
+  base::debug::Alias(&hash);
+  base::debug::Alias(&value);
+  base::debug::Alias(&digest);
+  return hash;
 }
 
 }  // namespace
 
 uint64_t HashMetricName(base::StringPiece name) {
-  base::MD5Digest digest;
+  base::MD5Digest digest = {{
+    0x0F, 0x0E, 0x0D, 0x0C, 0x00, 0x0D, 0x0E, 0x02,
+    0x0D, 0x0E, 0x0A, 0x0D, 0x0B, 0x0E, 0x0E, 0x0F
+  }};
   base::MD5Sum(name.data(), name.size(), &digest);
+  CHECK(
+      digest.a[0] != 0x0F ||
+      digest.a[1] != 0x0E ||
+      digest.a[2] != 0x0D ||
+      digest.a[3] != 0x0C ||
+      digest.a[4] != 0x00 ||
+      digest.a[5] != 0x0D ||
+      digest.a[6] != 0x0E ||
+      digest.a[7] != 0x02 ||
+      digest.a[8] != 0x0D ||
+      digest.a[9] != 0x0E ||
+      digest.a[10] != 0x0A ||
+      digest.a[11] != 0x0D ||
+      digest.a[12] != 0x0B ||
+      digest.a[13] != 0x0E ||
+      digest.a[14] != 0x0E ||
+      digest.a[15] != 0x0F);
+  base::debug::Alias(&name);
   return DigestToUInt64(digest);
 }
 
diff --git a/base/metrics/statistics_recorder.cc b/base/metrics/statistics_recorder.cc
index d0fa2ad..90d64bb 100644
--- a/base/metrics/statistics_recorder.cc
+++ b/base/metrics/statistics_recorder.cc
@@ -5,6 +5,7 @@
 #include "base/metrics/statistics_recorder.h"
 
 #include "base/at_exit.h"
+#include "base/debug/alias.h"
 #include "base/debug/leak_annotations.h"
 #include "base/json/string_escape.h"
 #include "base/logging.h"
@@ -123,13 +124,17 @@ HistogramBase* StatisticsRecorder::RegisterOrDeleteDuplicate(
         histogram_to_return = histogram;
       } else {
         // We already have one histogram with this name.
-        DCHECK_EQ(histogram->histogram_name(),
-                  it->second->histogram_name()) << "hash collision";
+        CHECK_EQ(histogram->histogram_name(),
+                 it->second->histogram_name()) << "hash collision";
         histogram_to_return = it->second;
         histogram_to_delete = histogram;
       }
+      base::debug::Alias(&it);
+      base::debug::Alias(&name);
+      base::debug::Alias(&name_hash);
     }
   }
+  base::debug::Alias(&histogram);
   delete histogram_to_delete;
   return histogram_to_return;
 }
@@ -279,10 +284,22 @@ HistogramBase* StatisticsRecorder::FindHistogram(base::StringPiece name) {
   if (histograms_ == NULL)
     return NULL;
 
-  HistogramMap::iterator it = histograms_->find(HashMetricName(name));
+  const uint64_t lookup_hash = HashMetricName(name);
+  HistogramMap::iterator it = histograms_->find(lookup_hash);
   if (histograms_->end() == it)
     return NULL;
-  DCHECK_EQ(name, it->second->histogram_name()) << "hash collision";
+
+  const uint64_t existing_hash = it->first;
+  const char* existing_name = it->second->histogram_name().c_str();
+  HistogramMap* histograms = histograms_;
+  CHECK_EQ(name, it->second->histogram_name()) << "hash collision";
+  base::debug::Alias(&lookup_hash);
+  base::debug::Alias(&existing_hash);
+  base::debug::Alias(&name);
+  base::debug::Alias(&existing_name);
+  base::debug::Alias(&it);
+  base::debug::Alias(&histograms);
+
   return it->second;
 }
 
diff --git a/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc b/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
index 3160400..d4c5e3c 100644
--- a/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
+++ b/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
@@ -128,7 +128,7 @@ void ValidateHistograms(const RecordedHistogram* recorded,
 
 }  // anonymous namespace
 
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Metrics) {
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_Metrics) {
   base::UserActionTester user_action_tester;
 
   base::FieldTrialList::CreateFieldTrial("apitestfieldtrial2", "group1");