[histogram] Make histograms more resistant to overflows.

base/metrics/histogram.cc

Index: base/metrics/histogram.cc
diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index c35f76ce5548d8f0264f26fbeefab9c5d62aea82..b06090bcd3d2bb0340ab28ad9a56ea3a2176c334 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -414,7 +414,7 @@ bool Histogram::InspectConstructionArguments(const std::string& name,
 }
 
 uint64_t Histogram::name_hash() const {
-  return samples_->id();
+  return unlogged_samples_->id();
 }
 
 HistogramType Histogram::GetHistogramType() const {
@@ -445,7 +445,7 @@ void Histogram::AddCount(int value, int count) {
     NOTREACHED();
     return;
   }
-  samples_->Accumulate(value, count);
+  unlogged_samples_->Accumulate(value, count);
 
   FindAndRunCallback(value);
 }
@@ -457,17 +457,11 @@ std::unique_ptr<HistogramSamples> Histogram::SnapshotSamples() const {
 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();
-  }
-
-  // Subtract what was previously logged and update that information.
-  snapshot->Subtract(*logged_samples_);
+  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.

+
   return snapshot;
 }
 
@@ -475,20 +469,15 @@ 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.
@@ -521,8 +510,10 @@ Histogram::Histogram(const std::string& 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,
@@ -538,10 +529,10 @@ Histogram::Histogram(const std::string& name,
       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));
   }
 }
 
@@ -599,9 +590,16 @@ HistogramBase* Histogram::DeserializeInfoImpl(PickleIterator* iter) {
 }
 
 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;
 }