From c1241a08a76b76e405802e178a15a1f29a629052 Mon Sep 17 00:00:00 2001 From: Serg Date: Mon, 25 May 2026 17:29:24 -0400 Subject: [PATCH] [Origin] Brave Stats startup ping races BraveStatsPingEnabled policy (#36698) `BraveStatsUpdater` is a browser-process singleton; its 3-second startup timer reads `kStatsReportingEnabled` without synchronising against `BraveOriginPolicyManager`. Make the updater observe the manager directly and defer the timer behind a two-phase gate (referrals barrier + `OnBravePoliciesReady`), so the startup ping cannot fire before the policy bundle is merged into local state. Resolves: https://github.com/brave/brave-browser/issues/55811 --- browser/brave_stats/BUILD.gn | 2 ++ browser/brave_stats/brave_stats_updater.cc | 30 ++++++++++++++++++++++ browser/brave_stats/brave_stats_updater.h | 23 ++++++++++++++++- 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/browser/brave_stats/BUILD.gn b/browser/brave_stats/BUILD.gn index 55188569ede..0b81b4bbdf5 100644 --- a/browser/brave_stats/BUILD.gn +++ b/browser/brave_stats/BUILD.gn @@ -46,6 +46,8 @@ source_set("brave_stats") { "//brave/browser/serp_metrics", "//brave/common", "//brave/components/brave_ads/buildflags", + "//brave/components/brave_origin", + "//brave/components/brave_policy:brave_policy_observer", "//brave/components/brave_referrals/common", "//brave/components/brave_wallet/common/buildflags", "//brave/components/constants", diff --git a/browser/brave_stats/brave_stats_updater.cc b/browser/brave_stats/brave_stats_updater.cc index 039344ad477..9073c9ddd2f 100644 --- a/browser/brave_stats/brave_stats_updater.cc +++ b/browser/brave_stats/brave_stats_updater.cc @@ -152,6 +152,14 @@ BraveStatsUpdater::BraveStatsUpdater(PrefService* pref_service, : pref_service_(pref_service), profile_manager_(profile_manager), testing_url_loader_factory_(nullptr) { + // Observe `BraveOriginPolicyManager` so we can defer the boot ping until + // its policies have been merged into local state. `AddObserver` fires + // `OnBravePoliciesReady` immediately if the manager is already initialised, + // so construction order between this service and the manager doesn't + // matter. + policy_manager_observation_.Observe( + brave_origin::BraveOriginPolicyManager::GetInstance()); + const base::CommandLine& command_line = *base::CommandLine::ForCurrentProcess(); if (command_line.HasSwitch(switches::kBraveStatsUpdaterServer)) { @@ -337,6 +345,28 @@ void BraveStatsUpdater::OnReferralInitialization() { void BraveStatsUpdater::StartServerPingStartupTimer() { stats_preconditions_barrier_.Reset(); stats_startup_complete_ = true; + MaybeStartStartupTimer(); +} + +void BraveStatsUpdater::OnBravePoliciesReady() { + // `AddObserver` may fire this synchronously (if the manager was already + // initialised when we attached), and `BraveOriginPolicyManager` may notify + // again later. The startup-timer path must run at most once. + if (brave_policies_ready_) { + return; + } + brave_policies_ready_ = true; + MaybeStartStartupTimer(); +} + +void BraveStatsUpdater::MaybeStartStartupTimer() { + // Both gates must have fired: referrals barrier resolved + // (`stats_startup_complete_`) and Brave Origin policies merged into local + // state (`brave_policies_ready_`). Whichever finishes last actually starts + // the timer. + if (!stats_startup_complete_ || !brave_policies_ready_) { + return; + } server_ping_startup_timer_->Start( FROM_HERE, base::Seconds(kUpdateServerStartupPingDelaySeconds), this, &BraveStatsUpdater::OnServerPingTimerFired); diff --git a/browser/brave_stats/brave_stats_updater.h b/browser/brave_stats/brave_stats_updater.h index be9460712ec..383c4c9414e 100644 --- a/browser/brave_stats/brave_stats_updater.h +++ b/browser/brave_stats/brave_stats_updater.h @@ -15,6 +15,8 @@ #include "base/memory/weak_ptr.h" #include "base/scoped_observation.h" #include "brave/browser/brave_stats/buildflags.h" +#include "brave/components/brave_origin/brave_origin_policy_manager.h" +#include "brave/components/brave_policy/brave_policy_observer.h" #include "chrome/browser/profiles/profile_manager_observer.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "url/gurl.h" @@ -55,7 +57,8 @@ inline constexpr char kP3ADailyPingHistogramName[] = "Brave.Core.UsageDaily"; class BraveStatsUpdaterParams; -class BraveStatsUpdater : public ProfileManagerObserver { +class BraveStatsUpdater : public ProfileManagerObserver, + public brave_policy::BravePolicyObserver { public: BraveStatsUpdater(PrefService* pref_service, ProfileManager* profile_manager); BraveStatsUpdater(const BraveStatsUpdater&) = delete; @@ -90,6 +93,11 @@ class BraveStatsUpdater : public ProfileManagerObserver { void OnReferralInitialization(); void StartServerPingStartupTimer(); + // Fires only when both `stats_startup_complete_` (referrals barrier + // resolved) and `brave_policies_ready_` (Brave Origin policies merged) are + // true. Either path is allowed to be first; the second one's call actually + // starts the timer. + void MaybeStartStartupTimer(); void QueueServerPing(); void SendServerPing(); @@ -98,6 +106,9 @@ class BraveStatsUpdater : public ProfileManagerObserver { // ProfileManagerObserver: void OnProfileAdded(Profile* profile) override; + // brave_policy::BravePolicyObserver: + void OnBravePoliciesReady() override; + network::mojom::URLLoaderFactory* GetURLLoaderFactory(); friend class ::BraveStatsUpdaterBrowserTest; @@ -115,6 +126,16 @@ class BraveStatsUpdater : public ProfileManagerObserver { scoped_refptr testing_url_loader_factory_; std::unique_ptr general_browser_usage_p3a_; + + // True once `OnBravePoliciesReady` has been observed. `AddObserver` fires + // it synchronously when the manager is already initialised, so this can be + // true even before the ctor finishes. Subsequent firings are idempotent. + bool brave_policies_ready_ = false; + + base::ScopedObservation + policy_manager_observation_{this}; + base::WeakPtrFactory weak_ptr_factory_{this}; };