diff options
author | bcwhite <bcwhite@chromium.org> | 2016-03-11 04:50:37 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2016-03-11 12:51:53 +0000 |
commit | ccb033d86c3371aa90b079b7bac879fd536e02ad (patch) | |
tree | b6a1a477bf076ff8f98d323f0309428b92d85db9 | |
parent | 3b43e5a81c83605870ba5406143fcc5bb885890b (diff) | |
download | chromium_src-ccb033d86c3371aa90b079b7bac879fd536e02ad.zip chromium_src-ccb033d86c3371aa90b079b7bac879fd536e02ad.tar.gz chromium_src-ccb033d86c3371aa90b079b7bac879fd536e02ad.tar.bz2 |
Revert of Collect information about failing histogram factory calls. (patchset #8 id:240001 of https://codereview.chromium.org/1719363002/ )
Reason for revert:
Confirmed in 51.0.2674. Reverting while crashes are analyzed.
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}
>
> Committed: https://crrev.com/0abc5c2d2cb7ac70e26eab73d7e3f61f0472461e
> Cr-Commit-Position: refs/heads/master@{#380070}
>
> Committed: https://crrev.com/1d50ab3eec1dab7d3a6c70ec85b8937d5073b66d
> Cr-Commit-Position: refs/heads/master@{#380474}
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/1777223004
Cr-Commit-Position: refs/heads/master@{#380612}
-rw-r--r-- | base/metrics/histogram.cc | 92 | ||||
-rw-r--r-- | base/metrics/histogram_persistence.cc | 8 | ||||
-rw-r--r-- | base/metrics/histogram_unittest.cc | 2 | ||||
-rw-r--r-- | base/metrics/metrics_hashes.cc | 10 | ||||
-rw-r--r-- | base/metrics/statistics_recorder.cc | 25 | ||||
-rw-r--r-- | chrome/browser/extensions/api/metrics_private/metrics_apitest.cc | 2 |
6 files changed, 14 insertions, 125 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(®istered_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 34b3100..f18d175 100644 --- a/base/metrics/histogram_persistence.cc +++ b/base/metrics/histogram_persistence.cc @@ -244,12 +244,6 @@ HistogramBase* CreatePersistentHistogram( // validated below; the local copy is to ensure that the contents cannot // be externally changed between validation and use. PersistentHistogramData histogram_data = *histogram_data_ptr; - CHECK_EQ(histogram_data.histogram_type, histogram_data_ptr->histogram_type); - CHECK_EQ(histogram_data.flags, histogram_data_ptr->flags); - CHECK_EQ(histogram_data.minimum, histogram_data_ptr->minimum); - CHECK_EQ(histogram_data.maximum, histogram_data_ptr->maximum); - CHECK_EQ(histogram_data.bucket_count, histogram_data_ptr->bucket_count); - CHECK_EQ(histogram_data.ranges_checksum, histogram_data_ptr->ranges_checksum); HistogramBase::Sample* ranges_data = allocator->GetAsObject<HistogramBase::Sample>(histogram_data.ranges_ref, @@ -294,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/metrics_hashes.cc b/base/metrics/metrics_hashes.cc index 54c117b..5672b06 100644 --- a/base/metrics/metrics_hashes.cc +++ b/base/metrics/metrics_hashes.cc @@ -4,7 +4,6 @@ #include "base/metrics/metrics_hashes.h" -#include "base/debug/alias.h" #include "base/logging.h" #include "base/md5.h" #include "base/sys_byteorder.h" @@ -16,14 +15,9 @@ namespace { // Converts the 8-byte prefix of an MD5 hash into a uint64_t value. inline uint64_t DigestToUInt64(const base::MD5Digest& digest) { uint64_t value; - CHECK_GE(sizeof(digest.a), sizeof(value)); + DCHECK_GE(sizeof(digest.a), sizeof(value)); memcpy(&value, digest.a, sizeof(value)); - uint64_t hash = base::NetToHost64(value); - CHECK_NE(0U, hash); - base::debug::Alias(&hash); - base::debug::Alias(&value); - base::debug::Alias(&digest); - return hash; + return base::NetToHost64(value); } } // namespace 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; } diff --git a/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc b/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc index d4c5e3c..3160400 100644 --- a/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc +++ b/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc @@ -128,7 +128,7 @@ void ValidateHistograms(const RecordedHistogram* recorded, } // anonymous namespace -IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_Metrics) { +IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Metrics) { base::UserActionTester user_action_tester; base::FieldTrialList::CreateFieldTrial("apitestfieldtrial2", "group1"); |