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

net/disk_cache/stats_histogram.cc

 // Copyright (c) 2008 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.
 
 #include "net/disk_cache/stats_histogram.h"
 
 #include "base/logging.h"
 #include "net/disk_cache/stats.h"
 
 namespace disk_cache {
@@ skipping 14 matching lines @@
     const std::string& name) {
   scoped_refptr<Histogram> histogram(NULL);
 
   Sample minimum = 1;
   Sample maximum = disk_cache::Stats::kDataSizesLength - 1;
   size_t bucket_count = disk_cache::Stats::kDataSizesLength;
 
   if (StatisticsRecorder::FindHistogram(name, &histogram)) {
     DCHECK(histogram.get() != NULL);
   } else {
-    histogram = new StatsHistogram(name, minimum, maximum, bucket_count);
-    scoped_refptr<Histogram> registered_histogram(NULL);
-    StatisticsRecorder::FindHistogram(name, &registered_histogram);
-    if (registered_histogram.get() != NULL &&
-        registered_histogram.get() != histogram.get())
-      histogram = registered_histogram;
+    StatsHistogram* stats_histogram = new StatsHistogram(name, minimum, maximum,

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

Why use a temp variable at all?

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

This is subtle, and perchance worth a comment.

If I put it directly into histogram, then it is implicitly down-cast to a
Histogram type.  At that point, I'm no longer allowed to call the protected
InitializeBucketRange method.  I have to remain as a derived type until after
that call, and then I can down-cast to a Histogram and Register().

[rvargas (doing something else), 2011/02/28 23:05:33]

Thanks for the explanation. Wouldn't it be better just to make histogram a
StatsHistogram, and also get rid of return_histogram? Just thinking out loud.

[jar (doing other things), 2011/03/01 00:21:18]

You're code was fairly special (starting around lines 47) where you manually
up-cast to a derived class.  The pattern I used here (leading to line 42) mimics
what I did in the other derived class (which didn't need an upcast).

The original call to FindHistogram() handles only the base class type, and can't
accept a StatsHistogram* (or smart pointer).  The Register() function also has
to take a base-class type (now that it too can do an assignment to the
referenced argument), so it too can't take a StatsHistogram.

By keeping the pattern more similar in this derived class, I figured it would be
easier (in the future, if necessary) to merge future changes.

+                                                         bucket_count);
+    stats_histogram->InitializeBucketRange();
+    histogram = stats_histogram;
+    StatisticsRecorder::Register(&histogram);
   }
 
   DCHECK(HISTOGRAM == histogram->histogram_type());
   DCHECK(histogram->HasConstructorArguments(minimum, maximum, bucket_count));
 
   // We're preparing for an otherwise unsafe upcast by ensuring we have the
   // proper class type.
   Histogram* temp_histogram = histogram.get();
   StatsHistogram* temp_stats_histogram =
       static_cast<StatsHistogram*>(temp_histogram);
+  // Validate upcast by seeing that we're probably providing the checksum.
+  CHECK_EQ(temp_stats_histogram->StatsHistogram::CalculateRangeChecksum(),
+           temp_stats_histogram->CalculateRangeChecksum());
   scoped_refptr<StatsHistogram> return_histogram(temp_stats_histogram);
   return return_histogram;
 }
 
 bool StatsHistogram::Init(const Stats* stats) {
   DCHECK(stats);
   if (stats_)
     return false;
 
   SetFlags(kUmaTargetedHistogramFlag);
@@ skipping 23 matching lines @@
   // Only report UMA data once.
   StatsHistogram* mutable_me = const_cast<StatsHistogram*>(this);
   mutable_me->ClearFlags(kUmaTargetedHistogramFlag);
 }
 
 Histogram::Inconsistencies StatsHistogram::FindCorruption(
     const SampleSet& snapshot) const {
   return NO_INCONSISTENCIES;  // This class won't monitor inconsistencies.
 }
 
+uint32 StatsHistogram::CalculateRangeChecksum() const {
+  // We don't calculate checksums, so at least establish a unique constant.
+  const uint32 kStatsHistogramChecksum = 0x0cecce;
+  return kStatsHistogramChecksum;
+}
 
 }  // namespace disk_cache