Return a NULL histogram pointer on construction error

base/metrics/histogram.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Histogram is an object that aggregates statistics, and can summarize them in
 // various forms, including ASCII graphical, HTML, and numerically (as a
 // vector of numbers corresponding to each of the aggregating buckets).
 // See header file for details and examples.
 
 #include "base/metrics/histogram.h"
@@ skipping 58 matching lines @@
 }
 
 bool ValidateRangeChecksum(const HistogramBase& histogram,
                            uint32 range_checksum) {
   const Histogram& casted_histogram =
       static_cast<const Histogram&>(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;
 typedef HistogramBase::Sample Sample;
 
 // static
 const size_t Histogram::kBucketCount_MAX = 16384u;
 
 HistogramBase* Histogram::FactoryGet(const string& name,
                                      Sample minimum,
@@ skipping 14 matching lines @@
 
     Histogram* tentative_histogram =
         new Histogram(name, minimum, maximum, registered_ranges);
 
     tentative_histogram->SetFlags(flags);
     histogram =
         StatisticsRecorder::RegisterOrDeleteDuplicate(tentative_histogram);
   }
 
   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;
 }
 
 HistogramBase* Histogram::FactoryTimeGet(const string& name,
                                          TimeDelta minimum,
                                          TimeDelta maximum,
                                          size_t bucket_count,
                                          int32 flags) {
   return FactoryGet(name, minimum.InMilliseconds(), maximum.InMilliseconds(),
                     bucket_count, flags);
@@ skipping 436 matching lines @@
             descriptions[i].description;
       }
     }
 
     tentative_histogram->SetFlags(flags);
     histogram =
         StatisticsRecorder::RegisterOrDeleteDuplicate(tentative_histogram);
   }
 
   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;
 }
 
 HistogramType LinearHistogram::GetHistogramType() const {
   return LINEAR_HISTOGRAM;
 }
 
 LinearHistogram::LinearHistogram(const string& name,
                                  Sample minimum,
                                  Sample maximum,
@@ skipping 244 matching lines @@
 
   BucketRanges* bucket_ranges = new BucketRanges(ranges.size());
   for (size_t i = 0; i < ranges.size(); i++) {
     bucket_ranges->set_range(i, ranges[i]);
   }
   bucket_ranges->ResetChecksum();
   return bucket_ranges;
 }
 
 }  // namespace base