Detect corruption of previous snapshots in histograms

Having verified that histograms can be corrupted by
random memory smashers (or DRAM problems), this CL
looks at one last example of histograms that are
at rest for an extended period of time, and hence
vulnerable.  Between each UMA upload, we save
snapshots of the data we've already sent, so that
we can just send "new samples."  If those snapshots
are corrupted, the noise would be directly
injected into the UMA uploads.  This CL checks
for consistency in those snapshots, and if there
is any inconsistency, it skips over the
recent changes (since it has no baseline).

Since the code to do this was getting larger,
I factored it out a bit.  The hassle is that
I wanted to keep separate records of corruption
in the renderer vs browser, which complicates
the factoring a bit.

BUG=61281
r=mbelshe
Review URL: http://codereview.chromium.org/4733002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@65675 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 166 insertions, 112 deletions

diff --git a/chrome/common/metrics_helpers.cc b/chrome/common/metrics_helpers.cc
index e25355c..d5d8f78 100644
--- a/chrome/common/metrics_helpers.cc
+++ b/chrome/common/metrics_helpers.cc
@@ -16,7 +16,6 @@
 #include "base/file_util.h"
 #include "base/md5.h"
 #include "base/perftimer.h"
-#include "base/scoped_ptr.h"
 #include "base/string_number_conversions.h"
 #include "base/sys_info.h"
 #include "base/utf_string_conversions.h"
@@ -413,8 +412,7 @@ void MetricsLogBase::RecordHistogramDelta(
 MetricsServiceBase::MetricsServiceBase()
     : pending_log_(NULL),
       compressed_log_(),
-      current_log_(NULL),
-      logged_samples_() {
+      current_log_(NULL) {
 }
 
 MetricsServiceBase::~MetricsServiceBase() {
@@ -467,20 +465,56 @@ bool MetricsServiceBase::Bzip2Compress(const std::string& input,
   return true;
 }
 
+void MetricsServiceBase::DiscardPendingLog() {
+  if (pending_log_) {  // Shutdown might have deleted it!
+    delete pending_log_;
+    pending_log_ = NULL;
+  }
+  compressed_log_.clear();
+}
+
 void MetricsServiceBase::RecordCurrentHistograms() {
   DCHECK(current_log_);
+  TransmitAllHistograms(base::Histogram::kNoFlags, true);
+}
+
+void MetricsServiceBase::TransmitHistogramDelta(
+      const base::Histogram& histogram,
+      const base::Histogram::SampleSet& snapshot) {
+  current_log_->RecordHistogramDelta(histogram, snapshot);
+}
 
+void MetricsServiceBase::InconsistencyDetected(int problem) {
+  UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesBrowser",
+                            problem, Histogram::NEVER_EXCEEDED_VALUE);
+}
+
+void MetricsServiceBase::UniqueInconsistencyDetected(int problem) {
+  UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesBrowserUnique",
+                            problem, Histogram::NEVER_EXCEEDED_VALUE);
+}
+
+void MetricsServiceBase::SnapshotProblemResolved(int amount) {
+  UMA_HISTOGRAM_COUNTS("Histogram.InconsistentSnapshotBrowser",
+                       std::abs(amount));
+}
+
+void HistogramSender::TransmitAllHistograms(Histogram::Flags flag_to_set,
+                                            bool send_only_uma) {
   StatisticsRecorder::Histograms histograms;
   StatisticsRecorder::GetHistograms(&histograms);
   for (StatisticsRecorder::Histograms::const_iterator it = histograms.begin();
        histograms.end() != it;
        ++it) {
-    if ((*it)->flags() & Histogram::kUmaTargetedHistogramFlag)
-      RecordHistogram(**it);
+    (*it)->SetFlags(flag_to_set);
+    if (send_only_uma &&
+        0 == ((*it)->flags() & Histogram::kUmaTargetedHistogramFlag))
+      continue;
+    TransmitHistogram(**it);
   }
 }
 
-void MetricsServiceBase::RecordHistogram(const Histogram& histogram) {
+void HistogramSender::TransmitHistogram(const Histogram& histogram) {
   // Get up-to-date snapshot of sample stats.
   Histogram::SampleSet snapshot;
   histogram.SnapshotSample(&snapshot);
@@ -489,21 +523,20 @@ void MetricsServiceBase::RecordHistogram(const Histogram& histogram) {
   int corruption = histogram.FindCorruption(snapshot);
   if (corruption) {
     NOTREACHED();
+    InconsistencyDetected(corruption);
     // Don't send corrupt data to metrics survices.
-    UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesBrowser",
-                              corruption, Histogram::NEVER_EXCEEDED_VALUE);
-    typedef std::map<std::string, int> ProblemMap;
-    static ProblemMap* inconsistencies = new ProblemMap;
-    int old_corruption = (*inconsistencies)[histogram_name];
+    if (NULL == inconsistencies_.get())
+      inconsistencies_.reset(new ProblemMap);
+    int old_corruption = (*inconsistencies_)[histogram_name];
     if (old_corruption == (corruption | old_corruption))
       return;  // We've already seen this corruption for this histogram.
-    (*inconsistencies)[histogram_name] |= corruption;
-    UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesBrowserUnique",
-                              corruption, Histogram::NEVER_EXCEEDED_VALUE);
+    (*inconsistencies_)[histogram_name] |= corruption;
+    UniqueInconsistencyDetected(corruption);
     return;
   }
 
-  // Find the already sent stats, or create an empty set.
+  // Find the already sent stats, or create an empty set.  Remove from our
+  // snapshot anything that we've already sent.
   LoggedSampleMap::iterator it = logged_samples_.find(histogram_name);
   Histogram::SampleSet* already_logged;
   if (logged_samples_.end() == it) {
@@ -512,23 +545,28 @@ void MetricsServiceBase::RecordHistogram(const Histogram& histogram) {
     already_logged->Resize(histogram);  // Complete initialization.
   } else {
     already_logged = &(it->second);
+    int64 discrepancy(already_logged->TotalCount() -
+                    already_logged->redundant_count());
+    if (discrepancy) {
+      NOTREACHED();  // Already_logged has become corrupt.
+      int problem = static_cast<int>(discrepancy);
+      if (problem != discrepancy)
+        problem = INT_MAX;
+      SnapshotProblemResolved(problem);
+      // With no valid baseline, we'll act like we've sent everything in our
+      // snapshot.
+      already_logged->Subtract(*already_logged);
+      already_logged->Add(snapshot);
+    }
     // Deduct any stats we've already logged from our snapshot.
     snapshot.Subtract(*already_logged);
   }
 
   // Snapshot now contains only a delta to what we've already_logged.
-
-  if (snapshot.TotalCount() > 0) {
-    current_log_->RecordHistogramDelta(histogram, snapshot);
+  DCHECK_EQ(snapshot.TotalCount(), snapshot.redundant_count());
+  if (snapshot.redundant_count() > 0) {
+    TransmitHistogramDelta(histogram, snapshot);
     // Add new data into our running total.
     already_logged->Add(snapshot);
   }
 }
-
-void MetricsServiceBase::DiscardPendingLog() {
-  if (pending_log_) {  // Shutdown might have deleted it!
-    delete pending_log_;
-    pending_log_ = NULL;
-  }
-  compressed_log_.clear();
-}
diff --git a/chrome/common/metrics_helpers.h b/chrome/common/metrics_helpers.h
index b0bbec2..b261488 100644
--- a/chrome/common/metrics_helpers.h
+++ b/chrome/common/metrics_helpers.h
@@ -14,6 +14,7 @@
 
 #include "base/basictypes.h"
 #include "base/metrics/histogram.h"
+#include "base/scoped_ptr.h"
 #include "base/time.h"
 #include "chrome/common/page_transition_types.h"
 
@@ -173,11 +174,62 @@ class MetricsLogBase {
   DISALLOW_COPY_AND_ASSIGN(MetricsLogBase);
 };
 
+// HistogramSender handles the logistics of gathering up available histograms
+// for transmission (such as from renderer to browser, or from browser to UMA
+// upload).  It has several pure virtual functions that are replaced in
+// derived classes to allow the exact lower level transmission mechanism,
+// or error report mechanism, to be replaced.  Since histograms can sit in
+// memory for an extended period of time, and are vulnerable to memory
+// corruption, this class also validates as much rendundancy as it can before
+// calling for the marginal change (a.k.a., delta) in a histogram to be sent
+// onward.
+class HistogramSender {
+ protected:
+  HistogramSender() {}
+  virtual ~HistogramSender() {}
+
+  // Snapshot all histograms, and transmit the delta.
+  // The arguments allow a derived class to select only a subset for
+  // transmission, or to set a flag in each transmitted histogram.
+  void TransmitAllHistograms(base::Histogram::Flags flags_to_set,
+                             bool send_only_uma);
+
+  // Send the histograms onward, as defined in a derived class.
+  // This is only called with a delta, listing samples that have not previously
+  // been transmitted.
+  virtual void TransmitHistogramDelta(
+      const base::Histogram& histogram,
+      const base::Histogram::SampleSet& snapshot) = 0;
+
+  // Record various errors found during attempts to send histograms.
+  virtual void InconsistencyDetected(int problem) = 0;
+  virtual void UniqueInconsistencyDetected(int problem) = 0;
+  virtual void SnapshotProblemResolved(int amount) = 0;
+
+ private:
+  // Maintain a map of histogram names to the sample stats we've sent.
+  typedef std::map<std::string, base::Histogram::SampleSet> LoggedSampleMap;
+  // List of histograms names, and their encontered corruptions.
+  typedef std::map<std::string, int> ProblemMap;
+
+  // Snapshot this histogram, and transmit the delta.
+  void TransmitHistogram(const base::Histogram& histogram);
+
+  // For histograms, record what we've already transmitted (as a sample for each
+  // histogram) so that we can send only the delta with the next log.
+  LoggedSampleMap logged_samples_;
+
+  // List of histograms found corrupt to be corrupt, and their problems.
+  scoped_ptr<ProblemMap> inconsistencies_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistogramSender);
+};
+
 // This class provides base functionality for logging metrics data.
 // TODO(ananta)
 // Factor out more common code from chrome and chrome frame metrics service
 // into this class.
-class MetricsServiceBase {
+class MetricsServiceBase : public HistogramSender {
  protected:
   MetricsServiceBase();
   virtual ~MetricsServiceBase();
@@ -199,9 +251,6 @@ class MetricsServiceBase {
   // Called when we close a log.
   void RecordCurrentHistograms();
 
-  // Record a specific histogram .
-  void RecordHistogram(const base::Histogram& histogram);
-
   // A log that we are currently transmiting, or about to try to transmit.
   MetricsLogBase* pending_log_;
 
@@ -213,12 +262,14 @@ class MetricsServiceBase {
   // The log that we are still appending to.
   MetricsLogBase* current_log_;
 
-  // Maintain a map of histogram names to the sample stats we've sent.
-  typedef std::map<std::string, base::Histogram::SampleSet> LoggedSampleMap;
-
-  // For histograms, record what we've already logged (as a sample for each
-  // histogram) so that we can send only the delta with the next log.
-  LoggedSampleMap logged_samples_;
+ private:
+  // HistogramSender interface (override) methods.
+  virtual void TransmitHistogramDelta(
+      const base::Histogram& histogram,
+      const base::Histogram::SampleSet& snapshot);
+  virtual void InconsistencyDetected(int problem);
+  virtual void UniqueInconsistencyDetected(int problem);
+  virtual void SnapshotProblemResolved(int amount);
 
   DISALLOW_COPY_AND_ASSIGN(MetricsServiceBase);
 };
diff --git a/chrome/renderer/renderer_histogram_snapshots.cc b/chrome/renderer/renderer_histogram_snapshots.cc
index 6124e04..bc91647 100644
--- a/chrome/renderer/renderer_histogram_snapshots.cc
+++ b/chrome/renderer/renderer_histogram_snapshots.cc
@@ -20,7 +20,7 @@ using base::StatisticsRecorder;
 
 RendererHistogramSnapshots::RendererHistogramSnapshots()
     : ALLOW_THIS_IN_INITIALIZER_LIST(
-        renderer_histogram_snapshots_factory_(this)) {
+          renderer_histogram_snapshots_factory_(this)) {
 }
 
 RendererHistogramSnapshots::~RendererHistogramSnapshots() {
@@ -34,81 +34,43 @@ void RendererHistogramSnapshots::SendHistograms(int sequence_number) {
 }
 
 void RendererHistogramSnapshots::UploadAllHistrograms(int sequence_number) {
-  StatisticsRecorder::Histograms histograms;
-  StatisticsRecorder::GetHistograms(&histograms);
+  DCHECK_EQ(0u, pickled_histograms_.size());
 
-  HistogramPickledList pickled_histograms;
+  // Push snapshots into our pickled_histograms_ vector.
+  TransmitAllHistograms(Histogram::kIPCSerializationSourceFlag, false);
 
-  for (StatisticsRecorder::Histograms::iterator it = histograms.begin();
-       histograms.end() != it;
-       it++) {
-    (*it)->SetFlags(Histogram::kIPCSerializationSourceFlag);
-    UploadHistrogram(**it, &pickled_histograms);
-  }
   // Send the sequence number and list of pickled histograms over synchronous
-  // IPC.
+  // IPC, so we can clear pickled_histograms_ afterwards.
   RenderThread::current()->Send(
       new ViewHostMsg_RendererHistograms(
-          sequence_number, pickled_histograms));
-}
+          sequence_number, pickled_histograms_));
 
-// Extract snapshot data, remember what we've seen so far, and then send off the
-// delta to the browser.
-void RendererHistogramSnapshots::UploadHistrogram(
-    const Histogram& histogram,
-    HistogramPickledList* pickled_histograms) {
-  // Get up-to-date snapshot of sample stats.
-  Histogram::SampleSet snapshot;
-  histogram.SnapshotSample(&snapshot);
-  const std::string& histogram_name = histogram.histogram_name();
-
-  int corruption = histogram.FindCorruption(snapshot);
-  if (corruption) {
-    NOTREACHED();
-    // Don't send corrupt data to the browser.
-    UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesRenderer",
-                              corruption, Histogram::NEVER_EXCEEDED_VALUE);
-    typedef std::map<std::string, int> ProblemMap;
-    static ProblemMap* inconsistencies = new ProblemMap;
-    int old_corruption = (*inconsistencies)[histogram_name];
-    if (old_corruption == (corruption | old_corruption))
-      return;  // We've already seen this corruption for this histogram.
-    (*inconsistencies)[histogram_name] |= corruption;
-    UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesRendererUnique",
-                              corruption, Histogram::NEVER_EXCEEDED_VALUE);
-    return;
-  }
-
-  // Find the already sent stats, or create an empty set.
-  LoggedSampleMap::iterator it = logged_samples_.find(histogram_name);
-  Histogram::SampleSet* already_logged;
-  if (logged_samples_.end() == it) {
-    // Add new entry.
-    already_logged = &logged_samples_[histogram.histogram_name()];
-    already_logged->Resize(histogram);  // Complete initialization.
-  } else {
-    already_logged = &(it->second);
-    // Deduct any stats we've already logged from our snapshot.
-    snapshot.Subtract(*already_logged);
-  }
-
-  // Snapshot now contains only a delta to what we've already_logged.
-
-  if (snapshot.TotalCount() > 0) {
-    UploadHistogramDelta(histogram, snapshot, pickled_histograms);
-    // Add new data into our running total.
-    already_logged->Add(snapshot);
-  }
+  pickled_histograms_.clear();
 }
 
-void RendererHistogramSnapshots::UploadHistogramDelta(
-    const Histogram& histogram,
-    const Histogram::SampleSet& snapshot,
-    HistogramPickledList* pickled_histograms) {
-  DCHECK(0 != snapshot.TotalCount());
+void RendererHistogramSnapshots::TransmitHistogramDelta(
+      const base::Histogram& histogram,
+      const base::Histogram::SampleSet& snapshot) {
+  DCHECK_NE(0, snapshot.TotalCount());
   snapshot.CheckSize(histogram);
 
   std::string histogram_info =
       Histogram::SerializeHistogramInfo(histogram, snapshot);
-  pickled_histograms->push_back(histogram_info);
+  pickled_histograms_.push_back(histogram_info);
+}
+
+void RendererHistogramSnapshots::InconsistencyDetected(int problem) {
+  UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesRenderer",
+                            problem, Histogram::NEVER_EXCEEDED_VALUE);
+}
+
+void RendererHistogramSnapshots::UniqueInconsistencyDetected(int problem) {
+  UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesRendererUnique",
+                            problem, Histogram::NEVER_EXCEEDED_VALUE);
+}
+
+void RendererHistogramSnapshots::SnapshotProblemResolved(int amount) {
+  UMA_HISTOGRAM_COUNTS("Histogram.InconsistentSnapshotRenderer",
+                       std::abs(amount));
 }
+
diff --git a/chrome/renderer/renderer_histogram_snapshots.h b/chrome/renderer/renderer_histogram_snapshots.h
index eff6f3e..5155b6f 100644
--- a/chrome/renderer/renderer_histogram_snapshots.h
+++ b/chrome/renderer/renderer_histogram_snapshots.h
@@ -14,8 +14,9 @@
 #include "base/metrics/histogram.h"
 #include "base/process.h"
 #include "base/task.h"
+#include "chrome/common/metrics_helpers.h"
 
-class RendererHistogramSnapshots {
+class RendererHistogramSnapshots : public HistogramSender {
  public:
   RendererHistogramSnapshots();
   ~RendererHistogramSnapshots();
@@ -23,26 +24,28 @@ class RendererHistogramSnapshots {
   // Send the histogram data.
   void SendHistograms(int sequence_number);
 
+ private:
   // Maintain a map of histogram names to the sample stats we've sent.
   typedef std::map<std::string, base::Histogram::SampleSet> LoggedSampleMap;
   typedef std::vector<std::string> HistogramPickledList;
 
- private:
   // Extract snapshot data and then send it off the the Browser process.
   // Send only a delta to what we have already sent.
   void UploadAllHistrograms(int sequence_number);
-  void UploadHistrogram(const base::Histogram& histogram,
-                        HistogramPickledList* histograms);
-  void UploadHistogramDelta(const base::Histogram& histogram,
-                            const base::Histogram::SampleSet& snapshot,
-                            HistogramPickledList* histograms);
 
   ScopedRunnableMethodFactory<RendererHistogramSnapshots>
       renderer_histogram_snapshots_factory_;
 
-  // For histograms, record what we've already logged (as a sample for each
-  // histogram) so that we can send only the delta with the next log.
-  LoggedSampleMap logged_samples_;
+  // HistogramSender interface (override) methods.
+  void TransmitHistogramDelta(
+      const base::Histogram& histogram,
+      const base::Histogram::SampleSet& snapshot);
+  void InconsistencyDetected(int problem);
+  void UniqueInconsistencyDetected(int problem);
+  void SnapshotProblemResolved(int amount);
+
+  // Collection of histograms to send to the browser.
+  HistogramPickledList pickled_histograms_;
 
   DISALLOW_COPY_AND_ASSIGN(RendererHistogramSnapshots);
 };