Crash when we notice a corruption of the histogram range-vector...

base/metrics/histogram.h

 // Copyright (c) 2011 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).
 
 // It supports calls to accumulate either time intervals (which are processed
 // as integral number of milliseconds), or arbitrary integral units.
@@ skipping 393 matching lines @@
   virtual Inconsistencies FindCorruption(const SampleSet& snapshot) const;
 
   //----------------------------------------------------------------------------
   // Accessors for factory constuction, serialization and testing.
   //----------------------------------------------------------------------------
   virtual ClassType histogram_type() const;
   const std::string& histogram_name() const { return histogram_name_; }
   Sample declared_min() const { return declared_min_; }
   Sample declared_max() const { return declared_max_; }
   virtual Sample ranges(size_t i) const;
-  Sample range_checksum() const { return range_checksum_; }
+  uint32 range_checksum() const { return range_checksum_; }
   virtual size_t bucket_count() const;
   // Snapshot the current complete set of sample data.
   // Override with atomic/locked snapshot if needed.
   virtual void SnapshotSample(SampleSet* sample) const;
 
   virtual bool HasConstructorArguments(Sample minimum, Sample maximum,
                                        size_t bucket_count);
 
   virtual bool HasConstructorTimeDeltaArguments(TimeDelta minimum,
                                                 TimeDelta maximum,
                                                 size_t bucket_count);
+  // Return true iff the range_checksum_ matches current ranges_ vector.
+  bool HasValidRangeChecksum() const;
 
  protected:
   friend class base::RefCountedThreadSafe<Histogram>;
   Histogram(const std::string& name, Sample minimum,
             Sample maximum, size_t bucket_count);
   Histogram(const std::string& name, TimeDelta minimum,
             TimeDelta maximum, size_t bucket_count);
 
   virtual ~Histogram();
 
@@ skipping 26 matching lines @@
 
   //----------------------------------------------------------------------------
   // Accessors for derived classes.
   //----------------------------------------------------------------------------
   void SetBucketRange(size_t i, Sample value);
 
   // Validate that ranges_ was created sensibly (top and bottom range
   // values relate properly to the declared_min_ and declared_max_)..
   bool ValidateBucketRanges() const;
 
+  virtual uint32 CalculateRangeChecksum() const;
+
  private:
   // Allow tests to corrupt our innards for testing purposes.
   FRIEND_TEST(HistogramTest, CorruptBucketBounds);
   FRIEND_TEST(HistogramTest, CorruptSampleCounts);
+  FRIEND_TEST(HistogramTest, Crc32SampleHash);
+  FRIEND_TEST(HistogramTest, Crc32TableTest);
 
   // Post constructor initialization.
   void Initialize();
 
-  // Return true iff the range_checksum_ matches current ranges_ vector.
-  bool HasValidRangeChecksum() const;
-
-  Sample CalculateRangeChecksum() const;
+  // Checksum function for accumulating range values into a checksum.
+  static uint32 Crc32(uint32 sum, Sample range);
 
   //----------------------------------------------------------------------------
   // Helpers for emitting Ascii graphic.  Each method appends data to output.
 
   // Find out how large the (graphically) the largest bucket will appear to be.
   double GetPeakBucketSize(const SampleSet& snapshot) const;
 
   // Write a common header message describing this histogram.
   void WriteAsciiHeader(const SampleSet& snapshot,
                         Count sample_count, std::string* output) const;
 
   // Write information about previous, current, and next buckets.
   // Information such as cumulative percentage, etc.
   void WriteAsciiBucketContext(const int64 past, const Count current,
                                const int64 remaining, const size_t i,
                                std::string* output) const;
 
   // Write textual description of the bucket contents (relative to histogram).
   // Output is the count in the buckets, as well as the percentage.
   void WriteAsciiBucketValue(Count current, double scaled_sum,
                              std::string* output) const;
 
   // Produce actual graph (set of blank vs non blank char's) for a bucket.
   void WriteAsciiBucketGraph(double current_size, double max_size,
                              std::string* output) const;
 
   //----------------------------------------------------------------------------
+  // Table for generating Crc32 values.
+  static const uint32 kCrcTable[256];
+  //----------------------------------------------------------------------------
   // Invariant values set at/near construction time
 
   // ASCII version of original name given to the constructor.  All identically
   // named instances will be coalesced cross-project.
   const std::string histogram_name_;
   Sample declared_min_;  // Less than this goes into counts_[0]
   Sample declared_max_;  // Over this goes into counts_[bucket_count_ - 1].
   size_t bucket_count_;  // Dimension of counts_[].
 
   // Flag the histogram for recording by UMA via metric_services.h.
   Flags flags_;
 
   // For each index, show the least value that can be stored in the
   // corresponding bucket. We also append one extra element in this array,
   // containing kSampleType_MAX, to make calculations easy.
   // The dimension of ranges_ is bucket_count + 1.
   Ranges ranges_;
 
-  // For redundancy, we store the sum of all the sample ranges when ranges are
-  // generated.  If ever there is ever a difference, then the histogram must
+  // For redundancy, we store a checksum of all the sample ranges when ranges
+  // are generated.  If ever there is ever a difference, then the histogram must
   // have been corrupted.
-  Sample range_checksum_;
+  uint32 range_checksum_;
 
   // Finally, provide the state that changes with the addition of each new
   // sample.
   SampleSet sample_;
 
   DISALLOW_COPY_AND_ASSIGN(Histogram);
 };
 
 //------------------------------------------------------------------------------
 
@@ skipping 75 matching lines @@
       const std::vector<Sample>& custom_ranges, Flags flags);
 
   // Overridden from Histogram:
   virtual ClassType histogram_type() const;
 
  protected:
   CustomHistogram(const std::string& name,
                   const std::vector<Sample>& custom_ranges);
 
   // Initialize ranges_ mapping.
-  virtual void InitializeBucketRange();
+  virtual void InitializeBucketRange(const std::vector<Sample>& custom_ranges);
   virtual double GetBucketSize(Count current, size_t i) const;
 
- private:
-  // Temporary pointer used during construction/initialization, and then NULLed.
-  const std::vector<Sample>* ranges_vector_;
-
   DISALLOW_COPY_AND_ASSIGN(CustomHistogram);
 };
 
 //------------------------------------------------------------------------------
 // StatisticsRecorder handles all histograms in the system.  It provides a
 // general place for histograms to register, and supports a global API for
 // accessing (i.e., dumping, or graphing) the data in all the histograms.
 
 class StatisticsRecorder {
  public:
   typedef std::vector<scoped_refptr<Histogram> > Histograms;
 
   StatisticsRecorder();
 
   ~StatisticsRecorder();
 
   // Find out if histograms can now be registered into our list.
   static bool IsActive();
 
-  // Register, or add a new histogram to the collection of statistics.
-  static void Register(Histogram* histogram);
+  // Register, or add a new histogram to the collection of statistics. If an
+  // identically named histogram is already registered, then the argument
+  // |histogram| will be replaced by the previously registered value.
+  static void Register(scoped_refptr<Histogram>* histogram);

[rvargas (doing something else), 2011/02/28 19:50:53]

In general, I would consider this to be unexpected behavior given the function
name. I suggest changing the name to reflecting the new behavior.

[jar (doing other things), 2011/02/28 22:11:44]

I landed the code this morning... so.... 
I've changed the name in a separate CL, 6591052, along with
a few other small items.  I'll send you a review request shortly.

 
   // Methods for printing histograms.  Only histograms which have query as
   // a substring are written to output (an empty string will process all
   // registered histograms).
   static void WriteHTMLGraph(const std::string& query, std::string* output);
   static void WriteGraph(const std::string& query, std::string* output);
 
   // Method for extracting histograms which were marked for use by UMA.
   static void GetHistograms(Histograms* output);
 
@@ skipping 25 matching lines @@
 
   // Dump all known histograms to log.
   static bool dump_on_exit_;
 
   DISALLOW_COPY_AND_ASSIGN(StatisticsRecorder);
 };
 
 }  // namespace base
 
 #endif  // BASE_METRICS_HISTOGRAM_H_