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

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

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

3 files changed, 51 insertions, 8 deletions

diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index ea4f816..e85c217 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -154,10 +154,23 @@ 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;
+  bool bad_args = false;
+
   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
@@ -176,9 +189,9 @@ HistogramBase* Histogram::Factory::Build() {
     // 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;
@@ -223,8 +236,38 @@ 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
+
+  base::debug::Alias(&bad_args);  // Down here so var is always "used".
   return histogram;
 }
 
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/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");