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

net/disk_cache/stats_histogram.cc

Index: net/disk_cache/stats_histogram.cc
===================================================================
--- net/disk_cache/stats_histogram.cc	(revision 76117)
+++ net/disk_cache/stats_histogram.cc	(working copy)
@@ -32,12 +32,11 @@
   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());
@@ -48,6 +47,9 @@
   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;
 }
@@ -91,5 +93,10 @@
   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