From af3bc8d2cef79adb8d7c68b50d0d17cfc8e9c1cf Mon Sep 17 00:00:00 2001 From: Paul Moy Date: Mon, 15 May 2023 17:11:14 +0000 Subject: [PATCH] printing: wait for primary profile in CupsProxyServiceManager CupsProxyServiceManager spawns CupsProxyService, which depends on the primary profile existing. Before spawning CupsProxyService, check to make sure the primary profile exists. If the primary profile does not exist, CupsProxyServiceManager will continue periodically checking for the primary profile's existence until it reaches an attempt limit, when it will then stop trying to spawn CupsProxyService. Bug: crbug:1424583 Change-Id: I4baecf0387fb066175e7e55c9ae598503293bdd1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4401076 Reviewed-by: Benjamin Gordon Commit-Queue: Paul Moy Reviewed-by: Hidehiko Abe Cr-Commit-Position: refs/heads/main@{#1144186} --- chrome/browser/ash/BUILD.gn | 6 +- .../printing/cups_proxy_service_manager.cc | 101 +++++++++++++- .../ash/printing/cups_proxy_service_manager.h | 21 ++- .../cups_proxy_service_manager_unittest.cc | 123 ++++++++++++++++++ .../services/cups_proxy/cups_proxy_service.cc | 34 +++-- .../services/cups_proxy/cups_proxy_service.h | 11 +- 6 files changed, 274 insertions(+), 22 deletions(-) create mode 100644 chrome/browser/ash/printing/cups_proxy_service_manager_unittest.cc diff --git a/chrome/browser/ash/BUILD.gn b/chrome/browser/ash/BUILD.gn index 93e5cc0b2fc504..548cd52476aede 100644 --- a/chrome/browser/ash/BUILD.gn +++ b/chrome/browser/ash/BUILD.gn @@ -6261,8 +6261,12 @@ source_set("unit_tests") { } if (use_cups) { - sources += [ "printing/cups_print_job_manager_utils_unittest.cc" ] + sources += [ + "printing/cups_print_job_manager_utils_unittest.cc", + "printing/cups_proxy_service_manager_unittest.cc", + ] deps += [ + "//chromeos/ash/components/dbus/cups_proxy:cups_proxy", "//printing:printing_base", "//printing/backend", ] diff --git a/chrome/browser/ash/printing/cups_proxy_service_manager.cc b/chrome/browser/ash/printing/cups_proxy_service_manager.cc index 6fe43cfcf3ea8f..307439482b57fc 100644 --- a/chrome/browser/ash/printing/cups_proxy_service_manager.cc +++ b/chrome/browser/ash/printing/cups_proxy_service_manager.cc @@ -6,24 +6,63 @@ #include +#include "base/check.h" #include "base/feature_list.h" #include "chrome/browser/ash/printing/cups_proxy_service_delegate_impl.h" #include "chrome/common/chrome_features.h" +#include "chrome/services/cups_proxy/cups_proxy_service.h" +#include "chromeos/ash/components/browser_context_helper/browser_context_helper.h" #include "chromeos/ash/components/dbus/cups_proxy/cups_proxy_client.h" +#include "components/user_manager/user.h" +#include "components/user_manager/user_manager.h" #include "content/public/browser/browser_context.h" namespace ash { -CupsProxyServiceManager::CupsProxyServiceManager() { - // Don't wait for the daemon if the feature is turned off anyway. - if (base::FeatureList::IsEnabled(features::kPluginVm)) { - CupsProxyClient::Get()->WaitForServiceToBeAvailable( - base::BindOnce(&CupsProxyServiceManager::OnDaemonAvailable, - weak_factory_.GetWeakPtr())); +namespace { + +// Returns true iff the primary profile has been created. +bool IsPrimaryProfileCreated() { + if (!user_manager::UserManager::IsInitialized()) { + return false; + } + + const user_manager::User* primary_user = + user_manager::UserManager::Get()->GetPrimaryUser(); + return primary_user && primary_user->is_profile_created(); +} + +} // namespace + +CupsProxyServiceManager::CupsProxyServiceManager() + : profile_manager_(g_browser_process->profile_manager()) { + // Don't wait for the daemon or subscribe to ProfileManager if the feature is + // turned off anyway. + if (!base::FeatureList::IsEnabled(features::kPluginVm)) { + return; + } + + // The primary profile might have been created already. If so, there's no + // need to subscribe to ProfileManager. + primary_profile_available_ = IsPrimaryProfileCreated(); + + if (!primary_profile_available_ && profile_manager_) { + profile_manager_->AddObserver(this); } + + CupsProxyClient::Get()->WaitForServiceToBeAvailable(base::BindOnce( + &CupsProxyServiceManager::OnDaemonAvailable, weak_factory_.GetWeakPtr())); } -CupsProxyServiceManager::~CupsProxyServiceManager() = default; +CupsProxyServiceManager::~CupsProxyServiceManager() { + if (profile_manager_) { + profile_manager_->RemoveObserver(this); + } + + if (cups_proxy::CupsProxyService::GetInstance() != nullptr) { + cups_proxy::CupsProxyService::Shutdown(); + } +} void CupsProxyServiceManager::OnDaemonAvailable(bool daemon_available) { if (!daemon_available) { @@ -31,6 +70,54 @@ void CupsProxyServiceManager::OnDaemonAvailable(bool daemon_available) { return; } + daemon_available_ = true; + + MaybeSpawnCupsProxyService(); +} + +void CupsProxyServiceManager::OnProfileAdded(Profile* profile) { + if (!profile) { + return; + } + + BrowserContextHelper* browser_context_helper = BrowserContextHelper::Get(); + if (!browser_context_helper) { + return; + } + + const user_manager::User* user = + browser_context_helper->GetUserByBrowserContext(profile); + if (!user) { + return; + } + + DCHECK(user_manager::UserManager::IsInitialized()); + if (!user_manager::UserManager::Get()->IsPrimaryUser(user)) { + return; + } + + // Now that we've seen the primary profile, there's no need to keep our + // subscription to ProfileManager. + profile_manager_->RemoveObserver(this); + profile_manager_ = nullptr; + + primary_profile_available_ = true; + + MaybeSpawnCupsProxyService(); +} + +void CupsProxyServiceManager::OnProfileManagerDestroying() { + if (profile_manager_) { + profile_manager_->RemoveObserver(this); + profile_manager_ = nullptr; + } +} + +void CupsProxyServiceManager::MaybeSpawnCupsProxyService() { + if (!primary_profile_available_ || !daemon_available_) { + return; + } + // Attempt to start the service, which will then bootstrap a connection // with the daemon. cups_proxy::CupsProxyService::Spawn( diff --git a/chrome/browser/ash/printing/cups_proxy_service_manager.h b/chrome/browser/ash/printing/cups_proxy_service_manager.h index be74069bbe6ae2..1e568614a31fce 100644 --- a/chrome/browser/ash/printing/cups_proxy_service_manager.h +++ b/chrome/browser/ash/printing/cups_proxy_service_manager.h @@ -6,6 +6,9 @@ #define CHROME_BROWSER_ASH_PRINTING_CUPS_PROXY_SERVICE_MANAGER_H_ #include "base/memory/weak_ptr.h" +#include "chrome/browser/browser_process.h" +#include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/profiles/profile_manager_observer.h" #include "chrome/services/cups_proxy/cups_proxy_service.h" #include "components/keyed_service/core/keyed_service.h" @@ -19,7 +22,8 @@ namespace ash { // // Note: This manager is not fault-tolerant, i.e. should the service/daemon // fail, we do not try to restart. -class CupsProxyServiceManager : public KeyedService { +class CupsProxyServiceManager : public KeyedService, + public ProfileManagerObserver { public: CupsProxyServiceManager(); @@ -28,9 +32,24 @@ class CupsProxyServiceManager : public KeyedService { ~CupsProxyServiceManager() override; + // ProfileManagerObserver overrides: + void OnProfileAdded(Profile* profile) override; + void OnProfileManagerDestroying() override; + private: void OnDaemonAvailable(bool daemon_available); + // Spawns CupsProxyService iff the primary profile is available and the + // CupsProxyDaemon is available. + void MaybeSpawnCupsProxyService(); + + // Whether or not the CupsProxyDaemon is available. + bool daemon_available_ = false; + // Whether or not the primary profile is available. + bool primary_profile_available_ = false; + + ProfileManager* profile_manager_ = nullptr; + base::WeakPtrFactory weak_factory_{this}; }; diff --git a/chrome/browser/ash/printing/cups_proxy_service_manager_unittest.cc b/chrome/browser/ash/printing/cups_proxy_service_manager_unittest.cc new file mode 100644 index 00000000000000..9653cbe5deeb99 --- /dev/null +++ b/chrome/browser/ash/printing/cups_proxy_service_manager_unittest.cc @@ -0,0 +1,123 @@ +// Copyright 2023 The Chromium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/ash/printing/cups_proxy_service_manager.h" + +#include +#include + +#include "base/test/scoped_feature_list.h" +#include "chrome/common/chrome_features.h" +#include "chrome/services/cups_proxy/cups_proxy_service.h" +#include "chrome/test/base/testing_browser_process.h" +#include "chrome/test/base/testing_profile_manager.h" +#include "chromeos/ash/components/dbus/cups_proxy/cups_proxy_client.h" +#include "components/account_id/account_id.h" +#include "components/user_manager/fake_user_manager.h" +#include "components/user_manager/scoped_user_manager.h" +#include "content/public/test/browser_task_environment.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace ash { + +namespace { + +constexpr char kProfileName[] = "user@example.com"; + +} // namespace + +class CupsProxyServiceManagerTest : public testing::Test { + protected: + CupsProxyServiceManagerTest() + : testing_profile_manager_(TestingBrowserProcess::GetGlobal()) {} + CupsProxyServiceManagerTest(const CupsProxyServiceManagerTest&) = delete; + CupsProxyServiceManagerTest& operator=(const CupsProxyServiceManagerTest&) = + delete; + ~CupsProxyServiceManagerTest() override = default; + + void SetUp() override { + ASSERT_TRUE(testing_profile_manager_.SetUp()); + + auto fake_user_manager = std::make_unique(); + fake_user_manager_ = fake_user_manager.get(); + scoped_user_manager_ = std::make_unique( + std::move(fake_user_manager)); + + CupsProxyClient::InitializeFake(); + } + + void TearDown() override { CupsProxyClient::Shutdown(); } + + void CreatePrimaryProfile() { + AccountId account_id = AccountId::FromUserEmail(kProfileName); + fake_user_manager_->AddUser(account_id); + user_manager::UserManager::Get()->UserLoggedIn( + account_id, + user_manager::FakeUserManager::GetFakeUsernameHash(account_id), + /*browser_restart=*/false, + /*is_child=*/false); + testing_profile_manager_.CreateTestingProfile(kProfileName, + /*is_main_profile=*/true); + } + + content::BrowserTaskEnvironment* task_environment() { + return &task_environment_; + } + + base::test::ScopedFeatureList* scoped_feature_list() { + return &scoped_feature_list_; + } + + user_manager::FakeUserManager* fake_user_manager() { + return fake_user_manager_; + } + + private: + content::BrowserTaskEnvironment task_environment_; + base::test::ScopedFeatureList scoped_feature_list_; + TestingProfileManager testing_profile_manager_; + // Owned by `scoped_user_manager_`. + user_manager::FakeUserManager* fake_user_manager_ = nullptr; + std::unique_ptr scoped_user_manager_; +}; + +TEST_F(CupsProxyServiceManagerTest, FeatureNotEnabled) { + scoped_feature_list()->InitAndDisableFeature(features::kPluginVm); + + CupsProxyServiceManager manager; + + EXPECT_EQ(nullptr, cups_proxy::CupsProxyService::GetInstance()); +} + +TEST_F(CupsProxyServiceManagerTest, PrimaryProfileAlreadyCreated) { + scoped_feature_list()->InitAndEnableFeature(features::kPluginVm); + CreatePrimaryProfile(); + + CupsProxyServiceManager manager; + + task_environment()->RunUntilIdle(); + + EXPECT_NE(nullptr, cups_proxy::CupsProxyService::GetInstance()); +} + +TEST_F(CupsProxyServiceManagerTest, PrimaryProfileCreatedLater) { + scoped_feature_list()->InitAndEnableFeature(features::kPluginVm); + + // Before the primary profile has been created, we don't expect + // CupsProxyService to have been spawned. + CupsProxyServiceManager manager; + + task_environment()->RunUntilIdle(); + + EXPECT_EQ(nullptr, cups_proxy::CupsProxyService::GetInstance()); + + CreatePrimaryProfile(); + + task_environment()->RunUntilIdle(); + + EXPECT_NE(nullptr, cups_proxy::CupsProxyService::GetInstance()); +} + +} // namespace ash diff --git a/chrome/services/cups_proxy/cups_proxy_service.cc b/chrome/services/cups_proxy/cups_proxy_service.cc index 2a978cf11a9556..854209945d7c87 100644 --- a/chrome/services/cups_proxy/cups_proxy_service.cc +++ b/chrome/services/cups_proxy/cups_proxy_service.cc @@ -8,7 +8,6 @@ #include #include -#include "base/no_destructor.h" #include "chrome/services/cups_proxy/cups_proxy_service_delegate.h" #include "chrome/services/cups_proxy/proxy_manager.h" #include "chromeos/ash/components/dbus/cups_proxy/cups_proxy_client.h" @@ -18,16 +17,15 @@ namespace cups_proxy { +namespace { + +CupsProxyService* g_instance = nullptr; + +} // namespace + CupsProxyService::CupsProxyService() = default; CupsProxyService::~CupsProxyService() = default; -// static -void CupsProxyService::Spawn( - std::unique_ptr delegate) { - static base::NoDestructor service; - service->BindToCupsProxyDaemon(std::move(delegate)); -} - void CupsProxyService::BindToCupsProxyDaemon( std::unique_ptr delegate) { DCHECK(delegate); @@ -71,4 +69,24 @@ void CupsProxyService::OnBindToCupsProxyDaemon(bool success) { DVLOG(1) << "CupsProxyService: bootstrap success!"; } +// static +void CupsProxyService::Spawn( + std::unique_ptr delegate) { + DCHECK(!g_instance); + g_instance = new CupsProxyService(); + g_instance->BindToCupsProxyDaemon(std::move(delegate)); +} + +// static +CupsProxyService* CupsProxyService::GetInstance() { + return g_instance; +} + +// static +void CupsProxyService::Shutdown() { + DCHECK(g_instance); + delete g_instance; + g_instance = nullptr; +} + } // namespace cups_proxy diff --git a/chrome/services/cups_proxy/cups_proxy_service.h b/chrome/services/cups_proxy/cups_proxy_service.h index 18669f965224d4..9e42a35f43150c 100644 --- a/chrome/services/cups_proxy/cups_proxy_service.h +++ b/chrome/services/cups_proxy/cups_proxy_service.h @@ -20,10 +20,6 @@ class ProxyManager; // This service lives in the browser process and is managed by the // CupsProxyServiceManager. It bootstraps/maintains a mojom connection with the // CupsProxyDaemon. -// -// Note: There is no method granting a service handle since beyond creation, -// this service's only client is the daemon, who's connection is managed -// internally. class CupsProxyService { public: CupsProxyService(const CupsProxyService&) = delete; @@ -32,8 +28,13 @@ class CupsProxyService { // Spawns the global service instance. static void Spawn(std::unique_ptr delegate); + // Gets the global service instance. May be null. + static CupsProxyService* GetInstance(); + + // Destroys the global service instance. + static void Shutdown(); + private: - friend base::NoDestructor; CupsProxyService(); ~CupsProxyService();