From 08de6ceebce218d70ab796de28bbd3cf7007dc80 Mon Sep 17 00:00:00 2001 From: Serg Date: Thu, 7 May 2026 16:25:06 -0400 Subject: [PATCH] Revert "[Origin] Wait for all policy managers before refreshing profile policies" (#36258) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revert "[Origin] Wait for all policy managers before refreshing profile polic…" This reverts commit a044cc5bdebf2f40efaa41539af9ef81ca0feb28. --- 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, 2 insertions(+), 292 deletions(-) delete mode 100644 components/brave_policy/brave_policy_manager_registry.cc delete mode 100644 components/brave_policy/brave_policy_manager_registry.h delete 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 9e76bb61871..d68163bb6e0 100644 --- a/components/brave_origin/BUILD.gn +++ b/components/brave_origin/BUILD.gn @@ -68,7 +68,6 @@ 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 0d7ad6271a2..5cda8ec41e1 100644 --- a/components/brave_origin/brave_origin_policy_manager.cc +++ b/components/brave_origin/brave_origin_policy_manager.cc @@ -6,7 +6,6 @@ #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" @@ -219,10 +218,7 @@ bool BraveOriginPolicyManager::GetPolicyValueInternal( return default_value; } -BraveOriginPolicyManager::BraveOriginPolicyManager() - : registration_( - base::BindRepeating(&BraveOriginPolicyManager::IsInitialized, - base::Unretained(this))) {} +BraveOriginPolicyManager::BraveOriginPolicyManager() = default; BraveOriginPolicyManager::~BraveOriginPolicyManager() = default; diff --git a/components/brave_origin/brave_origin_policy_manager.h b/components/brave_origin/brave_origin_policy_manager.h index b1fd5df39fe..390741877af 100644 --- a/components/brave_origin/brave_origin_policy_manager.h +++ b/components/brave_origin/brave_origin_policy_manager.h @@ -12,7 +12,6 @@ #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; @@ -103,11 +102,6 @@ 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 b1bbd56ad36..4ce183e0f1b 100644 --- a/components/brave_policy/BUILD.gn +++ b/components/brave_policy/BUILD.gn @@ -13,7 +13,6 @@ static_library("brave_policy") { ] deps = [ - ":brave_policy_manager_registry", ":brave_policy_observer", "//base", "//brave/components/brave_origin", @@ -24,15 +23,6 @@ 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" ] @@ -44,19 +34,14 @@ 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 317652d4d8c..9cae45f489c 100644 --- a/components/brave_policy/ad_block_only_mode/BUILD.gn +++ b/components/brave_policy/ad_block_only_mode/BUILD.gn @@ -17,7 +17,6 @@ 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 5f34429ce3e..b634a5ef847 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,7 +6,6 @@ #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" @@ -44,10 +43,6 @@ void AdBlockOnlyModePolicyManager::Shutdown() { local_state_ = nullptr; } -bool AdBlockOnlyModePolicyManager::IsInitialized() const { - return local_state_ != nullptr; -} - void AdBlockOnlyModePolicyManager::AddObserver(BravePolicyObserver* observer) { observers_.AddObserver(observer); @@ -76,10 +71,7 @@ AdBlockOnlyModePolicies AdBlockOnlyModePolicyManager::GetPolicies() const { return GetPoliciesImpl(); } -AdBlockOnlyModePolicyManager::AdBlockOnlyModePolicyManager() - : registration_( - base::BindRepeating(&AdBlockOnlyModePolicyManager::IsInitialized, - base::Unretained(this))) {} +AdBlockOnlyModePolicyManager::AdBlockOnlyModePolicyManager() = default; 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 377e7f56bef..7e0752675f8 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,7 +10,6 @@ #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" @@ -37,8 +36,6 @@ class AdBlockOnlyModePolicyManager final { void Init(PrefService* local_state); void Shutdown(); - bool IsInitialized() const; - void AddObserver(BravePolicyObserver* observer); void RemoveObserver(BravePolicyObserver* observer); @@ -59,11 +56,6 @@ 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 deleted file mode 100644 index a477930e48e..00000000000 --- a/components/brave_policy/brave_policy_manager_registry.cc +++ /dev/null @@ -1,57 +0,0 @@ -/* 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 deleted file mode 100644 index c8fb10e5026..00000000000 --- a/components/brave_policy/brave_policy_manager_registry.h +++ /dev/null @@ -1,65 +0,0 @@ -/* 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 deleted file mode 100644 index 1ef42f93206..00000000000 --- a/components/brave_policy/brave_policy_manager_registry_unittest.cc +++ /dev/null @@ -1,82 +0,0 @@ -/* 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 125afebb752..25e4d255d8b 100644 --- a/components/brave_policy/brave_profile_policy_provider.cc +++ b/components/brave_policy/brave_profile_policy_provider.cc @@ -11,7 +11,6 @@ #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" @@ -86,20 +85,6 @@ 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 1b06e0e765d..eed96386888 100644 --- a/components/brave_policy/brave_profile_policy_provider_unittest.cc +++ b/components/brave_policy/brave_profile_policy_provider_unittest.cc @@ -9,17 +9,11 @@ #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" @@ -30,36 +24,14 @@ 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_; };