[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 <rogerm@chromium.org> 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
This commit is contained in:
@@ -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<HttpsEvent>(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) {
|
||||
|
||||
@@ -7,6 +7,7 @@
|
||||
#define BRAVE_COMPONENTS_MISC_METRICS_PAGE_METRICS_H_
|
||||
|
||||
#include <memory>
|
||||
#include <string_view>
|
||||
#include <utility>
|
||||
|
||||
#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);
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -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<base::StatisticsRecorder::ScopedHistogramSampleObserver>(
|
||||
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<base::StatisticsRecorder::ScopedHistogramSampleObserver>(
|
||||
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<base::HistogramSamples> 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<bool> 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);
|
||||
}
|
||||
|
||||
|
||||
@@ -96,7 +96,7 @@ class P3AService : public base::RefCountedThreadSafe<P3AService>,
|
||||
|
||||
// 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<P3AService>,
|
||||
|
||||
void OnP3AEnabledChanged();
|
||||
|
||||
void OnHistogramChangedOnUI(const char* histogram_name,
|
||||
void OnHistogramChangedOnUI(std::string_view histogram_name,
|
||||
base::HistogramBase::Sample32 sample,
|
||||
size_t bucket);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user