Return a NULL histogram pointer on construction error

Check the pointer returned for the metricsPrivate API to keep from crashing Chrome with bad data.

BUG=334135

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

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

2 files changed, 46 insertions, 2 deletions

diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index fbe66d0..beb9b9e 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -110,7 +110,16 @@ HistogramBase* Histogram::FactoryGet(const string& name,
   }
 
   DCHECK_EQ(HISTOGRAM, histogram->GetHistogramType());
-  CHECK(histogram->HasConstructionArguments(minimum, maximum, bucket_count));
+  if (!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 NULL;
+  }
   return histogram;
 }
 
@@ -567,7 +576,16 @@ HistogramBase* LinearHistogram::FactoryGetWithRangeDescription(
   }
 
   DCHECK_EQ(LINEAR_HISTOGRAM, histogram->GetHistogramType());
-  CHECK(histogram->HasConstructionArguments(minimum, maximum, bucket_count));
+  if (!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 NULL;
+  }
   return histogram;
 }
 
diff --git a/base/metrics/histogram_unittest.cc b/base/metrics/histogram_unittest.cc
index e7a01aa5..5fbd28a 100644
--- a/base/metrics/histogram_unittest.cc
+++ b/base/metrics/histogram_unittest.cc
@@ -443,6 +443,32 @@ TEST_F(HistogramTest, CustomHistogramSerializeInfo) {
   EXPECT_FALSE(iter.SkipBytes(1));
 }
 
+TEST_F(HistogramTest, BadConstruction) {
+  HistogramBase* histogram = Histogram::FactoryGet(
+      "BadConstruction", 0, 100, 8, HistogramBase::kNoFlags);
+  EXPECT_TRUE(histogram->HasConstructionArguments(1, 100, 8));
+
+  // Try to get the same histogram name with different arguments.
+  HistogramBase* bad_histogram = Histogram::FactoryGet(
+      "BadConstruction", 0, 100, 7, HistogramBase::kNoFlags);
+  EXPECT_EQ(NULL, bad_histogram);
+  bad_histogram = Histogram::FactoryGet(
+      "BadConstruction", 0, 99, 8, HistogramBase::kNoFlags);
+  EXPECT_EQ(NULL, bad_histogram);
+
+  HistogramBase* linear_histogram = LinearHistogram::FactoryGet(
+      "BadConstructionLinear", 0, 100, 8, HistogramBase::kNoFlags);
+  EXPECT_TRUE(linear_histogram->HasConstructionArguments(1, 100, 8));
+
+  // Try to get the same histogram name with different arguments.
+  bad_histogram = LinearHistogram::FactoryGet(
+      "BadConstructionLinear", 0, 100, 7, HistogramBase::kNoFlags);
+  EXPECT_EQ(NULL, bad_histogram);
+  bad_histogram = LinearHistogram::FactoryGet(
+      "BadConstructionLinear", 10, 100, 8, HistogramBase::kNoFlags);
+  EXPECT_EQ(NULL, bad_histogram);
+}
+
 #if GTEST_HAS_DEATH_TEST
 // For Histogram, LinearHistogram and CustomHistogram, the minimum for a
 // declared range is 1, while the maximum is (HistogramBase::kSampleType_MAX -