Don't crash for mismatched histogram parms.

These mismatches shouldn't happen but crashing is a fairly severe
response for the released products.  Just report the error.

Bug: 756948
Change-Id: Ia8f249afafec1e8ddf054a2a818b78b2ffd98132
Reviewed-on: https://chromium-review.googlesource.com/1012469Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552089}

base/metrics/histogram.cc

@@ -228,17 +228,19 @@ HistogramBase* Histogram::Factory::Build() {
     }
   }
 
-  CHECK_EQ(histogram_type_, histogram->GetHistogramType()) << name_;
-  if (bucket_count_ != 0 &&
-      !histogram->HasConstructionArguments(minimum_, maximum_, bucket_count_)) {
+  if (histogram_type_ != histogram->GetHistogramType() ||
+      (bucket_count_ != 0 && !histogram->HasConstructionArguments(
+                                 minimum_, maximum_, bucket_count_))) {
     // The construction arguments do not match the existing histogram.  This can
     // come about if an extension updates in the middle of a chrome run and has
-    // changed one of them, or simply by bad code within Chrome itself.  We
-    // return NULL here with the expectation that bad code in Chrome will crash
-    // on dereference, but extension/Pepper APIs will guard against NULL and not
-    // crash.
-    DLOG(ERROR) << "Histogram " << name_ << " has bad construction arguments";
-    return nullptr;
+    // changed one of them, or simply by bad code within Chrome itself.  A NULL
+    // return would cause Chrome to crash; better to just record it for later
+    // analysis.
+    UmaHistogramSparse("Histogram.MismatchedConstructionArguments",
+                       static_cast<Sample>(HashMetricName(name_)));
+    DLOG(ERROR) << "Histogram " << name_
+                << " has mismatched construction arguments";
+    return DummyHistogram::GetInstance();
   }
   return histogram;
 }

base/metrics/histogram_unittest.cc

@@ -15,6 +15,7 @@
 
 #include "base/logging.h"
 #include "base/metrics/bucket_ranges.h"
+#include "base/metrics/dummy_histogram.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/metrics_hashes.h"
 #include "base/metrics/persistent_histogram_allocator.h"
@@ -657,10 +658,10 @@ TEST_P(HistogramTest, BadConstruction) {
   // Try to get the same histogram name with different arguments.
   HistogramBase* bad_histogram = Histogram::FactoryGet(
       "BadConstruction", 0, 100, 7, HistogramBase::kNoFlags);
-  EXPECT_EQ(nullptr, bad_histogram);
+  EXPECT_EQ(DummyHistogram::GetInstance(), bad_histogram);
   bad_histogram = Histogram::FactoryGet(
       "BadConstruction", 0, 99, 8, HistogramBase::kNoFlags);
-  EXPECT_EQ(nullptr, bad_histogram);
+  EXPECT_EQ(DummyHistogram::GetInstance(), bad_histogram);
 
   HistogramBase* linear_histogram = LinearHistogram::FactoryGet(
       "BadConstructionLinear", 0, 100, 8, HistogramBase::kNoFlags);
@@ -669,10 +670,10 @@ TEST_P(HistogramTest, BadConstruction) {
   // Try to get the same histogram name with different arguments.
   bad_histogram = LinearHistogram::FactoryGet(
       "BadConstructionLinear", 0, 100, 7, HistogramBase::kNoFlags);
-  EXPECT_EQ(nullptr, bad_histogram);
+  EXPECT_EQ(DummyHistogram::GetInstance(), bad_histogram);
   bad_histogram = LinearHistogram::FactoryGet(
       "BadConstructionLinear", 10, 100, 8, HistogramBase::kNoFlags);
-  EXPECT_EQ(nullptr, bad_histogram);
+  EXPECT_EQ(DummyHistogram::GetInstance(), bad_histogram);
 }
 
 TEST_P(HistogramTest, FactoryTime) {

base/metrics/single_sample_metrics_unittest.cc

@@ -5,6 +5,7 @@
 #include "base/metrics/single_sample_metrics.h"
 
 #include "base/memory/ptr_util.h"
+#include "base/metrics/dummy_histogram.h"
 #include "base/test/gtest_util.h"
 #include "base/test/histogram_tester.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -82,11 +83,13 @@ TEST_F(SingleSampleMetricsTest, DefaultSingleSampleMetricWithValue) {
 
   // Verify construction implicitly by requesting a histogram with the same
   // parameters; this test relies on the fact that histogram objects are unique
-  // per name. Different parameters will result in a nullptr being returned.
-  EXPECT_FALSE(
+  // per name. Different parameters will result in a Dummy histogram returned.
+  EXPECT_EQ(
+      DummyHistogram::GetInstance(),
       Histogram::FactoryGet(kMetricName, 1, 3, 3, HistogramBase::kNoFlags));
-  EXPECT_TRUE(Histogram::FactoryGet(kMetricName, kMin, kMax, kBucketCount,
-                                    HistogramBase::kUmaTargetedHistogramFlag));
+  EXPECT_NE(DummyHistogram::GetInstance(),
+            Histogram::FactoryGet(kMetricName, kMin, kMax, kBucketCount,
+                                  HistogramBase::kUmaTargetedHistogramFlag));
 }
 
 TEST_F(SingleSampleMetricsTest, MultipleMetricsAreDistinct) {

components/metrics/single_sample_metrics_factory_impl_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/metrics/single_sample_metrics_factory_impl.h"
 
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/dummy_histogram.h"
 #include "base/run_loop.h"
 #include "base/test/gtest_util.h"
 #include "base/test/histogram_tester.h"
@@ -124,12 +125,14 @@ TEST_F(SingleSampleMetricsFactoryImplTest, DefaultSingleSampleMetricWithValue) {
 
   // Verify construction implicitly by requesting a histogram with the same
   // parameters; this test relies on the fact that histogram objects are unique
-  // per name. Different parameters will result in a nullptr being returned.
-  EXPECT_FALSE(base::Histogram::FactoryGet(kMetricName, 1, 3, 3,
-                                           base::HistogramBase::kNoFlags));
-  EXPECT_TRUE(base::Histogram::FactoryGet(
-      kMetricName, kMin, kMax, kBucketCount,
-      base::HistogramBase::kUmaTargetedHistogramFlag));
+  // per name. Different parameters will result in a Dummy histogram returned.
+  EXPECT_EQ(base::DummyHistogram::GetInstance(),
+            base::Histogram::FactoryGet(kMetricName, 1, 3, 3,
+                                        base::HistogramBase::kNoFlags));
+  EXPECT_NE(base::DummyHistogram::GetInstance(),
+            base::Histogram::FactoryGet(
+                kMetricName, kMin, kMax, kBucketCount,
+                base::HistogramBase::kUmaTargetedHistogramFlag));
 }
 
 TEST_F(SingleSampleMetricsFactoryImplTest, MultithreadedMetrics) {

tools/metrics/histograms/histograms.xml

@@ -30180,6 +30180,18 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Histogram.MismatchedConstructionArguments"
+    enum="HistogramNameHash">
+  <owner>asvitkine@chromium.org</owner>
+  <owner>bcwhite@chromium.org</owner>
+  <summary>
+    The hash codes of histograms that were found to have construction arguments
+    different from a previous instantiation of the same name. Entries here have
+    conflicting definitions and should be investigated. Data collected for the
+    secondary definitions will be silently dropped.
+  </summary>
+</histogram>
+
 <histogram name="Histogram.PermanentNameChanged" enum="HistogramNameHash">
   <owner>asvitkine@chromium.org</owner>
   <owner>bcwhite@chromium.org</owner>