Make non-matching histograms between parent and child process dump.

This follows up https://chromium-review.googlesource.com/5866873,
which updated the notreached logic around non-matching buckets.

Bug: 367379194
Change-Id: I383154cf5a62e68a4d382cb3fa0ccc063762ae26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5891286
Commit-Queue: Alexei Svitkine <[email protected]>
Reviewed-by: Luc Nguyen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1363290}


CrOS-Libchrome-Original-Commit: 1e86b0c896af21d2a2aee2545513c2af23e0b980

Author: asvitkine-chromium

base/metrics/dummy_histogram.cc

@@ -85,6 +85,10 @@ bool DummyHistogram::HasConstructionArguments(
   return true;
 }
 
+bool DummyHistogram::AddSamples(const HistogramSamples& samples) {
+  return true;
+}
+
 bool DummyHistogram::AddSamplesFromPickle(PickleIterator* iter) {
   return true;
 }

base/metrics/dummy_histogram.h

@@ -36,7 +36,7 @@ class BASE_EXPORT DummyHistogram : public HistogramBase {
                                 size_t expected_bucket_count) const override;
   void Add(Sample value) override {}
   void AddCount(Sample value, int count) override {}
-  void AddSamples(const HistogramSamples& samples) override {}
+  bool AddSamples(const HistogramSamples& samples) override;
   bool AddSamplesFromPickle(PickleIterator* iter) override;
   std::unique_ptr<HistogramSamples> SnapshotSamples() const override;
   std::unique_ptr<HistogramSamples> SnapshotUnloggedSamples() const override;

base/metrics/histogram.cc

@@ -589,8 +589,8 @@ std::unique_ptr<HistogramSamples> Histogram::SnapshotFinalDelta() const {
   return SnapshotUnloggedSamples();
 }
 
-void Histogram::AddSamples(const HistogramSamples& samples) {
-  unlogged_samples_->Add(samples);
+bool Histogram::AddSamples(const HistogramSamples& samples) {
+  return unlogged_samples_->Add(samples);
 }
 
 bool Histogram::AddSamplesFromPickle(PickleIterator* iter) {

base/metrics/histogram.h

@@ -236,7 +236,7 @@ class BASE_EXPORT Histogram : public HistogramBase {
   void MarkSamplesAsLogged(const HistogramSamples& samples) final;
   std::unique_ptr<HistogramSamples> SnapshotDelta() override;
   std::unique_ptr<HistogramSamples> SnapshotFinalDelta() const override;
-  void AddSamples(const HistogramSamples& samples) override;
+  bool AddSamples(const HistogramSamples& samples) override;
   bool AddSamplesFromPickle(base::PickleIterator* iter) override;
   base::Value::Dict ToGraphDict() const override;
 

base/metrics/histogram_base.h

@@ -202,7 +202,7 @@ class BASE_EXPORT HistogramBase {
   void AddTimeMicrosecondsGranularity(const TimeDelta& time);
   void AddBoolean(bool value);
 
-  virtual void AddSamples(const HistogramSamples& samples) = 0;
+  virtual bool AddSamples(const HistogramSamples& samples) = 0;
   virtual bool AddSamplesFromPickle(base::PickleIterator* iter) = 0;
 
   // Serialize the histogram info into |pickle|.

base/metrics/histogram_samples.cc

@@ -237,11 +237,10 @@ HistogramSamples::HistogramSamples(uint64_t id, std::unique_ptr<Metadata> meta)
 // be invalid by the time this dtor gets called.
 HistogramSamples::~HistogramSamples() = default;
 
-void HistogramSamples::Add(const HistogramSamples& other) {
+bool HistogramSamples::Add(const HistogramSamples& other) {
   IncreaseSumAndCount(other.sum(), other.redundant_count());
   std::unique_ptr<SampleCountIterator> it = other.Iterator();
-  bool success = AddSubtractImpl(it.get(), ADD);
-  DCHECK(success);
+  return AddSubtractImpl(it.get(), ADD);
 }
 
 bool HistogramSamples::AddFromPickle(PickleIterator* iter) {
@@ -257,14 +256,13 @@ bool HistogramSamples::AddFromPickle(PickleIterator* iter) {
   return AddSubtractImpl(&pickle_iter, ADD);
 }
 
-void HistogramSamples::Subtract(const HistogramSamples& other) {
+bool HistogramSamples::Subtract(const HistogramSamples& other) {
   IncreaseSumAndCount(-other.sum(), -other.redundant_count());
   std::unique_ptr<SampleCountIterator> it = other.Iterator();
-  bool success = AddSubtractImpl(it.get(), SUBTRACT);
-  DCHECK(success);
+  return AddSubtractImpl(it.get(), SUBTRACT);
 }
 
-void HistogramSamples::Extract(HistogramSamples& other) {
+bool HistogramSamples::Extract(HistogramSamples& other) {
   static_assert(sizeof(other.meta_->sum) == 8);
 
 #ifdef ARCH_CPU_64_BITS
@@ -294,8 +292,7 @@ void HistogramSamples::Extract(HistogramSamples& other) {
       subtle::NoBarrier_AtomicExchange(&other.meta_->redundant_count, 0);
   IncreaseSumAndCount(other_sum, other_redundant_count);
   std::unique_ptr<SampleCountIterator> it = other.ExtractingIterator();
-  bool success = AddSubtractImpl(it.get(), ADD);
-  DCHECK(success);
+  return AddSubtractImpl(it.get(), ADD);
 }
 
 bool HistogramSamples::IsDefinitelyEmpty() const {

base/metrics/histogram_samples.h

@@ -147,16 +147,16 @@ class BASE_EXPORT HistogramSamples {
   virtual HistogramBase::Count GetCount(HistogramBase::Sample value) const = 0;
   virtual HistogramBase::Count TotalCount() const = 0;
 
-  void Add(const HistogramSamples& other);
+  bool Add(const HistogramSamples& other);
 
   // Add from serialized samples.
   bool AddFromPickle(PickleIterator* iter);
 
-  void Subtract(const HistogramSamples& other);
+  bool Subtract(const HistogramSamples& other);
 
   // Adds the samples from |other| while also resetting |other|'s sample counts
   // to 0.
-  void Extract(HistogramSamples& other);
+  bool Extract(HistogramSamples& other);
 
   // Returns an iterator to read the sample counts.
   virtual std::unique_ptr<SampleCountIterator> Iterator() const = 0;

base/metrics/persistent_histogram_allocator.cc

@@ -104,60 +104,41 @@ size_t CalculateRequiredCountsBytes(size_t bucket_count) {
   return bucket_count * kBytesPerBucket;
 }
 
-void MergeSamplesToExistingHistogram(
+bool MergeSamplesToExistingHistogram(
     HistogramBase* existing,
     const HistogramBase* histogram,
     std::unique_ptr<HistogramSamples> samples) {
-#if !BUILDFLAG(IS_NACL)
-  // If the passed |histogram| does not match with |existing| (i.e. the one
-  // registered with the global StatisticsRecorder) due to not being the same
-  // type of histogram or due to specifying different buckets, then unexpected
-  // things may happen further down the line. This may be indicative that a
-  // child process is emitting a histogram with different parameters than the
-  // browser process, for example.
-  // TODO(crbug.com/40064026): Remove this. Used to investigate failures when
-  // merging histograms from an allocator to the global StatisticsRecorder.
-  bool histograms_match = true;
+  // Check if the histograms match, which is necessary for merging their data.
   HistogramType existing_type = existing->GetHistogramType();
   if (histogram->GetHistogramType() != existing_type) {
-    // Different histogram types.
-    histograms_match = false;
-  } else if (existing_type == HistogramType::HISTOGRAM ||
-             existing_type == HistogramType::LINEAR_HISTOGRAM ||
-             existing_type == HistogramType::BOOLEAN_HISTOGRAM ||
-             existing_type == HistogramType::CUSTOM_HISTOGRAM) {
+    return false;  // Merge failed due to different histogram types.
+  }
+
+  if (existing_type == HistogramType::HISTOGRAM ||
+      existing_type == HistogramType::LINEAR_HISTOGRAM ||
+      existing_type == HistogramType::BOOLEAN_HISTOGRAM ||
+      existing_type == HistogramType::CUSTOM_HISTOGRAM) {
     // Only numeric histograms make use of BucketRanges.
     const BucketRanges* existing_buckets =
         static_cast<const Histogram*>(existing)->bucket_ranges();
     const BucketRanges* histogram_buckets =
         static_cast<const Histogram*>(histogram)->bucket_ranges();
     // DCHECK because HasValidChecksum() recomputes the checksum which can be
     // expensive to do in a loop.
-    DCHECK(existing_buckets->HasValidChecksum() &&
-           histogram_buckets->HasValidChecksum());
+    DCHECK(existing_buckets->HasValidChecksum());
+    DCHECK(histogram_buckets->HasValidChecksum());
 
     if (existing_buckets->checksum() != histogram_buckets->checksum()) {
-      // Different buckets.
-      histograms_match = false;
+      return false;  // Merge failed due to different buckets.
     }
   }
 
-  if (!histograms_match) {
-    // If the histograms do not match, then the call to AddSamples() below might
-    // trigger a NOTREACHED(). Include the histogram name here for
-    // debugging purposes. This is not done in
-    // GetOrCreateStatisticsRecorderHistogram() directly, since that could
-    // incorrectly create crash reports for enum histograms that have newly
-    // appended entries (different bucket max and count).
-    SCOPED_CRASH_KEY_STRING256("PersistentHistogramAllocator", "histogram",
-                               existing->histogram_name());
-    existing->AddSamples(*samples);
-    return;
-  }
-#endif  // !BUILDFLAG(IS_NACL)
-
   // Merge the delta from the passed object to the one in the SR.
-  existing->AddSamples(*samples);
+
+  // It's possible for the buckets to differ but their checksums to match due
+  // to a collision, in which case AddSamples() will return false, which we
+  // propagate to the caller (indicating histogram mismatch).
+  return existing->AddSamples(*samples);
 }
 
 }  // namespace
@@ -504,7 +485,7 @@ void PersistentHistogramAllocator::FinalizeHistogram(Reference ref,
   }
 }
 
-void PersistentHistogramAllocator::MergeHistogramDeltaToStatisticsRecorder(
+bool PersistentHistogramAllocator::MergeHistogramDeltaToStatisticsRecorder(
     HistogramBase* histogram) {
   DCHECK(histogram);
 
@@ -513,20 +494,21 @@ void PersistentHistogramAllocator::MergeHistogramDeltaToStatisticsRecorder(
   // the StatisticsRecorder, which requires acquiring a lock.
   std::unique_ptr<HistogramSamples> samples = histogram->SnapshotDelta();
   if (samples->IsDefinitelyEmpty()) {
-    return;
+    return true;
   }
 
   HistogramBase* existing = GetOrCreateStatisticsRecorderHistogram(histogram);
   if (!existing) {
     // The above should never fail but if it does, no real harm is done.
     // Some metric data will be lost but that is better than crashing.
-    return;
+    return false;
   }
 
-  MergeSamplesToExistingHistogram(existing, histogram, std::move(samples));
+  return MergeSamplesToExistingHistogram(existing, histogram,
+                                         std::move(samples));
 }
 
-void PersistentHistogramAllocator::MergeHistogramFinalDeltaToStatisticsRecorder(
+bool PersistentHistogramAllocator::MergeHistogramFinalDeltaToStatisticsRecorder(
     const HistogramBase* histogram) {
   DCHECK(histogram);
 
@@ -535,17 +517,18 @@ void PersistentHistogramAllocator::MergeHistogramFinalDeltaToStatisticsRecorder(
   // requires acquiring a lock.
   std::unique_ptr<HistogramSamples> samples = histogram->SnapshotFinalDelta();
   if (samples->IsDefinitelyEmpty()) {
-    return;
+    return true;
   }
 
   HistogramBase* existing = GetOrCreateStatisticsRecorderHistogram(histogram);
   if (!existing) {
     // The above should never fail but if it does, no real harm is done.
     // Some metric data will be lost but that is better than crashing.
-    return;
+    return false;
   }
 
-  MergeSamplesToExistingHistogram(existing, histogram, std::move(samples));
+  return MergeSamplesToExistingHistogram(existing, histogram,
+                                         std::move(samples));
 }
 
 std::unique_ptr<PersistentSampleMapRecords>

base/metrics/persistent_histogram_allocator.h

@@ -264,13 +264,18 @@ class BASE_EXPORT PersistentHistogramAllocator {
   // StatisticsRecorder, updating the "logged" samples within the passed
   // object so that repeated merges are allowed. Don't call this on a "global"
   // allocator because histograms created there will already be in the SR.
-  void MergeHistogramDeltaToStatisticsRecorder(HistogramBase* histogram);
+  // Returns whether the merge was successful; if false, the histogram did not
+  // have the same shape (different types or buckets), or we couldn't get a
+  // target histogram from the statistic recorder.
+  bool MergeHistogramDeltaToStatisticsRecorder(HistogramBase* histogram);
 
   // As above but merge the "final" delta. No update of "logged" samples is
   // done which means it can operate on read-only objects. It's essential,
   // however, not to call this more than once or those final samples will
-  // get recorded again.
-  void MergeHistogramFinalDeltaToStatisticsRecorder(
+  // get recorded again. Returns whether the merge was successful; if false, the
+  // histogram did not have the same shape (different types or buckets), or we
+  // couldn't get a target histogram from the statistic recorder.
+  bool MergeHistogramFinalDeltaToStatisticsRecorder(
       const HistogramBase* histogram);
 
   // Returns an object that manages persistent-sample-map records for a given

base/metrics/sparse_histogram.cc

@@ -181,9 +181,9 @@ std::unique_ptr<HistogramSamples> SparseHistogram::SnapshotFinalDelta() const {
   return std::move(snapshot);
 }
 
-void SparseHistogram::AddSamples(const HistogramSamples& samples) {
+bool SparseHistogram::AddSamples(const HistogramSamples& samples) {
   base::AutoLock auto_lock(lock_);
-  unlogged_samples_->Add(samples);
+  return unlogged_samples_->Add(samples);
 }
 
 bool SparseHistogram::AddSamplesFromPickle(PickleIterator* iter) {