Return a NULL histogram pointer on construction error

base/metrics/histogram.cc

Index: base/metrics/histogram.cc
diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index fbe66d05d29fd3c2896f8499dd1dc07d9793d095..52dc8ef656d8a372d53803171fd779a52a5ece33 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -76,6 +76,23 @@ bool ValidateRangeChecksum(const HistogramBase& histogram,
   return casted_histogram.bucket_ranges()->checksum() == range_checksum;
 }
 
+// DummyHistogram is an object that overrides Histogram's Add* functions
+// to provide no functionality. This can be returned in the case of mismatched
+// construction arguments to any of the other Histograms' FactoryGet calls.
+class DummyHistogram : public Histogram {
+ public:
+  virtual void Add(Sample value) OVERRIDE {}
+  virtual void AddSamples(const HistogramSamples& samples) OVERRIDE {}
+  virtual bool AddSamplesFromPickle(PickleIterator* iter) OVERRIDE {
+    return false;
+  }
+
+  DummyHistogram() : Histogram("Dummy", 0, 1, NULL) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DummyHistogram);
+};
+
 }  // namespace
 
 typedef HistogramBase::Count Count;
@@ -110,7 +127,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)) {

[jar (doing other things), 2014/01/23 02:16:18]

This is a sad thing... as it means we can't catch folks that try to construct a
histogram with different args.

Is there any way to preserve this CHECK?

+    // 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.  We return a dummy object here instead of crashing
+    // so poor use of extension/Pepper APIs do not cause crashes in Chrome.
+    DLOG(ERROR) << "Histogram " << name << " has bad construction arguments";
+    DummyHistogram* tentative_histogram = new DummyHistogram;
+    histogram =
+        StatisticsRecorder::RegisterOrDeleteDuplicate(tentative_histogram);
+  }
   return histogram;
 }
 
@@ -567,7 +593,16 @@ HistogramBase* LinearHistogram::FactoryGetWithRangeDescription(
   }
 
   DCHECK_EQ(LINEAR_HISTOGRAM, histogram->GetHistogramType());
-  CHECK(histogram->HasConstructionArguments(minimum, maximum, bucket_count));

[jar (doing other things), 2014/01/23 02:16:18]

Same issue... I really wish we could preserve this... and maybe ignore *only*
the case where a plugin provides the change.

Can to adjust things to catch this further up the stack, closer to your use
case??  ... and avoid calling this far in?

Your use case involves a slow find anyway (and no use of a static), so it is
probably plausible to call to check for the histogram's presence, and get its
args, before going further (but it has been years since I've looked at this
code).

[Alexei Svitkine (slow), 2014/01/23 15:28:22]

Personally, I think this is a reasonable place to have this check because this
is the codepath of the "slow find". Meaning that for regular macro'd histograms,
they'd only hit this check once, and from then on cache a pointer to the
histogram, whereas for dynamic users of the API, they'd do the check every time.
This seems like a reasonable trade off.

Also note that this shouldn't add any extra performance loss, since the check
was being done already (just in a CHECK() rather than an if).

I do somewhat agree that it would also be nice to preserve the CHECK at the same
time, but I'm not sure of a good way to do that. Hmm, perhaps the factory API
could take a parameter specifying what the failure mode is (i.e. whether a dummy
object is returned vs. a check is hit) and the default being the CHECK? WDYT?

[elijahtaylor1, 2014/01/24 01:27:26]

I've implemented this suggestion to see how ugly it might be.  It means adding
optional parameters to 5 different function and propagating that parameter
across all of them, but not too bad.

I'm not in love with the names I chose for the new parameter, if either of you
have suggestions to improve them, please offer them.

+  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.  We return a dummy object here instead of crashing
+    // so poor use of extension/Pepper APIs do not cause crashes in Chrome.
+    DLOG(ERROR) << "Histogram " << name << " has bad construction arguments";
+    DummyHistogram* tentative_histogram = new DummyHistogram;
+    histogram =
+        StatisticsRecorder::RegisterOrDeleteDuplicate(tentative_histogram);
+  }
   return histogram;
 }