[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
This commit is contained in:
@@ -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",
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<network::SharedURLLoaderFactory> testing_url_loader_factory_;
|
||||
|
||||
std::unique_ptr<misc_metrics::GeneralBrowserUsage> 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<brave_origin::BraveOriginPolicyManager,
|
||||
brave_policy::BravePolicyObserver>
|
||||
policy_manager_observation_{this};
|
||||
|
||||
base::WeakPtrFactory<BraveStatsUpdater> weak_ptr_factory_{this};
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user