Revert of Collect information about failing histogram factory calls. (patchset #6 id:180001 of https://codereview.chromium.org/1719363002/ )

Reason for revert:
Build is no longer exhibiting crashes as before.  It's as though the debug code is preventing whatever error is causing the crashes, or at least those caused by the StatisticsRecorder finding the wrong Histogram.

More to follow...

Original issue's description:
> Collect information about failing histogram factory calls.
>
> Some tests need to be disabled because they create the same histogram with different parameters, a condition which does not always crash in the calling code but does with this CL.
>
> This CL will be reverted once the necessary information has been collected, including re-enabling of any tests.
>
> SHERIFFS: If a test breaks in the waterfall, you can either revert this CL or disable the test.  I'll re-enable it when reverting this after the necessary data has been collected.
>
> BUG=588946
>
> Committed: https://crrev.com/ce1315b94863f40d6323fa488490ed866aae4ed4
> Cr-Commit-Position: refs/heads/master@{#378957}
>
> Committed: https://crrev.com/63f1c5d846f6dac2cf148b81961c38b88ff1bd86
> Cr-Commit-Position: refs/heads/master@{#379323}
>
> Committed: https://crrev.com/89dca78efa00acb8fc8749adcc1a08a24eb105d5
> Cr-Commit-Position: refs/heads/master@{#379489}
>
> Committed: https://crrev.com/1375777f66fcf8edd3f393dadd9d9de6a7409030
> Cr-Commit-Position: refs/heads/master@{#379690}

TBR=asvitkine@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=588946

Review URL: https://codereview.chromium.org/1774973003

Cr-Commit-Position: refs/heads/master@{#379865}

4 files changed, 11 insertions, 110 deletions

diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index 7feacdb..ea4f816 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -154,22 +154,10 @@ HistogramBase* Histogram::Factory::Build() {
   ImportPersistentHistograms();
 
   HistogramBase* histogram = StatisticsRecorder::FindHistogram(name_);
-
-  // crbug.com/588946 debugging. See comment at end of function.
-  const BucketRanges* created_ranges =
-      reinterpret_cast<const BucketRanges*>(0xDEADBEEF);
-  const BucketRanges* registered_ranges =
-      reinterpret_cast<const BucketRanges*>(0xDEADBEEF);
-  HistogramBase* tentative_histogram =
-      reinterpret_cast<HistogramBase*>(0xDEADBEEF);
-  PersistentMemoryAllocator* allocator =
-      reinterpret_cast<PersistentMemoryAllocator*>(0xDEADBEEF);
-  PersistentMemoryAllocator::Reference histogram_ref = 0xDEADBEEF;
-
   if (!histogram) {
     // To avoid racy destruction at shutdown, the following will be leaked.
-    created_ranges = CreateRanges();
-    registered_ranges =
+    const BucketRanges* created_ranges = CreateRanges();
+    const BucketRanges* registered_ranges =
         StatisticsRecorder::RegisterOrDeleteDuplicateRanges(created_ranges);
 
     // In most cases, the bucket-count, minimum, and maximum values are known
@@ -182,17 +170,15 @@ HistogramBase* Histogram::Factory::Build() {
       minimum_ = registered_ranges->range(1);
       maximum_ = registered_ranges->range(bucket_count_ - 1);
     }
-    CHECK_LT(0, minimum_);
-    CHECK_LT(0, maximum_);
 
     // Try to create the histogram using a "persistent" allocator. As of
     // 2015-01-14, the availability of such is controlled by a base::Feature
     // that is off by default. If the allocator doesn't exist or if
     // allocating from it fails, code below will allocate the histogram from
     // the process heap.
-    histogram_ref = 0;
-    tentative_histogram = nullptr;
-    allocator =
+    PersistentMemoryAllocator::Reference histogram_ref = 0;
+    HistogramBase* tentative_histogram = nullptr;
+    PersistentMemoryAllocator* allocator =
         GetPersistentHistogramMemoryAllocator();
     if (allocator) {
       flags_ |= HistogramBase::kIsPersistent;
@@ -205,12 +191,6 @@ HistogramBase* Histogram::Factory::Build() {
           registered_ranges,
           flags_,
           &histogram_ref);
-      CHECK_LT(0, minimum_);
-      CHECK_LT(0, maximum_);
-      CHECK_EQ(minimum_,
-               static_cast<Histogram*>(tentative_histogram)->declared_min_);
-      CHECK_EQ(maximum_,
-               static_cast<Histogram*>(tentative_histogram)->declared_max_);
     }
 
     // Handle the case where no persistent allocator is present or the
@@ -220,12 +200,6 @@ HistogramBase* Histogram::Factory::Build() {
       DCHECK(!allocator);  // Shouldn't have failed.
       flags_ &= ~HistogramBase::kIsPersistent;
       tentative_histogram = HeapAlloc(registered_ranges);
-      CHECK_LT(0, minimum_);
-      CHECK_LT(0, maximum_);
-      CHECK_EQ(minimum_,
-               static_cast<Histogram*>(tentative_histogram)->declared_min_);
-      CHECK_EQ(maximum_,
-               static_cast<Histogram*>(tentative_histogram)->declared_max_);
     }
 
     FillHistogram(tentative_histogram);
@@ -240,15 +214,6 @@ HistogramBase* Histogram::Factory::Build() {
   }
 
   DCHECK_EQ(histogram_type_, histogram->GetHistogramType());
-  bool bad_args = false;
-  HistogramBase* existing_histogram = histogram;
-  HistogramType existing_type = histogram->GetHistogramType();
-  const char* existing_name = histogram->histogram_name().c_str();
-  Sample existing_minimum = static_cast<Histogram*>(histogram)->declared_min_;
-  Sample existing_maximum = static_cast<Histogram*>(histogram)->declared_max_;
-  uint32_t existing_bucket_count =
-    static_cast<Histogram*>(histogram)->bucket_count();
-
   if (bucket_count_ != 0 &&
       !histogram->HasConstructionArguments(minimum_, maximum_, bucket_count_)) {
     // The construction arguments do not match the existing histogram.  This can
@@ -258,45 +223,8 @@ HistogramBase* Histogram::Factory::Build() {
     // on dereference, but extension/Pepper APIs will guard against NULL and not
     // crash.
     DLOG(ERROR) << "Histogram " << name_ << " has bad construction arguments";
-    bad_args = true;
-    histogram = nullptr;
-  }
-
-#if !DCHECK_IS_ON()  // Don't affect tests, only release builds.
-  // For the moment, crash here so that collected crash reports have access
-  // to the construction values in order to figure out why this is failing.
-  // TODO(bcwhite): Remove this once crbug.com/588946 is resolved. Also remove
-  // from beta-branch because we don't want crashes due to misbehaving
-  // extensions (see comment above).
-  if (!histogram) {
-    HistogramType histogram_type = histogram_type_;
-    HistogramBase::Sample minimum = minimum_;
-    HistogramBase::Sample maximum = maximum_;
-    uint32_t bucket_count = bucket_count_;
-    int32_t flags = flags_;
-    CHECK(histogram) << name_ << ": bad-args=" << bad_args;
-    base::debug::Alias(&histogram_type);
-    base::debug::Alias(&minimum);
-    base::debug::Alias(&maximum);
-    base::debug::Alias(&bucket_count);
-    base::debug::Alias(&flags);
-    base::debug::Alias(&created_ranges);
-    base::debug::Alias(&registered_ranges);
-    base::debug::Alias(&histogram_ref);
-    base::debug::Alias(&tentative_histogram);
-    base::debug::Alias(&allocator);
-    base::debug::Alias(&tentative_histogram);
+    return nullptr;
   }
-#endif
-
-  // Down here so vars are always "used".
-  base::debug::Alias(&bad_args);
-  base::debug::Alias(&existing_histogram);
-  base::debug::Alias(&existing_type);
-  base::debug::Alias(&existing_name);
-  base::debug::Alias(&existing_minimum);
-  base::debug::Alias(&existing_maximum);
-  base::debug::Alias(&existing_bucket_count);
   return histogram;
 }
 
@@ -558,12 +486,8 @@ Histogram::Histogram(const std::string& name,
     bucket_ranges_(ranges),
     declared_min_(minimum),
     declared_max_(maximum) {
-  CHECK_LT(0, minimum);
-  CHECK_LT(0, maximum);
   if (ranges)
     samples_.reset(new SampleVector(HashMetricName(name), ranges));
-  CHECK_EQ(minimum, declared_min_);
-  CHECK_EQ(maximum, declared_max_);
 }
 
 Histogram::Histogram(const std::string& name,
@@ -579,16 +503,12 @@ Histogram::Histogram(const std::string& name,
     bucket_ranges_(ranges),
     declared_min_(minimum),
     declared_max_(maximum) {
-  CHECK_LT(0, minimum);
-  CHECK_LT(0, maximum);
   if (ranges) {
     samples_.reset(new SampleVector(HashMetricName(name),
                                     counts, counts_size, meta, ranges));
     logged_samples_.reset(new SampleVector(samples_->id(), logged_counts,
                                            counts_size, logged_meta, ranges));
   }
-  CHECK_EQ(minimum, declared_min_);
-  CHECK_EQ(maximum, declared_max_);
 }
 
 Histogram::~Histogram() {
diff --git a/base/metrics/histogram_persistence.cc b/base/metrics/histogram_persistence.cc
index 8b94647..f18d175 100644
--- a/base/metrics/histogram_persistence.cc
+++ b/base/metrics/histogram_persistence.cc
@@ -288,8 +288,6 @@ HistogramBase* CreatePersistentHistogram(
   HistogramBase::AtomicCount* logged_data =
       counts_data + histogram_data.bucket_count;
 
-  CHECK_LT(0, histogram_data.minimum);
-  CHECK_LT(0, histogram_data.maximum);
   std::string name(histogram_data_ptr->name);
   HistogramBase* histogram = nullptr;
   switch (histogram_data.histogram_type) {
diff --git a/base/metrics/histogram_unittest.cc b/base/metrics/histogram_unittest.cc
index e6aefd3..beb6a71 100644
--- a/base/metrics/histogram_unittest.cc
+++ b/base/metrics/histogram_unittest.cc
@@ -668,7 +668,7 @@ TEST_P(HistogramTest, CustomHistogramSerializeInfo) {
   EXPECT_FALSE(iter.SkipBytes(1));
 }
 
-TEST_P(HistogramTest, DISABLED_BadConstruction) {
+TEST_P(HistogramTest, BadConstruction) {
   HistogramBase* histogram = Histogram::FactoryGet(
       "BadConstruction", 0, 100, 8, HistogramBase::kNoFlags);
   EXPECT_TRUE(histogram->HasConstructionArguments(1, 100, 8));
diff --git a/base/metrics/statistics_recorder.cc b/base/metrics/statistics_recorder.cc
index 90d64bb..d0fa2ad 100644
--- a/base/metrics/statistics_recorder.cc
+++ b/base/metrics/statistics_recorder.cc
@@ -5,7 +5,6 @@
 #include "base/metrics/statistics_recorder.h"
 
 #include "base/at_exit.h"
-#include "base/debug/alias.h"
 #include "base/debug/leak_annotations.h"
 #include "base/json/string_escape.h"
 #include "base/logging.h"
@@ -124,17 +123,13 @@ HistogramBase* StatisticsRecorder::RegisterOrDeleteDuplicate(
         histogram_to_return = histogram;
       } else {
         // We already have one histogram with this name.
-        CHECK_EQ(histogram->histogram_name(),
-                 it->second->histogram_name()) << "hash collision";
+        DCHECK_EQ(histogram->histogram_name(),
+                  it->second->histogram_name()) << "hash collision";
         histogram_to_return = it->second;
         histogram_to_delete = histogram;
       }
-      base::debug::Alias(&it);
-      base::debug::Alias(&name);
-      base::debug::Alias(&name_hash);
     }
   }
-  base::debug::Alias(&histogram);
   delete histogram_to_delete;
   return histogram_to_return;
 }
@@ -284,22 +279,10 @@ HistogramBase* StatisticsRecorder::FindHistogram(base::StringPiece name) {
   if (histograms_ == NULL)
     return NULL;
 
-  const uint64_t lookup_hash = HashMetricName(name);
-  HistogramMap::iterator it = histograms_->find(lookup_hash);
+  HistogramMap::iterator it = histograms_->find(HashMetricName(name));
   if (histograms_->end() == it)
     return NULL;
-
-  const uint64_t existing_hash = it->first;
-  const char* existing_name = it->second->histogram_name().c_str();
-  HistogramMap* histograms = histograms_;
-  CHECK_EQ(name, it->second->histogram_name()) << "hash collision";
-  base::debug::Alias(&lookup_hash);
-  base::debug::Alias(&existing_hash);
-  base::debug::Alias(&name);
-  base::debug::Alias(&existing_name);
-  base::debug::Alias(&it);
-  base::debug::Alias(&histograms);
-
+  DCHECK_EQ(name, it->second->histogram_name()) << "hash collision";
   return it->second;
 }