From 3bde22d3887320989762b67fae9c794860bc6b02 Mon Sep 17 00:00:00 2001 From: Thomas Tangl Date: Thu, 31 May 2018 11:52:43 +0000 Subject: [PATCH] Fix infinite loop in LoginUIService The call to LoginUIServiceFactory::GetForProfile(profile_) inside ConsentBumpActivator is removed to fix an infinite loop in the LoginUIService. Additional changes: - ConsentBumpActivator is only created when IsUnifiedConsentBumpEnabled() is true - LoginUIService is now created with the browser context - Missing dependencies are added to LoginUIServiceFactory Bug: 847238 Change-Id: I10850be8c3007ed6e68bc89bd64d9c1971d6a22b Reviewed-on: https://chromium-review.googlesource.com/1076067 Commit-Queue: Thomas Tangl Reviewed-by: Mihai Sardarescu Cr-Original-Commit-Position: refs/heads/master@{#562480}(cherry picked from commit b0b60641b8eeece1ee87fa664ba1109204ca46e5) Reviewed-on: https://chromium-review.googlesource.com/1079872 Reviewed-by: Thomas Tangl Cr-Commit-Position: refs/branch-heads/3440@{#58} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} --- ...hrome_browser_main_extra_parts_profiles.cc | 4 +++ .../ui/webui/signin/login_ui_service.cc | 17 +++++---- .../webui/signin/login_ui_service_factory.cc | 8 +++++ .../webui/signin/login_ui_service_factory.h | 1 + .../webui/signin/login_ui_service_unittest.cc | 35 ++++++++++++++++--- .../signin/user_manager_ui_browsertest.cc | 5 +-- 6 files changed, 58 insertions(+), 12 deletions(-) diff --git a/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc b/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc index 4c2b5a3739d5..9778f5f6b40a 100644 --- a/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc +++ b/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc @@ -79,6 +79,7 @@ #include "chrome/browser/ui/prefs/prefs_tab_helper.h" #include "chrome/browser/ui/tabs/pinned_tab_service_factory.h" #include "chrome/browser/ui/webui/ntp/ntp_resource_cache_factory.h" +#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h" #include "chrome/browser/undo/bookmark_undo_service_factory.h" #include "chrome/browser/web_data_service_factory.h" #include "chrome/common/buildflags.h" @@ -285,6 +286,9 @@ void ChromeBrowserMainExtraPartsProfiles:: #endif #endif LanguageModelFactory::GetInstance(); +#if !defined(OS_ANDROID) + LoginUIServiceFactory::GetInstance(); +#endif if (MediaEngagementService::IsEnabled()) MediaEngagementServiceFactory::GetInstance(); media_router::MediaRouterFactory::GetInstance(); diff --git a/chrome/browser/ui/webui/signin/login_ui_service.cc b/chrome/browser/ui/webui/signin/login_ui_service.cc index 9fe10295d159..3b88d7989241 100644 --- a/chrome/browser/ui/webui/signin/login_ui_service.cc +++ b/chrome/browser/ui/webui/signin/login_ui_service.cc @@ -35,8 +35,11 @@ class ConsentBumpActivator : public BrowserListObserver, public LoginUIService::Observer { public: - explicit ConsentBumpActivator(Profile* profile) - : profile_(profile), + // Creates a ConsentBumpActivator for |profile| which is owned by + // |login_ui_service|. + ConsentBumpActivator(LoginUIService* login_ui_service, Profile* profile) + : login_ui_service_(login_ui_service), + profile_(profile), scoped_browser_list_observer_(this), scoped_login_ui_service_observer_(this) { // Check if there is already an active browser window for |profile|. @@ -57,8 +60,7 @@ class ConsentBumpActivator : public BrowserListObserver, if (ShouldShowConsentBumpFor(profile_)) { selected_browser_ = browser; - scoped_login_ui_service_observer_.Add( - LoginUIServiceFactory::GetForProfile(profile_)); + scoped_login_ui_service_observer_.Add(login_ui_service_); selected_browser_->signin_view_controller()->ShowModalSyncConsentBump( selected_browser_); } @@ -108,6 +110,8 @@ class ConsentBumpActivator : public BrowserListObserver, } private: + LoginUIService* login_ui_service_; // owner + Profile* profile_; ScopedObserver @@ -129,8 +133,9 @@ LoginUIService::LoginUIService(Profile* profile) #endif { #if !defined(OS_CHROMEOS) - if (profile && !profile->IsOffTheRecord() && !profile->IsSupervised()) { - consent_bump_activator_ = std::make_unique(profile); + if (IsUnifiedConsentBumpEnabled(profile)) { + consent_bump_activator_ = + std::make_unique(this, profile); } #endif } diff --git a/chrome/browser/ui/webui/signin/login_ui_service_factory.cc b/chrome/browser/ui/webui/signin/login_ui_service_factory.cc index 66c7b5c39ea8..a36f3c07aba4 100644 --- a/chrome/browser/ui/webui/signin/login_ui_service_factory.cc +++ b/chrome/browser/ui/webui/signin/login_ui_service_factory.cc @@ -7,6 +7,8 @@ #include "base/bind.h" #include "base/bind_helpers.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/signin/signin_manager_factory.h" +#include "chrome/browser/sync/profile_sync_service_factory.h" #include "chrome/browser/ui/webui/signin/login_ui_service.h" #include "components/keyed_service/content/browser_context_dependency_manager.h" #include "components/prefs/pref_service.h" @@ -16,6 +18,8 @@ LoginUIServiceFactory::LoginUIServiceFactory() : BrowserContextKeyedServiceFactory( "LoginUIServiceFactory", BrowserContextDependencyManager::GetInstance()) { + DependsOn(SigninManagerFactory::GetInstance()); + DependsOn(ProfileSyncServiceFactory::GetInstance()); } LoginUIServiceFactory::~LoginUIServiceFactory() {} @@ -43,3 +47,7 @@ KeyedService* LoginUIServiceFactory::BuildServiceInstanceFor( content::BrowserContext* profile) const { return new LoginUIService(static_cast(profile)); } + +bool LoginUIServiceFactory::ServiceIsCreatedWithBrowserContext() const { + return true; +} diff --git a/chrome/browser/ui/webui/signin/login_ui_service_factory.h b/chrome/browser/ui/webui/signin/login_ui_service_factory.h index d940d3466856..f6defd3a23e2 100644 --- a/chrome/browser/ui/webui/signin/login_ui_service_factory.h +++ b/chrome/browser/ui/webui/signin/login_ui_service_factory.h @@ -39,6 +39,7 @@ class LoginUIServiceFactory : public BrowserContextKeyedServiceFactory { // BrowserContextKeyedServiceFactory: KeyedService* BuildServiceInstanceFor( content::BrowserContext* profile) const override; + bool ServiceIsCreatedWithBrowserContext() const override; DISALLOW_COPY_AND_ASSIGN(LoginUIServiceFactory); }; diff --git a/chrome/browser/ui/webui/signin/login_ui_service_unittest.cc b/chrome/browser/ui/webui/signin/login_ui_service_unittest.cc index d77bb0d95b5d..bb219c5379ce 100644 --- a/chrome/browser/ui/webui/signin/login_ui_service_unittest.cc +++ b/chrome/browser/ui/webui/signin/login_ui_service_unittest.cc @@ -7,8 +7,35 @@ #include "base/compiler_specific.h" #include "base/logging.h" #include "base/macros.h" +#include "chrome/test/base/testing_browser_process.h" +#include "chrome/test/base/testing_profile.h" +#include "chrome/test/base/testing_profile_manager.h" +#include "content/public/test/test_browser_thread_bundle.h" #include "testing/gtest/include/gtest/gtest.h" +class LoginUIServiceTest : public testing::Test { + public: + LoginUIServiceTest() + : profile_manager_(TestingBrowserProcess::GetGlobal()), + profile_(nullptr) {} + ~LoginUIServiceTest() override {} + + void SetUp() override { + ASSERT_TRUE(profile_manager_.SetUp()); + profile_ = profile_manager_.CreateTestingProfile("Person 1"); + } + + protected: + content::TestBrowserThreadBundle thread_bundle_; + + TestingProfileManager profile_manager_; + // Test profile used by all tests - this is owned by profile_manager_. + TestingProfile* profile_; + + private: + DISALLOW_COPY_AND_ASSIGN(LoginUIServiceTest); +}; + class TestLoginUI : public LoginUIService::LoginUI { public: TestLoginUI() { } @@ -19,8 +46,8 @@ class TestLoginUI : public LoginUIService::LoginUI { DISALLOW_COPY_AND_ASSIGN(TestLoginUI); }; -TEST(LoginUIServiceTest, CanSetMultipleLoginUIs) { - LoginUIService service(nullptr); +TEST_F(LoginUIServiceTest, CanSetMultipleLoginUIs) { + LoginUIService service(profile_); EXPECT_EQ(nullptr, service.current_login_ui()); @@ -48,8 +75,8 @@ TEST(LoginUIServiceTest, CanSetMultipleLoginUIs) { EXPECT_EQ(nullptr, service.current_login_ui()); } -TEST(LoginUIServiceTest, SetProfileBlockingErrorMessage) { - LoginUIService service(nullptr); +TEST_F(LoginUIServiceTest, SetProfileBlockingErrorMessage) { + LoginUIService service(profile_); service.SetProfileBlockingErrorMessage(); diff --git a/chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc b/chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc index 282ee5794ba7..8de7fc51908a 100644 --- a/chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc +++ b/chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc @@ -33,7 +33,8 @@ using ::testing::_; class MockLoginUIService : public LoginUIService { public: - MockLoginUIService() : LoginUIService(nullptr) {} + explicit MockLoginUIService(content::BrowserContext* context) + : LoginUIService(static_cast(context)) {} ~MockLoginUIService() override {} MOCK_METHOD3(DisplayLoginResult, void(Browser* browser, @@ -44,7 +45,7 @@ class MockLoginUIService : public LoginUIService { std::unique_ptr CreateLoginUIService( content::BrowserContext* context) { - return std::make_unique(); + return std::make_unique(context); } class UserManagerUIBrowserTest : public InProcessBrowserTest,