From c4d376cf625b863b93e0d0e408a40de04592fbc1 Mon Sep 17 00:00:00 2001 From: Claudio DeSouza Date: Fri, 21 Feb 2025 17:52:32 +0000 Subject: [PATCH] [cr135] `ScopedHistogramSampleObserver` using `string_view` This change affects the ctor args and callback arg list. Chromium change: https://chromium.googlesource.com/chromium/src/+/8952859c39099ebf8ca13ae6b1901af7a482249c commit 8952859c39099ebf8ca13ae6b1901af7a482249c Author: Roger McFarlane Date: Thu Feb 20 12:50:27 2025 -0800 Use bounded ranges (string_view) for histogram names. This CL changes the histogram creation APIs to take a durable string view (i.e., a pointer+size pair referring to memory that the programmer has annotated that it will not be freed) instead of just an implicit pointer. The HistogramBase class internally represents the name's length and the histogram's flags using 16 bits in order to not grow the in-memory size of Histogram objects. The name of the histogram is subsequently exposed by HistogramBase as a string_view. This CL updates consumers of the histogram name's to use a string_view of the name. This removes many string length calculations and string copy operations. For histograms allocated in shared memory, it also avoids potential out-of- bounds reads if the underlying string data is modified or corrupted such that it no longer has a trailing NUL char at the end of the string. Lastly, this CL updates a number of call-sites where the histogram names were being copied into short-lived string objects for use as search keys into various containers to perform the functionality without making any unnecessary copies. Low-Coverage-Reason: TRIVIAL_CHANGE Use of string_view instead of string/char* in some error/logging paths have low coverage. AX-Relnotes: n/a Bug: 393394360, 40818143 --- components/misc_metrics/page_metrics.cc | 4 ++-- components/misc_metrics/page_metrics.h | 5 +++-- components/p3a/histograms_braveizer.cc | 15 +++++++-------- components/p3a/histograms_braveizer.h | 2 +- components/p3a/p3a_service.cc | 19 ++++++++----------- components/p3a/p3a_service.h | 4 ++-- 6 files changed, 23 insertions(+), 26 deletions(-) diff --git a/components/misc_metrics/page_metrics.cc b/components/misc_metrics/page_metrics.cc index d5c79cc0271..1f3a89152d3 100644 --- a/components/misc_metrics/page_metrics.cc +++ b/components/misc_metrics/page_metrics.cc @@ -107,7 +107,7 @@ PageMetrics::PageMetrics(PrefService* local_state, PageMetrics::~PageMetrics() = default; -void PageMetrics::OnHttpsNavigationEvent(const char* histogram_name, +void PageMetrics::OnHttpsNavigationEvent(std::string_view histogram_name, uint64_t name_hash, base::HistogramBase::Sample32 sample) { HttpsEvent event = static_cast(sample); @@ -131,7 +131,7 @@ void PageMetrics::OnHttpsNavigationEvent(const char* histogram_name, } void PageMetrics::OnInterstitialDecisionEvent( - const char* histogram_name, + std::string_view histogram_name, uint64_t name_hash, base::HistogramBase::Sample32 sample) { if (sample != security_interstitials::MetricsHelper::Decision::PROCEED) { diff --git a/components/misc_metrics/page_metrics.h b/components/misc_metrics/page_metrics.h index a4e97c3f33c..7a73fa7d44b 100644 --- a/components/misc_metrics/page_metrics.h +++ b/components/misc_metrics/page_metrics.h @@ -7,6 +7,7 @@ #define BRAVE_COMPONENTS_MISC_METRICS_PAGE_METRICS_H_ #include +#include #include #include "base/functional/callback_forward.h" @@ -88,10 +89,10 @@ class PageMetrics { void ReportFirstPageLoadTime(); - void OnHttpsNavigationEvent(const char* histogram_name, + void OnHttpsNavigationEvent(std::string_view histogram_name, uint64_t name_hash, base::HistogramBase::Sample32 sample); - void OnInterstitialDecisionEvent(const char* histogram_name, + void OnInterstitialDecisionEvent(std::string_view histogram_name, uint64_t name_hash, base::HistogramBase::Sample32 sample); diff --git a/components/p3a/histograms_braveizer.cc b/components/p3a/histograms_braveizer.cc index 511231b5dc3..df1fb96840d 100644 --- a/components/p3a/histograms_braveizer.cc +++ b/components/p3a/histograms_braveizer.cc @@ -47,7 +47,7 @@ void HistogramsBraveizer::InitCallbacks() { histogram_sample_callbacks_.push_back( std::make_unique< base::StatisticsRecorder::ScopedHistogramSampleObserver>( - std::string(histogram_name), + histogram_name, base::BindRepeating(&HistogramsBraveizer::DoHistogramBravetization, this))); } @@ -56,11 +56,10 @@ void HistogramsBraveizer::InitCallbacks() { // TODO(iefremov): Replace a bunch of 'if's with something more elegant. // Records the given sample using the proper Brave way. void HistogramsBraveizer::DoHistogramBravetization( - const char* histogram_name, + std::string_view histogram_name, uint64_t name_hash, base::HistogramBase::Sample32 sample) { - DCHECK(histogram_name); - if (strcmp("DefaultBrowser.State", histogram_name) == 0) { + if ("DefaultBrowser.State" == histogram_name) { int answer = 0; switch (sample) { case 0: // Not default. @@ -79,7 +78,7 @@ void HistogramsBraveizer::DoHistogramBravetization( UMA_HISTOGRAM_BOOLEAN("Brave.Core.IsDefault", answer); } - if (strcmp("Extensions.LoadExtension", histogram_name) == 0) { + if ("Extensions.LoadExtension" == histogram_name) { int answer = 0; if (sample == 1) answer = 1; @@ -92,8 +91,8 @@ void HistogramsBraveizer::DoHistogramBravetization( return; } - if (strcmp("Tabs.TabCount", histogram_name) == 0 || - strcmp("Tabs.TabCountPerLoad", histogram_name) == 0) { + if ("Tabs.TabCount" == histogram_name || + "Tabs.TabCountPerLoad" == histogram_name) { int answer = 0; if (0 <= sample && sample <= 1) { answer = 0; @@ -111,7 +110,7 @@ void HistogramsBraveizer::DoHistogramBravetization( return; } - if (strcmp("Tabs.WindowCount", histogram_name) == 0) { + if ("Tabs.WindowCount" == histogram_name) { int answer = 0; if (sample <= 0) { answer = 0; diff --git a/components/p3a/histograms_braveizer.h b/components/p3a/histograms_braveizer.h index a2bd7e02e10..5298dfa2efa 100644 --- a/components/p3a/histograms_braveizer.h +++ b/components/p3a/histograms_braveizer.h @@ -33,7 +33,7 @@ class HistogramsBraveizer // i.e. reemitted using a different name and custom buckets. void InitCallbacks(); - void DoHistogramBravetization(const char* histogram_name, + void DoHistogramBravetization(std::string_view histogram_name, uint64_t name_hash, base::HistogramBase::Sample32 sample); diff --git a/components/p3a/p3a_service.cc b/components/p3a/p3a_service.cc index 4b6d425f693..e2220ed1f4d 100644 --- a/components/p3a/p3a_service.cc +++ b/components/p3a/p3a_service.cc @@ -107,9 +107,8 @@ void P3AService::RegisterPrefs(PrefRegistrySimple* registry, bool first_run) { void P3AService::InitCallback(std::string_view histogram_name) { histogram_sample_callbacks_.push_back( std::make_unique( - std::string(histogram_name), - base::BindRepeating(&P3AService::OnHistogramChanged, - base::Unretained(this)))); + histogram_name, base::BindRepeating(&P3AService::OnHistogramChanged, + base::Unretained(this)))); } void P3AService::InitCallbacks() { @@ -144,7 +143,7 @@ void P3AService::RegisterDynamicMetric(const std::string& histogram_name, dynamic_metric_log_types_[histogram_name] = log_type; dynamic_metric_sample_callbacks_[histogram_name] = std::make_unique( - std::string(histogram_name), + histogram_name, base::BindRepeating(&P3AService::OnHistogramChanged, this)); ScopedDictPrefUpdate update(&*local_state_, kDynamicMetricsDictPref); @@ -245,11 +244,9 @@ void P3AService::OnP3AEnabledChanged() { } } -void P3AService::OnHistogramChanged(const char* histogram_name, +void P3AService::OnHistogramChanged(std::string_view histogram_name, uint64_t name_hash, base::HistogramBase::Sample32 sample) { - DCHECK(histogram_name != nullptr); - std::unique_ptr samples = base::StatisticsRecorder::FindHistogram(histogram_name)->SnapshotDelta(); @@ -277,7 +274,7 @@ void P3AService::OnHistogramChanged(const char* histogram_name, } // Special handling of P2A histograms. - if (std::string_view(histogram_name).starts_with("Brave.P2A")) { + if (histogram_name.starts_with("Brave.P2A")) { // We need the bucket count to make proper perturbation. // All P2A metrics should be implemented as linear histograms. base::SampleVector* vector = @@ -296,7 +293,7 @@ void P3AService::OnHistogramChanged(const char* histogram_name, histogram_name, sample, bucket)); } -void P3AService::OnHistogramChangedOnUI(const char* histogram_name, +void P3AService::OnHistogramChangedOnUI(std::string_view histogram_name, base::HistogramBase::Sample32 sample, size_t bucket) { VLOG(2) << "P3AService::OnHistogramChanged: histogram_name = " @@ -314,7 +311,7 @@ void P3AService::HandleHistogramChange( size_t bucket, std::optional only_update_for_constellation) { if (IsSuspendedMetric(histogram_name, bucket)) { - message_manager_->RemoveMetricValue(std::string(histogram_name), + message_manager_->RemoveMetricValue(histogram_name, only_update_for_constellation); return; } @@ -322,7 +319,7 @@ void P3AService::HandleHistogramChange( if (metric_config && *metric_config && (*metric_config)->constellation_only) { only_update_for_constellation = true; } - message_manager_->UpdateMetricValue(std::string(histogram_name), bucket, + message_manager_->UpdateMetricValue(histogram_name, bucket, only_update_for_constellation); } diff --git a/components/p3a/p3a_service.h b/components/p3a/p3a_service.h index c451f88a5f5..0a7e70ded71 100644 --- a/components/p3a/p3a_service.h +++ b/components/p3a/p3a_service.h @@ -96,7 +96,7 @@ class P3AService : public base::RefCountedThreadSafe, // Invoked by callbacks registered by our service. Since these callbacks // can fire on any thread, this method reposts everything to UI thread. - void OnHistogramChanged(const char* histogram_name, + void OnHistogramChanged(std::string_view histogram_name, uint64_t name_hash, base::HistogramBase::Sample32 sample); @@ -119,7 +119,7 @@ class P3AService : public base::RefCountedThreadSafe, void OnP3AEnabledChanged(); - void OnHistogramChangedOnUI(const char* histogram_name, + void OnHistogramChangedOnUI(std::string_view histogram_name, base::HistogramBase::Sample32 sample, size_t bucket);