From 70d97d993e5cbdb2aba328a45ed3e8aaf31c899f Mon Sep 17 00:00:00 2001 From: Serg Date: Wed, 20 May 2026 07:22:43 -0400 Subject: [PATCH] [Origin] Defer policy init signal until BraveProfilePolicyProvider refreshes (#36495) [Origin] Gate BraveProfilePolicyProvider on BraveOriginPolicyManager init `BraveProvider`'s first `RefreshPolicies` is triggered by `AdBlockOnlyModePolicyManager` (initialised at process start), well before `BraveOriginPolicyManager` (lazily initialised by `BraveOriginServiceFactory`) is ready. The resulting bundle lacks `BraveRewardsDisabled`, but the default `IsInitializationComplete` lets `PolicyService` report ready anyway. `AdsServiceImpl::MaybeStartBatAdsService` then evaluates a stale managed pref store and bat-ads starts despite the policy. Override `IsInitializationComplete` to additionally gate on `BraveOriginPolicyManager::IsInitialized()`. Upstream unit tests (`ProfilePolicyConnectorTest`) bypass our factory bootstrap, so add a `SetExpectedToBeInitialized` flag set from `BraveOriginServiceFactory`'s ctor; the override short-circuits when unset (desktop/Android only -- iOS doesn't run those tests). Resolves: https://github.com/brave/brave-browser/issues/54962 --- .../brave_origin_service_factory.cc | 23 +++++++++ .../brave_origin_policy_manager.cc | 9 ++++ .../brave_origin_policy_manager.h | 8 ++++ components/brave_policy/BUILD.gn | 1 + .../brave_profile_policy_provider.cc | 35 +++++++++++++- .../brave_profile_policy_provider.h | 1 + .../brave_profile_policy_provider_unittest.cc | 48 +++++++++++++++++-- .../policy_initialization_waiter.h | 1 + 8 files changed, 121 insertions(+), 5 deletions(-) diff --git a/browser/brave_origin/brave_origin_service_factory.cc b/browser/brave_origin/brave_origin_service_factory.cc index 90f7496e582..0394b166161 100644 --- a/browser/brave_origin/brave_origin_service_factory.cc +++ b/browser/brave_origin/brave_origin_service_factory.cc @@ -233,6 +233,29 @@ BraveOriginServiceFactory::BraveOriginServiceFactory() "BraveOriginService", ProfileSelections::BuildRedirectedInIncognito()) { DependsOn(skus::SkusServiceFactory::GetInstance()); + // This factory owns the lazy initialization of `BraveOriginPolicyManager` + // (in `BuildServiceInstanceForBrowserContext`). Mark the manager as + // expected-to-be-initialized as soon as the factory is registered so that + // `BraveProfilePolicyProvider::IsInitializationComplete` knows to gate on + // the manager actually finishing initialization. Without this gate + // `BraveProvider`'s first `RefreshPolicies` (triggered by + // `AdBlockOnlyModePolicyManager`'s `OnBravePoliciesReady`, which fires at + // process start) would flip `first_policies_loaded_` to true with an + // empty Brave Origin bundle, and consumers of + // `PolicyService::OnPolicyServiceInitialized` (notably + // `AdsServiceImpl::MaybeStartBatAdsService`) would evaluate against a + // managed pref store that does not yet reflect `BraveRewardsDisabled`. + // + // Upstream unit tests build `ProfilePolicyConnector` directly without + // going through `EnsureBrowserContextKeyedServiceFactoriesBuilt()`, so this + // factory's `GetInstance()` is never called in those tests, the flag stays + // false, and `BraveProvider::IsInitializationComplete` short-circuits to + // true to avoid blocking the upstream tests forever. The affected tests + // are in `chrome/browser/policy/profile_policy_connector_unittest.cc`, + // primarily the `ProfilePolicyConnectorTest.AffiliationMetrics_*`, + // `LocalTestProviderUseAndRevert`, and `ChromeosPrimaryUserPoliciesProxied` + // cases that use `PolicyServiceInitializedWaiter`. + BraveOriginPolicyManager::GetInstance()->SetExpectedToBeInitialized(); } BraveOriginServiceFactory::~BraveOriginServiceFactory() = default; diff --git a/components/brave_origin/brave_origin_policy_manager.cc b/components/brave_origin/brave_origin_policy_manager.cc index 5cda8ec41e1..999e5441d3a 100644 --- a/components/brave_origin/brave_origin_policy_manager.cc +++ b/components/brave_origin/brave_origin_policy_manager.cc @@ -167,6 +167,14 @@ bool BraveOriginPolicyManager::IsInitialized() const { return initialized_; } +void BraveOriginPolicyManager::SetExpectedToBeInitialized() { + expected_to_be_initialized_ = true; +} + +bool BraveOriginPolicyManager::IsExpectedToBeInitialized() const { + return expected_to_be_initialized_; +} + void BraveOriginPolicyManager::SetPurchased(bool purchased) { if (is_purchased_ == purchased) { return; @@ -196,6 +204,7 @@ bool BraveOriginPolicyManager::IsPurchased() const { void BraveOriginPolicyManager::Shutdown() { initialized_ = false; + expected_to_be_initialized_ = false; is_purchased_ = false; browser_policy_definitions_.clear(); profile_policy_definitions_.clear(); diff --git a/components/brave_origin/brave_origin_policy_manager.h b/components/brave_origin/brave_origin_policy_manager.h index 390741877af..01a1d993dbc 100644 --- a/components/brave_origin/brave_origin_policy_manager.h +++ b/components/brave_origin/brave_origin_policy_manager.h @@ -69,6 +69,13 @@ class BraveOriginPolicyManager { // Check if the singleton has been initialized bool IsInitialized() const; + // Declares that `Init()` is expected to be called later in this process. + // Consumers gated on this manager use it to distinguish production startup + // (where lazy `Init()` will eventually run, so they should wait) from test + // contexts that never bootstrap us (where there is nothing to wait for). + void SetExpectedToBeInitialized(); + bool IsExpectedToBeInitialized() const; + // Set/get the purchase state. When the purchase state changes, // observers are notified to refresh policies. void SetPurchased(bool purchased); @@ -97,6 +104,7 @@ class BraveOriginPolicyManager { std::optional profile_id) const; bool initialized_ = false; + bool expected_to_be_initialized_ = false; bool is_purchased_ = false; BraveOriginPolicyMap browser_policy_definitions_; BraveOriginPolicyMap profile_policy_definitions_; diff --git a/components/brave_policy/BUILD.gn b/components/brave_policy/BUILD.gn index 79a346ec90e..219bc062eb5 100644 --- a/components/brave_policy/BUILD.gn +++ b/components/brave_policy/BUILD.gn @@ -56,6 +56,7 @@ source_set("unit_tests") { ":policy_initialization_waiter", "//base", "//base/test:test_support", + "//brave/components/brave_origin", "//brave/components/resources:strings", "//components/enterprise", "//components/policy/core/browser", diff --git a/components/brave_policy/brave_profile_policy_provider.cc b/components/brave_policy/brave_profile_policy_provider.cc index 4a869af2ece..63391bccd84 100644 --- a/components/brave_policy/brave_profile_policy_provider.cc +++ b/components/brave_policy/brave_profile_policy_provider.cc @@ -7,10 +7,10 @@ #include -#include "base/logging.h" #include "base/values.h" #include "brave/components/brave_origin/brave_origin_utils.h" #include "brave/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h" +#include "build/build_config.h" #include "components/policy/core/common/policy_bundle.h" #include "components/policy/core/common/policy_map.h" #include "components/policy/core/common/policy_namespace.h" @@ -50,6 +50,39 @@ void BraveProfilePolicyProvider::RefreshPolicies( UpdatePolicy(std::move(bundle)); } +bool BraveProfilePolicyProvider::IsInitializationComplete( + policy::PolicyDomain domain) const { + auto* manager = brave_origin::BraveOriginPolicyManager::GetInstance(); +#if !BUILDFLAG(IS_IOS) + // Desktop/Android only: short-circuit when our factory has not been + // registered in this process. This is the case for upstream unit tests + // (e.g. `ProfilePolicyConnectorTest`) that build `ProfilePolicyConnector` + // directly without going through Brave's keyed-service-factory startup -- + // those tests have no Brave Origin source to wait for, so we report + // ready and let the upstream `OnPolicyServiceInitialized` signal fire + // normally. + // + // iOS does not run these upstream unit tests, and its own keyed-service + // startup always constructs `BraveOriginServiceFactory` (which lazily + // initializes `BraveOriginPolicyManager`). The gate below works there + // without the flag check. + if (!manager->IsExpectedToBeInitialized()) { + return true; + } +#endif // !BUILDFLAG(IS_IOS) + // We observe both `BraveOriginPolicyManager` and + // `AdBlockOnlyModePolicyManager` but they initialise on different schedules: + // `AdBlockOnlyModePolicyManager` is initialised synchronously at process + // start while `BraveOriginPolicyManager` is initialised lazily during + // profile keyed-service build. The first `RefreshPolicies` triggered by + // AdBlockOnlyMode's `OnBravePoliciesReady` produces a bundle without Brave + // Origin policies and would otherwise flip `first_policies_loaded_` to true + // prematurely. Gate on Brave Origin's `IsInitialized()` so consumers don't + // observe an empty managed pref store before Brave Origin policies (e.g. + // `BraveRewardsDisabled`) have been merged. + return first_policies_loaded_ && manager->IsInitialized(); +} + bool BraveProfilePolicyProvider::IsFirstPolicyLoadComplete( policy::PolicyDomain domain) const { return first_policies_loaded_; diff --git a/components/brave_policy/brave_profile_policy_provider.h b/components/brave_policy/brave_profile_policy_provider.h index 0465eb65aff..74a2109647f 100644 --- a/components/brave_policy/brave_profile_policy_provider.h +++ b/components/brave_policy/brave_profile_policy_provider.h @@ -34,6 +34,7 @@ class BraveProfilePolicyProvider : public policy::ConfigurationPolicyProvider, // ConfigurationPolicyProvider implementation. void Init(policy::SchemaRegistry* registry) override; void RefreshPolicies(policy::PolicyFetchReason reason) override; + bool IsInitializationComplete(policy::PolicyDomain domain) const override; bool IsFirstPolicyLoadComplete(policy::PolicyDomain domain) const override; // BravePolicyObserver implementation. diff --git a/components/brave_policy/brave_profile_policy_provider_unittest.cc b/components/brave_policy/brave_profile_policy_provider_unittest.cc index 86fce3176ae..8c75b224d93 100644 --- a/components/brave_policy/brave_profile_policy_provider_unittest.cc +++ b/components/brave_policy/brave_profile_policy_provider_unittest.cc @@ -8,6 +8,8 @@ #include #include "base/test/task_environment.h" +#include "brave/components/brave_origin/brave_origin_policy_manager.h" +#include "brave/components/brave_origin/brave_origin_prefs.h" #include "components/policy/core/common/policy_bundle.h" #include "components/policy/core/common/policy_namespace.h" #include "components/policy/core/common/policy_types.h" @@ -19,18 +21,23 @@ namespace brave_policy { class BraveProfilePolicyProviderTest : public ::testing::Test { public: - BraveProfilePolicyProviderTest() = default; + BraveProfilePolicyProviderTest() { + brave_origin::RegisterLocalStatePrefs(local_state_.registry()); + } ~BraveProfilePolicyProviderTest() override = default; void TearDown() override { - if (provider_.IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) { - provider_.Shutdown(); - } + provider_.Shutdown(); + // Reset the process-wide singleton so its state does not leak between + // tests. `BraveOriginPolicyManager::Shutdown()` is safe to call even if + // `Init()` was never called. + brave_origin::BraveOriginPolicyManager::GetInstance()->Shutdown(); } protected: base::test::TaskEnvironment task_environment_; policy::SchemaRegistry schema_registry_; + TestingPrefServiceSimple local_state_; BraveProfilePolicyProvider provider_; }; @@ -55,6 +62,39 @@ TEST_F(BraveProfilePolicyProviderTest, InitAndPolicyLoadComplete) { provider_.IsFirstPolicyLoadComplete(policy::POLICY_DOMAIN_CHROME)); } +TEST_F(BraveProfilePolicyProviderTest, IsInitializationCompleteGated) { + auto* manager = brave_origin::BraveOriginPolicyManager::GetInstance(); + // On desktop/Android the override short-circuits to true when the manager + // has not been declared as expected-to-initialize (see the `#if !IS_IOS` + // block in `IsInitializationComplete`). Set the flag so the gating path + // below is exercised on all platforms. + manager->SetExpectedToBeInitialized(); + + // Before the manager is initialized, the override gates initialisation on + // `IsInitialized()` so consumers do not observe an empty managed pref + // store before Brave Origin policies have been merged. + EXPECT_FALSE(provider_.IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) + << "Before provider Init: manager not initialised, no refresh"; + + provider_.Init(&schema_registry_); + EXPECT_FALSE(provider_.IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) + << "After provider Init: manager still not initialised"; + + // Initializing the manager flips `IsInitialized()` and synchronously + // notifies our observer, but `RefreshPolicies` is gated on a non-empty + // `profile_id_`, so `first_policies_loaded_` is still false. + manager->Init(/*browser_policy_definitions=*/{}, + /*profile_policy_definitions=*/{}, &local_state_); + EXPECT_FALSE(provider_.IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) + << "After manager Init: profile_id_ still empty, refresh has not run"; + + // Setting the profile ID drives the first refresh. + provider_.SetProfileID("test-profile-id"); + EXPECT_TRUE(provider_.IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) + << "After SetProfileID: first refresh has run and manager is " + "initialised"; +} + TEST_F(BraveProfilePolicyProviderTest, EmptyPolicyBundle) { // Initialize the provider provider_.Init(&schema_registry_); diff --git a/components/brave_policy/policy_initialization_waiter.h b/components/brave_policy/policy_initialization_waiter.h index 18ce0ae80f1..0c2368c7a0e 100644 --- a/components/brave_policy/policy_initialization_waiter.h +++ b/components/brave_policy/policy_initialization_waiter.h @@ -8,6 +8,7 @@ #include "base/functional/callback.h" #include "base/memory/raw_ptr.h" +#include "components/policy/core/common/policy_namespace.h" #include "components/policy/core/common/policy_service.h" namespace brave_policy {