Add more instrumentation to diagnose an Android memory corruption.

This change:
  - Does a separate pass on all histograms, validating them, before
    starting to prepare their deltas. This will confirm that data is
    bad already before we start processing them (i.e. verify that it's
    not the processing code having some side effect).
  - Counts the number of corrupted histograms to tell us if it's a single
    bad value or more widespread corruption.
  - Keeps track of the last histogram that someone logged something to,
    so that we could have a trail of the last thing that happened before
    corruption.
  - Moves a previously added instrumentation call to within the correct if
    block.

BUG=736675,744734

Change-Id: I4b860ad6e977c1555409bd628f9e4e5147e61654
Reviewed-on: https://chromium-review.googlesource.com/581418
Commit-Queue: Alexei Svitkine (slow) <asvitkine@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488815}

base/metrics/histogram.cc

@@ -39,6 +39,9 @@ namespace base {
 
 namespace {
 
+// TODO(asvitkine): Remove this after crbug/736675.
+char g_last_logged_histogram_name[256] = {0};
+
 bool ReadHistogramArguments(PickleIterator* iter,
                             std::string* histogram_name,
                             int* flags,
@@ -462,6 +465,9 @@ void Histogram::AddCount(int value, int count) {
   DCHECK_EQ(0, ranges(0));
   DCHECK_EQ(kSampleType_MAX, ranges(bucket_count()));
 
+  strlcpy(g_last_logged_histogram_name, histogram_name().c_str(),
+          sizeof(g_last_logged_histogram_name));
+
   if (value > kSampleType_MAX - 1)
     value = kSampleType_MAX - 1;
   if (value < 0)
@@ -532,7 +538,8 @@ void Histogram::WriteAscii(std::string* output) const {
   WriteAsciiImpl(true, "\n", output);
 }
 
-void Histogram::ValidateHistogramContents() const {
+bool Histogram::ValidateHistogramContents(bool crash_if_invalid,
+                                          int corrupted_count) const {
   enum Fields : int {
     kBucketRangesField,
     kUnloggedSamplesField,
@@ -556,17 +563,21 @@ void Histogram::ValidateHistogramContents() const {
   if (flags() == 0)
     bad_fields |= 1 << kFlagsField;
 
+  const bool is_valid = (bad_fields & ~(1 << kFlagsField)) == 0;
+  if (is_valid || !crash_if_invalid)
+    return is_valid;
+
   // Abort if a problem is found (except "flags", which could legally be zero).
-  if ((bad_fields & ~(1 << kFlagsField)) != 0) {
-    const std::string debug_string =
-        base::StringPrintf("%s/%" PRIu32, histogram_name().c_str(), bad_fields);
+  const std::string debug_string = base::StringPrintf(
+      "%s/%" PRIu32 "/%d/%s", histogram_name().c_str(), bad_fields,
+      corrupted_count, g_last_logged_histogram_name);
 #if !defined(OS_NACL)
-    // Temporary for https://crbug.com/736675.
-    base::debug::ScopedCrashKey crash_key("bad_histogram", debug_string);
+  // Temporary for https://crbug.com/736675.
+  base::debug::ScopedCrashKey crash_key("bad_histogram", debug_string);
 #endif
-    CHECK(false) << debug_string;
-    debug::Alias(&bad_fields);
-  }
+  CHECK(false) << debug_string;
+  debug::Alias(&bad_fields);
+  return false;
 }
 
 bool Histogram::SerializeInfoImpl(Pickle* pickle) const {

base/metrics/histogram.h

@@ -207,8 +207,14 @@ class BASE_EXPORT Histogram : public HistogramBase {
   void WriteHTMLGraph(std::string* output) const override;
   void WriteAscii(std::string* output) const override;
 
+  // Validates the histogram contents. If |crash_if_invalid| is true and the
+  // histogram is invalid, this will trigger a CHECK. Otherwise, it will return
+  // a bool indicating if the histogram is valid. |corrupted_count| is extra
+  // information the caller can provide about the number of corrupt histograms
+  // if available.
   // TODO(bcwhite): Remove this after crbug/736675.
-  void ValidateHistogramContents() const override;
+  bool ValidateHistogramContents(bool crash_if_invalid,
+                                 int corrupted_count) const override;
 
  protected:
   // This class, defined entirely within the .cc file, contains all the

base/metrics/histogram_base.cc

@@ -102,6 +102,11 @@ uint32_t HistogramBase::FindCorruption(const HistogramSamples& samples) const {
   return NO_INCONSISTENCIES;
 }
 
+bool HistogramBase::ValidateHistogramContents(bool crash_if_invalid,
+                                              int corrupted_count) const {
+  return true;
+}
+
 void HistogramBase::WriteJSON(std::string* output) const {
   Count count;
   int64_t sum;

base/metrics/histogram_base.h

@@ -211,7 +211,8 @@ class BASE_EXPORT HistogramBase {
   virtual void WriteAscii(std::string* output) const = 0;
 
   // TODO(bcwhite): Remove this after crbug/736675.
-  virtual void ValidateHistogramContents() const {}
+  virtual bool ValidateHistogramContents(bool crash_if_invalid,
+                                         int corrupted_count) const;
 
   // Produce a JSON representation of the histogram. This is implemented with
   // the help of GetParameters and GetCountAndBucketData; overwrite them to

base/metrics/histogram_snapshot_manager.cc

@@ -45,13 +45,13 @@ HistogramSnapshotManager::~HistogramSnapshotManager() {
 }
 
 void HistogramSnapshotManager::PrepareDelta(HistogramBase* histogram) {
-  histogram->ValidateHistogramContents();
+  histogram->ValidateHistogramContents(true, 0);
   PrepareSamples(histogram, histogram->SnapshotDelta());
 }
 
 void HistogramSnapshotManager::PrepareFinalDelta(
     const HistogramBase* histogram) {
-  histogram->ValidateHistogramContents();
+  histogram->ValidateHistogramContents(true, 0);
   PrepareSamples(histogram, histogram->SnapshotFinalDelta());
 }
 

base/metrics/histogram_snapshot_manager.h

@@ -33,6 +33,23 @@ class BASE_EXPORT HistogramSnapshotManager {
   explicit HistogramSnapshotManager(HistogramFlattener* histogram_flattener);
   virtual ~HistogramSnapshotManager();
 
+  // TODO(asvitkine): Remove this after crbug/736675.
+  template <class ForwardHistogramIterator>
+  void ValidateAllHistograms(ForwardHistogramIterator begin,
+                             ForwardHistogramIterator end) {
+    HistogramBase* last_invalid_histogram = nullptr;
+    int invalid_count = 0;
+    for (ForwardHistogramIterator it = begin; it != end; ++it) {
+      const bool is_valid = (*it)->ValidateHistogramContents(false, 0);
+      if (!is_valid) {
+        ++invalid_count;
+        last_invalid_histogram = *it;
+      }
+    }
+    if (last_invalid_histogram)
+      last_invalid_histogram->ValidateHistogramContents(true, invalid_count);
+  }
+
   // Snapshot all histograms, and ask |histogram_flattener_| to record the
   // delta. |flags_to_set| is used to set flags for each histogram.
   // |required_flags| is used to select histograms to be recorded.
@@ -44,6 +61,7 @@ class BASE_EXPORT HistogramSnapshotManager {
                      ForwardHistogramIterator end,
                      HistogramBase::Flags flags_to_set,
                      HistogramBase::Flags required_flags) {
+    ValidateAllHistograms(begin, end);
     for (ForwardHistogramIterator it = begin; it != end; ++it) {
       (*it)->SetFlags(flags_to_set);
       if (((*it)->flags() & required_flags) == required_flags)

base/metrics/persistent_histogram_allocator.cc

@@ -679,11 +679,11 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::CreateHistogram(
     DCHECK_EQ(histogram_type, histogram->GetHistogramType());
     histogram->SetFlags(histogram_flags);
     RecordCreateHistogramResult(CREATE_HISTOGRAM_SUCCESS);
+    histogram->ValidateHistogramContents(true, 0);
   } else {
     RecordCreateHistogramResult(CREATE_HISTOGRAM_UNKNOWN_TYPE);
   }
 
-  histogram->ValidateHistogramContents();
   return histogram;
 }
 

build/sanitizers/tsan_suppressions.cc

@@ -265,6 +265,9 @@ char kTSanDefaultSuppressions[] =
     //
     "race:third_party/harfbuzz-ng/src/*\n"
 
+    // TODO(asvitkine): Remove this after crbug/736675.
+    "race:base::Histogram::AddCount\n"
+
     // End of suppressions.
     ;  // Please keep this semicolon.