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 <[email protected]> Reviewed-by: Mihai Sardarescu <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#562480}(cherry picked from commit b0b6064) Reviewed-on: https://chromium-review.googlesource.com/1079872 Reviewed-by: Thomas Tangl <[email protected]> Cr-Commit-Position: refs/branch-heads/3440@{#58} Cr-Branched-From: 010ddcf-refs/heads/master@{#561733}
nwjs · May 31, 2018 · 3bde22d · 3bde22d
1 parent d4014a1
commit 3bde22d
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 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
@@ -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
@@ -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<BrowserList, ConsentBumpActivator>
@@ -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<ConsentBumpActivator>(profile);
+  if (IsUnifiedConsentBumpEnabled(profile)) {
+    consent_bump_activator_ =
+        std::make_unique<ConsentBumpActivator>(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
@@ -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*>(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
@@ -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
@@ -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
@@ -33,7 +33,8 @@ using ::testing::_;
 
 class MockLoginUIService : public LoginUIService {
  public:
-  MockLoginUIService() : LoginUIService(nullptr) {}
+  explicit MockLoginUIService(content::BrowserContext* context)
+      : LoginUIService(static_cast<Profile*>(context)) {}
   ~MockLoginUIService() override {}
   MOCK_METHOD3(DisplayLoginResult,
                void(Browser* browser,
@@ -44,7 +45,7 @@ class MockLoginUIService : public LoginUIService {
 
 std::unique_ptr<KeyedService> CreateLoginUIService(
     content::BrowserContext* context) {
-  return std::make_unique<MockLoginUIService>();
+  return std::make_unique<MockLoginUIService>(context);
 }
 
 class UserManagerUIBrowserTest : public InProcessBrowserTest,