[histogram] Make histograms more resistant to overflows.

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 396 matching lines @@
 
   if (!check_okay) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("Histogram.BadConstructionArguments",
                                 static_cast<Sample>(HashMetricName(name)));
   }
 
   return check_okay;
 }
 
 uint64_t Histogram::name_hash() const {
-  return samples_->id();
+  return unlogged_samples_->id();
 }
 
 HistogramType Histogram::GetHistogramType() const {
   return HISTOGRAM;
 }
 
 bool Histogram::HasConstructionArguments(Sample expected_minimum,
                                          Sample expected_maximum,
                                          uint32_t expected_bucket_count) const {
   return ((expected_minimum == declared_min_) &&
@@ skipping 10 matching lines @@
   DCHECK_EQ(kSampleType_MAX, ranges(bucket_count()));
 
   if (value > kSampleType_MAX - 1)
     value = kSampleType_MAX - 1;
   if (value < 0)
     value = 0;
   if (count <= 0) {
     NOTREACHED();
     return;
   }
-  samples_->Accumulate(value, count);
+  unlogged_samples_->Accumulate(value, count);
 
   FindAndRunCallback(value);
 }
 
 std::unique_ptr<HistogramSamples> Histogram::SnapshotSamples() const {
   return SnapshotSampleVector();
 }
 
 std::unique_ptr<HistogramSamples> Histogram::SnapshotDelta() {
   DCHECK(!final_delta_created_);
 
-  std::unique_ptr<HistogramSamples> snapshot = SnapshotSampleVector();
-  if (!logged_samples_) {
-    // If nothing has been previously logged, save this one as
-    // |logged_samples_| and gather another snapshot to return.
-    logged_samples_.swap(snapshot);
-    return SnapshotSampleVector();
-  }
+  std::unique_ptr<HistogramSamples> snapshot =
+      CloneSampleVector(*unlogged_samples_);
+  unlogged_samples_->Subtract(*snapshot);
+  logged_samples_->Add(*snapshot);

[Ilya Sherman, 2017/05/09 22:15:32]

I'm noticing that the samples in this vector could still overflow.  Might that
be an issue?  Please audit code that ends up reading values from this vector to
evaluate.  I'm not worried about the about:histograms UI, nor test code, since
overflow is both (a) unexpected and (b) not a big deal in those cases.  But I'm
not sure whether there are perhaps other parts of the code that might break from
overflow.  (I do realize that your CL doesn't directly make this situation
worse.  However, it does encourage people to author histograms that might
overflow, so it might have an impact in practice.)

[altimin, 2017/05/10 11:50:38]

I can see three call sites to Histogram::SnapshotSamples:
* SnapshotHistogramToValue in task_scheduler_internals
- Snapshots data for the UI.
* LogAggregatedMetrics in components/metrics/net
- Used for network errors. No real danger of overflowing here.
* OnSetUMACallback in tracing.
- This does not look at count, only at values.

But I think we should add a comment to HistogramBase::SnapshotSamples with a
warning.

 
-  // Subtract what was previously logged and update that information.
-  snapshot->Subtract(*logged_samples_);
-  logged_samples_->Add(*snapshot);
   return snapshot;
 }
 
 std::unique_ptr<HistogramSamples> Histogram::SnapshotFinalDelta() const {
   DCHECK(!final_delta_created_);
   final_delta_created_ = true;
 
-  std::unique_ptr<HistogramSamples> snapshot = SnapshotSampleVector();
-
-  // Subtract what was previously logged and then return.
-  if (logged_samples_)
-    snapshot->Subtract(*logged_samples_);
-  return snapshot;
+  return CloneSampleVector(*unlogged_samples_);
 }
 
 void Histogram::AddSamples(const HistogramSamples& samples) {
-  samples_->Add(samples);
+  unlogged_samples_->Add(samples);
 }
 
 bool Histogram::AddSamplesFromPickle(PickleIterator* iter) {
-  return samples_->AddFromPickle(iter);
+  return unlogged_samples_->AddFromPickle(iter);
 }
 
 // The following methods provide a graphical histogram display.
 void Histogram::WriteHTMLGraph(std::string* output) const {
   // TBD(jar) Write a nice HTML bar chart, with divs an mouse-overs etc.
   output->append("<PRE>");
   WriteAsciiImpl(true, "<br>", output);
   output->append("</PRE>");
 }
 
@@ skipping 12 matching lines @@
 }
 
 Histogram::Histogram(const std::string& name,
                      Sample minimum,
                      Sample maximum,
                      const BucketRanges* ranges)
   : HistogramBase(name),
     bucket_ranges_(ranges),
     declared_min_(minimum),
     declared_max_(maximum) {
-  if (ranges)
-    samples_.reset(new SampleVector(HashMetricName(name), ranges));
+  if (ranges) {
+    unlogged_samples_.reset(new SampleVector(HashMetricName(name), ranges));
+    logged_samples_.reset(new SampleVector(HashMetricName(name), ranges));

[Ilya Sherman, 2017/05/09 22:15:32]

nit: Could you avoid the second HashMetricName call?

[altimin, 2017/05/10 11:50:38]

Done.

+  }
 }
 
 Histogram::Histogram(const std::string& name,
                      Sample minimum,
                      Sample maximum,
                      const BucketRanges* ranges,
                      const DelayedPersistentAllocation& counts,
                      const DelayedPersistentAllocation& logged_counts,
                      HistogramSamples::Metadata* meta,
                      HistogramSamples::Metadata* logged_meta)
     : HistogramBase(name),
       bucket_ranges_(ranges),
       declared_min_(minimum),
       declared_max_(maximum) {
   if (ranges) {
-    samples_.reset(
+    unlogged_samples_.reset(
         new PersistentSampleVector(HashMetricName(name), ranges, meta, counts));
     logged_samples_.reset(new PersistentSampleVector(
-        samples_->id(), ranges, logged_meta, logged_counts));
+        unlogged_samples_->id(), ranges, logged_meta, logged_counts));
   }
 }
 
 Histogram::~Histogram() {
 }
 
 bool Histogram::PrintEmptyBucket(uint32_t index) const {
   return true;
 }
 
@@ skipping 37 matching lines @@
       histogram_name, declared_min, declared_max, bucket_count, flags);
 
   if (!ValidateRangeChecksum(*histogram, range_checksum)) {
     // The serialized histogram might be corrupted.
     return NULL;
   }
   return histogram;
 }
 
 std::unique_ptr<SampleVector> Histogram::SnapshotSampleVector() const {
-  std::unique_ptr<SampleVector> samples(
-      new SampleVector(samples_->id(), bucket_ranges()));
-  samples->Add(*samples_);
+  std::unique_ptr<SampleVector> samples = CloneSampleVector(*unlogged_samples_);
+  samples->Add(*logged_samples_);
   return samples;
 }
 
+std::unique_ptr<SampleVector> Histogram::CloneSampleVector(

[Ilya Sherman, 2017/05/09 22:15:32]

nit: I'd name this SnapshotUnloggedSamples, and drop the param, since the param
is always the same.

[altimin, 2017/05/10 11:50:38]

Nice, thanks!
Done.

+    const HistogramSamples& original) const {
+  std::unique_ptr<SampleVector> samples =
+      base::MakeUnique<SampleVector>(original.id(), bucket_ranges());
+  samples->Add(original);
+  return samples;
+}
+
 void Histogram::WriteAsciiImpl(bool graph_it,
                                const std::string& newline,
                                std::string* output) const {
   // Get local (stack) copies of all effectively volatile class data so that we
   // are consistent across our output activities.
   std::unique_ptr<SampleVector> snapshot = SnapshotSampleVector();
   Count sample_count = snapshot->TotalCount();
 
   WriteAsciiHeader(*snapshot, sample_count, output);
   output->append(newline);
@@ skipping 585 matching lines @@
     Sample sample = custom_ranges[i];
     if (sample < 0 || sample > HistogramBase::kSampleType_MAX - 1)
       return false;
     if (sample != 0)
       has_valid_range = true;
   }
   return has_valid_range;
 }
 
 }  // namespace base