From a044cc5bdebf2f40efaa41539af9ef81ca0feb28 Mon Sep 17 00:00:00 2001 From: Serg Date: Wed, 6 May 2026 11:23:42 -0400 Subject: [PATCH] [Origin] Wait for all policy managers before refreshing profile policies (#36193) * [Origin] Wait for all policy managers before refreshing profile policies `BraveProfilePolicyProvider` observes both `BraveOriginPolicyManager` and `AdBlockOnlyModePolicyManager`. Each fires `OnBravePoliciesReady` independently, so the first notification could call `RefreshPolicies` before the other manager's `Init()` had run -- producing a bundle that reflected only one source and flipping `IsFirstPolicyLoadComplete` true on the empty load. Introduce `BravePolicyManagerBase` with self-registration and a static `AllInitialized()`. `OnBravePoliciesReady` early-returns until every registered manager reports ready; `policies_ready_` is gated on the same check so `SetProfileID` can't trigger a partial refresh either. Future managers inheriting from the base are picked up automatically. Resolves: https://github.com/brave/brave-browser/issues/55276 --- components/brave_origin/BUILD.gn | 1 + .../brave_origin_policy_manager.cc | 6 +- .../brave_origin_policy_manager.h | 6 ++ components/brave_policy/BUILD.gn | 15 ++++ .../brave_policy/ad_block_only_mode/BUILD.gn | 1 + .../ad_block_only_mode_policy_manager.cc | 10 ++- .../ad_block_only_mode_policy_manager.h | 8 ++ .../brave_policy_manager_registry.cc | 57 +++++++++++++ .../brave_policy_manager_registry.h | 65 +++++++++++++++ .../brave_policy_manager_registry_unittest.cc | 82 +++++++++++++++++++ .../brave_profile_policy_provider.cc | 15 ++++ .../brave_profile_policy_provider_unittest.cc | 28 +++++++ 12 files changed, 292 insertions(+), 2 deletions(-) create mode 100644 components/brave_policy/brave_policy_manager_registry.cc create mode 100644 components/brave_policy/brave_policy_manager_registry.h create mode 100644 components/brave_policy/brave_policy_manager_registry_unittest.cc diff --git a/components/brave_origin/BUILD.gn b/components/brave_origin/BUILD.gn index d68163bb6e0..9e76bb61871 100644 --- a/components/brave_origin/BUILD.gn +++ b/components/brave_origin/BUILD.gn @@ -68,6 +68,7 @@ static_library("brave_origin") { ] deps = [ "//brave/brave_domains", + "//brave/components/brave_policy:brave_policy_manager_registry", "//brave/components/brave_policy:brave_policy_observer", "//brave/components/skus/browser", "//brave/components/skus/common", diff --git a/components/brave_origin/brave_origin_policy_manager.cc b/components/brave_origin/brave_origin_policy_manager.cc index 5cda8ec41e1..0d7ad6271a2 100644 --- a/components/brave_origin/brave_origin_policy_manager.cc +++ b/components/brave_origin/brave_origin_policy_manager.cc @@ -6,6 +6,7 @@ #include "brave/components/brave_origin/brave_origin_policy_manager.h" #include "base/containers/map_util.h" +#include "base/functional/bind.h" #include "base/logging.h" #include "base/no_destructor.h" #include "brave/components/brave_origin/brave_origin_utils.h" @@ -218,7 +219,10 @@ bool BraveOriginPolicyManager::GetPolicyValueInternal( return default_value; } -BraveOriginPolicyManager::BraveOriginPolicyManager() = default; +BraveOriginPolicyManager::BraveOriginPolicyManager() + : registration_( + base::BindRepeating(&BraveOriginPolicyManager::IsInitialized, + base::Unretained(this))) {} BraveOriginPolicyManager::~BraveOriginPolicyManager() = default; diff --git a/components/brave_origin/brave_origin_policy_manager.h b/components/brave_origin/brave_origin_policy_manager.h index 390741877af..b1fd5df39fe 100644 --- a/components/brave_origin/brave_origin_policy_manager.h +++ b/components/brave_origin/brave_origin_policy_manager.h @@ -12,6 +12,7 @@ #include "base/memory/raw_ptr.h" #include "base/observer_list.h" #include "brave/components/brave_origin/brave_origin_policy_info.h" +#include "brave/components/brave_policy/brave_policy_manager_registry.h" #include "brave/components/brave_policy/brave_policy_observer.h" class PrefService; @@ -102,6 +103,11 @@ class BraveOriginPolicyManager { BraveOriginPolicyMap profile_policy_definitions_; raw_ptr local_state_ = nullptr; base::ObserverList observers_; + + // Participates in `BravePolicyManagerRegistry::AllInitialized()`. Declared + // last so the readiness callback isn't invoked against partially + // constructed state (`initialized_` above must already exist). + brave_policy::BravePolicyManagerRegistration registration_; }; } // namespace brave_origin diff --git a/components/brave_policy/BUILD.gn b/components/brave_policy/BUILD.gn index 4ce183e0f1b..b1bbd56ad36 100644 --- a/components/brave_policy/BUILD.gn +++ b/components/brave_policy/BUILD.gn @@ -13,6 +13,7 @@ static_library("brave_policy") { ] deps = [ + ":brave_policy_manager_registry", ":brave_policy_observer", "//base", "//brave/components/brave_origin", @@ -23,6 +24,15 @@ static_library("brave_policy") { ] } +source_set("brave_policy_manager_registry") { + sources = [ + "brave_policy_manager_registry.cc", + "brave_policy_manager_registry.h", + ] + + public_deps = [ "//base" ] +} + source_set("brave_policy_observer") { sources = [ "brave_policy_observer.h" ] @@ -34,14 +44,19 @@ source_set("unit_tests") { sources = [ "brave_browser_policy_provider_unittest.cc", "brave_policy_conversions_unittest.cc", + "brave_policy_manager_registry_unittest.cc", "brave_policy_source_unittest.cc", "brave_profile_policy_provider_unittest.cc", ] deps = [ ":brave_policy", + ":brave_policy_manager_registry", "//base", "//base/test:test_support", + "//brave/components/brave_origin", + "//brave/components/brave_policy/ad_block_only_mode", + "//brave/components/brave_shields/core/common", "//brave/components/resources:strings", "//components/enterprise", "//components/policy/core/browser", diff --git a/components/brave_policy/ad_block_only_mode/BUILD.gn b/components/brave_policy/ad_block_only_mode/BUILD.gn index 9cae45f489c..317652d4d8c 100644 --- a/components/brave_policy/ad_block_only_mode/BUILD.gn +++ b/components/brave_policy/ad_block_only_mode/BUILD.gn @@ -17,6 +17,7 @@ static_library("ad_block_only_mode") { deps = [ "//base", + "//brave/components/brave_policy:brave_policy_manager_registry", "//brave/components/brave_policy:brave_policy_observer", "//brave/components/brave_shields/core/common", "//components/content_settings/core/common", diff --git a/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.cc b/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.cc index b634a5ef847..5f34429ce3e 100644 --- a/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.cc +++ b/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.cc @@ -6,6 +6,7 @@ #include "brave/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h" #include "base/feature_list.h" +#include "base/functional/bind.h" #include "base/no_destructor.h" #include "brave/components/brave_policy/ad_block_only_mode/buildflags/buildflags.h" #include "brave/components/brave_shields/core/common/features.h" @@ -43,6 +44,10 @@ void AdBlockOnlyModePolicyManager::Shutdown() { local_state_ = nullptr; } +bool AdBlockOnlyModePolicyManager::IsInitialized() const { + return local_state_ != nullptr; +} + void AdBlockOnlyModePolicyManager::AddObserver(BravePolicyObserver* observer) { observers_.AddObserver(observer); @@ -71,7 +76,10 @@ AdBlockOnlyModePolicies AdBlockOnlyModePolicyManager::GetPolicies() const { return GetPoliciesImpl(); } -AdBlockOnlyModePolicyManager::AdBlockOnlyModePolicyManager() = default; +AdBlockOnlyModePolicyManager::AdBlockOnlyModePolicyManager() + : registration_( + base::BindRepeating(&AdBlockOnlyModePolicyManager::IsInitialized, + base::Unretained(this))) {} AdBlockOnlyModePolicyManager::~AdBlockOnlyModePolicyManager() = default; diff --git a/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h b/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h index 7e0752675f8..377e7f56bef 100644 --- a/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h +++ b/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h @@ -10,6 +10,7 @@ #include "base/memory/raw_ptr.h" #include "base/observer_list.h" #include "base/values.h" +#include "brave/components/brave_policy/brave_policy_manager_registry.h" #include "brave/components/brave_policy/brave_policy_observer.h" #include "components/prefs/pref_change_registrar.h" @@ -36,6 +37,8 @@ class AdBlockOnlyModePolicyManager final { void Init(PrefService* local_state); void Shutdown(); + bool IsInitialized() const; + void AddObserver(BravePolicyObserver* observer); void RemoveObserver(BravePolicyObserver* observer); @@ -56,6 +59,11 @@ class AdBlockOnlyModePolicyManager final { raw_ptr local_state_; // Not owned. PrefChangeRegistrar pref_change_registrar_; base::ObserverList observers_; + + // Participates in `BravePolicyManagerRegistry::AllInitialized()`. Declared + // last so the readiness callback isn't invoked against partially + // constructed state (`local_state_` above must already exist). + BravePolicyManagerRegistration registration_; }; } // namespace brave_policy diff --git a/components/brave_policy/brave_policy_manager_registry.cc b/components/brave_policy/brave_policy_manager_registry.cc new file mode 100644 index 00000000000..a477930e48e --- /dev/null +++ b/components/brave_policy/brave_policy_manager_registry.cc @@ -0,0 +1,57 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/brave_policy/brave_policy_manager_registry.h" + +#include +#include +#include + +#include "base/memory/raw_ptr.h" +#include "base/no_destructor.h" + +namespace brave_policy { + +namespace { + +// Function-local static to avoid static-init-order issues with policy +// managers that may register during static init via `base::NoDestructor` +// singletons. +std::vector>& Registry() { + static base::NoDestructor< + std::vector>> + registry; + return *registry; +} + +} // namespace + +// static +bool BravePolicyManagerRegistry::AllInitialized() { + return std::ranges::all_of(Registry(), [](const auto& registration) { + return registration->IsInitialized(); + }); +} + +// static +size_t BravePolicyManagerRegistry::SizeForTesting() { + return Registry().size(); +} + +BravePolicyManagerRegistration::BravePolicyManagerRegistration( + IsInitializedCallback is_initialized) + : is_initialized_(std::move(is_initialized)) { + Registry().push_back(this); +} + +BravePolicyManagerRegistration::~BravePolicyManagerRegistration() { + std::erase(Registry(), this); +} + +bool BravePolicyManagerRegistration::IsInitialized() const { + return is_initialized_.Run(); +} + +} // namespace brave_policy diff --git a/components/brave_policy/brave_policy_manager_registry.h b/components/brave_policy/brave_policy_manager_registry.h new file mode 100644 index 00000000000..c8fb10e5026 --- /dev/null +++ b/components/brave_policy/brave_policy_manager_registry.h @@ -0,0 +1,65 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_BRAVE_POLICY_BRAVE_POLICY_MANAGER_REGISTRY_H_ +#define BRAVE_COMPONENTS_BRAVE_POLICY_BRAVE_POLICY_MANAGER_REGISTRY_H_ + +#include + +#include "base/functional/callback.h" + +namespace brave_policy { + +// Process-wide registry of Brave-side policy managers. Used by callers +// (e.g. `BraveProfilePolicyProvider`) that need to wait for every +// participating manager to report initialized before refreshing policies. +// +// Managers participate by holding a `BravePolicyManagerRegistration` +// member that captures their `IsInitialized()` predicate. Registration +// happens automatically in the registration's constructor and +// unregistration in its destructor. +// +// Single-threaded by design: registration happens during early +// browser-process startup on the UI thread, before worker threads access +// these managers. +class BravePolicyManagerRegistry { + public: + // Returns true once every registered manager's `IsInitialized()` + // callback returns true. + static bool AllInitialized(); + + // Returns the number of registered managers. For tests only. + static size_t SizeForTesting(); +}; + +// Registers itself with `BravePolicyManagerRegistry` on construction and +// unregisters on destruction. Construct as a member of any policy-manager +// class to participate in `AllInitialized()`. +// +// Member ordering matters: declare this *after* anything the +// `IsInitialized()` callback reads from, so the predicate isn't invoked +// against a partially constructed object during a future enclosing-class +// refactor. +class BravePolicyManagerRegistration { + public: + using IsInitializedCallback = base::RepeatingCallback; + + explicit BravePolicyManagerRegistration(IsInitializedCallback is_initialized); + ~BravePolicyManagerRegistration(); + + BravePolicyManagerRegistration(const BravePolicyManagerRegistration&) = + delete; + BravePolicyManagerRegistration& operator=( + const BravePolicyManagerRegistration&) = delete; + + bool IsInitialized() const; + + private: + const IsInitializedCallback is_initialized_; +}; + +} // namespace brave_policy + +#endif // BRAVE_COMPONENTS_BRAVE_POLICY_BRAVE_POLICY_MANAGER_REGISTRY_H_ diff --git a/components/brave_policy/brave_policy_manager_registry_unittest.cc b/components/brave_policy/brave_policy_manager_registry_unittest.cc new file mode 100644 index 00000000000..1ef42f93206 --- /dev/null +++ b/components/brave_policy/brave_policy_manager_registry_unittest.cc @@ -0,0 +1,82 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/brave_policy/brave_policy_manager_registry.h" + +#include "base/functional/bind.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace brave_policy { + +namespace { + +// Minimal policy-manager-like host used to exercise the registry without +// depending on real singletons. Owns a registration whose callback reports +// the host's initialized flag. +class FakePolicyManager { + public: + FakePolicyManager() + : initialized_(false), + registration_(base::BindRepeating(&FakePolicyManager::IsInitialized, + base::Unretained(this))) {} + + bool IsInitialized() const { return initialized_; } + void SetInitialized(bool initialized) { initialized_ = initialized; } + + private: + // `initialized_` must be declared before `registration_` so it is + // initialized first; the registration captures a callback that reads + // `initialized_`. + bool initialized_; + BravePolicyManagerRegistration registration_; +}; + +} // namespace + +// Construction adds an entry to the registry; destruction removes it. +TEST(BravePolicyManagerRegistryTest, + ConstructionRegistersAndDestructionUnregisters) { + const size_t baseline = BravePolicyManagerRegistry::SizeForTesting(); + { + FakePolicyManager fake; + EXPECT_EQ(baseline + 1, BravePolicyManagerRegistry::SizeForTesting()); + } + EXPECT_EQ(baseline, BravePolicyManagerRegistry::SizeForTesting()); +} + +// `AllInitialized` returns false as long as any registered manager +// reports not initialized. +TEST(BravePolicyManagerRegistryTest, + AllInitializedFalseWithUninitializedManager) { + FakePolicyManager fake; + EXPECT_FALSE(BravePolicyManagerRegistry::AllInitialized()); +} + +// Even with an initialized manager present, an uninitialized one +// elsewhere forces `AllInitialized()` false. +TEST(BravePolicyManagerRegistryTest, AllInitializedFalseWhenAnyUninitialized) { + FakePolicyManager fake_initialized; + fake_initialized.SetInitialized(true); + + FakePolicyManager fake_uninitialized; + EXPECT_FALSE(BravePolicyManagerRegistry::AllInitialized()); +} + +// After an uninitialized manager is destroyed, it no longer holds +// `AllInitialized()` false: the result reverts to the baseline observed +// before it existed. +TEST(BravePolicyManagerRegistryTest, + DestroyedManagerNoLongerAffectsAllInitialized) { + FakePolicyManager fake_outer; + fake_outer.SetInitialized(true); + const bool baseline = BravePolicyManagerRegistry::AllInitialized(); + { + FakePolicyManager fake_inner; // uninitialized + EXPECT_FALSE(BravePolicyManagerRegistry::AllInitialized()); + } + EXPECT_EQ(baseline, BravePolicyManagerRegistry::AllInitialized()); +} + +} // namespace brave_policy diff --git a/components/brave_policy/brave_profile_policy_provider.cc b/components/brave_policy/brave_profile_policy_provider.cc index 25e4d255d8b..125afebb752 100644 --- a/components/brave_policy/brave_profile_policy_provider.cc +++ b/components/brave_policy/brave_profile_policy_provider.cc @@ -11,6 +11,7 @@ #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 "brave/components/brave_policy/brave_policy_manager_registry.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" @@ -85,6 +86,20 @@ bool BraveProfilePolicyProvider::IsFirstPolicyLoadComplete( } void BraveProfilePolicyProvider::OnBravePoliciesReady() { + // Wait until every registered policy manager reports `IsInitialized()` + // before treating policies as ready. Each manager fires this callback + // independently from its own `AddObserver`; acting on the first one + // would push a partial bundle (e.g. AdBlockOnlyMode policies present + // but BraveOrigin policies missing because `IsBraveOriginPurchased()` + // short-circuits to false on an uninitialized manager), which then + // propagates through `PolicyServiceImpl` and flips + // `IsFirstPolicyLoadComplete` on the empty load. Likewise, + // `policies_ready_` is gated on full readiness so a `SetProfileID` + // call in this window doesn't trigger a partial refresh. + if (!BravePolicyManagerRegistry::AllInitialized()) { + return; + } + policies_ready_ = true; // Once we have Brave policies and a profile ID trigger Refresh policies diff --git a/components/brave_policy/brave_profile_policy_provider_unittest.cc b/components/brave_policy/brave_profile_policy_provider_unittest.cc index eed96386888..1b06e0e765d 100644 --- a/components/brave_policy/brave_profile_policy_provider_unittest.cc +++ b/components/brave_policy/brave_profile_policy_provider_unittest.cc @@ -9,11 +9,17 @@ #include "base/files/file_path.h" #include "base/test/task_environment.h" +#include "brave/components/brave_origin/brave_origin_policy_info.h" +#include "brave/components/brave_origin/brave_origin_policy_manager.h" +#include "brave/components/brave_origin/pref_names.h" +#include "brave/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h" +#include "brave/components/brave_shields/core/common/pref_names.h" #include "components/policy/core/common/configuration_policy_provider.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" #include "components/policy/core/common/schema_registry.h" +#include "components/prefs/pref_registry_simple.h" #include "components/prefs/testing_pref_service.h" #include "testing/gtest/include/gtest/gtest.h" @@ -24,14 +30,36 @@ class BraveProfilePolicyProviderTest : public ::testing::Test { BraveProfilePolicyProviderTest() = default; ~BraveProfilePolicyProviderTest() override = default; + void SetUp() override { + // Register prefs needed by both managers so their `Init` calls succeed + // and they report `IsInitialized() == true`. Without this the + // `BravePolicyManagerRegistry::AllInitialized()` gate inside + // `BraveProfilePolicyProvider::OnBravePoliciesReady` would early-return + // and the provider would never call `RefreshPolicies`. + pref_service_.registry()->RegisterDictionaryPref( + brave_origin::kBraveOriginPolicies); + pref_service_.registry()->RegisterBooleanPref( + brave_origin::kOriginPurchaseValidated, false); + pref_service_.registry()->RegisterBooleanPref( + brave_shields::prefs::kAdBlockOnlyModeEnabled, false); + + brave_origin::BraveOriginPolicyManager::GetInstance()->Init( + brave_origin::BraveOriginPolicyMap(), + brave_origin::BraveOriginPolicyMap(), &pref_service_); + AdBlockOnlyModePolicyManager::GetInstance()->Init(&pref_service_); + } + void TearDown() override { if (provider_.IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) { provider_.Shutdown(); } + AdBlockOnlyModePolicyManager::GetInstance()->Shutdown(); + brave_origin::BraveOriginPolicyManager::GetInstance()->Shutdown(); } protected: base::test::TaskEnvironment task_environment_; + TestingPrefServiceSimple pref_service_; policy::SchemaRegistry schema_registry_; BraveProfilePolicyProvider provider_; };