[Lacros] IdentityBrowserTestBase tests can trigger AccountManager UI

Tests derived from IdentityBrowserTestBase currently crash if they try to show any system dialog. This happens because AccountManagerMojoService never gets a proper AccountManagerUI implementation. This CL injects a fake implementation of AccountManagerUI into AccountManagerMojoService in IdentityBrowserTestBase tests. These tests cannot use a real implementation because AccountManagerUIImpl is implemented in Ash. Bug: 1263505 Change-Id: I9ebc8e708a9c0ea54c0342d0aba466a735a24372 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3256683 Commit-Queue: Alex Ilin <alexilin@chromium.org> Reviewed-by: Boris Sazonov <bsazonov@chromium.org> Cr-Commit-Position: refs/heads/main@{#939338}
chromium · Nov 8, 2021 · 3f577be · 3f577be
1 parent b185efd
commit 3f577be
Show file tree

Hide file tree

Showing 9 changed files with 184 additions and 66 deletions.
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -7620,6 +7620,10 @@ static_library("test_support") {
     ]
   }
 
+  if (is_chromeos) {
+    deps += [ "//components/account_manager_core:test_support" ]
+  }
+
   if (is_win) {
     sources += [
       "notifications/win/fake_itoastnotification.cc",

diff --git a/chrome/browser/signin/identity_browser_test_base.cc b/chrome/browser/signin/identity_browser_test_base.cc
@@ -17,6 +17,7 @@
 #include "chrome/browser/signin/signin_features.h"
 #include "components/account_manager_core/chromeos/account_manager.h"
 #include "components/account_manager_core/chromeos/account_manager_facade_factory.h"  // nogncheck
+#include "components/account_manager_core/chromeos/fake_account_manager_ui.h"
 #include "content/public/browser/browser_main_parts.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
@@ -29,7 +30,8 @@ class IdentityExtraSetUp : public ChromeBrowserMainExtraParts {
   void PreProfileInit() override {
     // Create and initialize Ash AccountManager.
     scoped_ash_account_manager_ =
-        std::make_unique<ScopedAshAccountManagerForTests>();
+        std::make_unique<ScopedAshAccountManagerForTests>(
+            std::make_unique<FakeAccountManagerUI>());
     auto* account_manager = MaybeGetAshAccountManagerForTests();
     CHECK(account_manager);
     account_manager->InitializeInEphemeralMode(
@@ -63,6 +65,13 @@ class IdentityExtraSetUp : public ChromeBrowserMainExtraParts {
 
 }  // namespace
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+FakeAccountManagerUI* IdentityBrowserTestBase::GetFakeAccountManagerUI() const {
+  return static_cast<FakeAccountManagerUI*>(
+      MaybeGetAshAccountManagerUIForTests());
+}
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
+
 void IdentityBrowserTestBase::CreatedBrowserMainParts(
     content::BrowserMainParts* parts) {
   InProcessBrowserTest::CreatedBrowserMainParts(parts);

diff --git a/chrome/browser/signin/identity_browser_test_base.h b/chrome/browser/signin/identity_browser_test_base.h
@@ -13,6 +13,10 @@ namespace content {
 class BrowserMainParts;
 }
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+class FakeAccountManagerUI;
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
+
 // Base class for browser tests that depend on AccountManager on Lacros - e.g.
 // tests that manage accounts by calling methods like
 // `signin::MakePrimaryAccountAvailable` from identity_test_utils.
@@ -25,6 +29,10 @@ class IdentityBrowserTestBase : public InProcessBrowserTest {
   IdentityBrowserTestBase& operator=(const IdentityBrowserTestBase&) = delete;
   ~IdentityBrowserTestBase() override = default;
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+  FakeAccountManagerUI* GetFakeAccountManagerUI() const;
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
+
  protected:
   void CreatedBrowserMainParts(content::BrowserMainParts* parts) override;
 };

diff --git a/components/account_manager_core/BUILD.gn b/components/account_manager_core/BUILD.gn
@@ -62,11 +62,14 @@ source_set("test_support") {
   sources = [
     "account_manager_test_util.cc",
     "account_manager_test_util.h",
+    "chromeos/fake_account_manager_ui.cc",
+    "chromeos/fake_account_manager_ui.h",
     "mock_account_manager_facade.cc",
     "mock_account_manager_facade.h",
   ]
 
   public_deps = [
+    "//base",
     "//google_apis",
     "//testing/gmock",
   ]

diff --git a/components/account_manager_core/chromeos/account_manager_facade_factory.h b/components/account_manager_core/chromeos/account_manager_facade_factory.h
@@ -14,16 +14,19 @@
 namespace account_manager {
 class AccountManagerFacade;
 class AccountManager;
+class AccountManagerUI;
 }  // namespace account_manager
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
 // Create a new instance of `account_manager::AccountManager` for tests. Should
 // be called before the first call to `GetAccountManagerFacade()`. After this
 // call `GetAccountManagerFacade()` will be returning an instance that is
-// connected to `AccountManagerMojoService`.
+// connected to `AccountManagerMojoService` and `AccountManagerMojoService` will
+// delegate all UI tasks to `account_manager_ui`.
 class COMPONENT_EXPORT(ACCOUNT_MANAGER_CORE) ScopedAshAccountManagerForTests {
  public:
-  ScopedAshAccountManagerForTests();
+  explicit ScopedAshAccountManagerForTests(
+      std::unique_ptr<account_manager::AccountManagerUI> account_manager_ui);
   ~ScopedAshAccountManagerForTests();
 
   ScopedAshAccountManagerForTests(const ScopedAshAccountManagerForTests&) =
@@ -46,6 +49,11 @@ account_manager::AccountManagerFacade* COMPONENT_EXPORT(ACCOUNT_MANAGER_CORE)
 // otherwise return `nullptr`.
 account_manager::AccountManager* COMPONENT_EXPORT(ACCOUNT_MANAGER_CORE)
     MaybeGetAshAccountManagerForTests();
+
+// Return a `AccountManagerUI` instance if it was created for tests,
+// otherwise return `nullptr`.
+account_manager::AccountManagerUI* COMPONENT_EXPORT(ACCOUNT_MANAGER_CORE)
+    MaybeGetAshAccountManagerUIForTests();
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
 
 #endif  // COMPONENTS_ACCOUNT_MANAGER_CORE_CHROMEOS_ACCOUNT_MANAGER_FACADE_FACTORY_H_
diff --git a/components/account_manager_core/chromeos/account_manager_facade_factory_lacros.cc b/components/account_manager_core/chromeos/account_manager_facade_factory_lacros.cc
@@ -36,12 +36,16 @@ mojo::Remote<crosapi::mojom::AccountManager> GetAccountManagerRemote() {
 
 class AccountManagerFacadeFactoryLacros {
  public:
-  void CreateAshAccountManagerForTests() {
+  void CreateAshAccountManagerForTests(
+      std::unique_ptr<account_manager::AccountManagerUI> account_manager_ui) {
     Reset();
     ash_account_manager_ = std::make_unique<account_manager::AccountManager>();
     account_manager_mojo_service_ =
         std::make_unique<crosapi::AccountManagerMojoService>(
             ash_account_manager_.get());
+    account_manager_ui_ = account_manager_ui.get();
+    account_manager_mojo_service_->SetAccountManagerUI(
+        std::move(account_manager_ui));
   }
 
   account_manager::AccountManagerFacadeImpl* GetAccountManagerFacade() {
@@ -55,13 +59,18 @@ class AccountManagerFacadeFactoryLacros {
     return ash_account_manager_.get();
   }
 
+  account_manager::AccountManagerUI* MaybeGetAshAccountManagerUIForTests() {
+    return account_manager_ui_;
+  }
+
   // Reset the pointers.
   void Reset() {
     // AccountManagerFacade depends on AccountManagerMojoService.
     account_manager_facade_.reset();
     // AccountManagerMojoService depends on AccountManager.
     account_manager_mojo_service_.reset();
     ash_account_manager_.reset();
+    account_manager_ui_ = nullptr;
   }
 
  private:
@@ -92,10 +101,13 @@ class AccountManagerFacadeFactoryLacros {
 
   std::unique_ptr<account_manager::AccountManagerFacadeImpl>
       account_manager_facade_;
+
   // Set only in tests:
   std::unique_ptr<account_manager::AccountManager> ash_account_manager_;
   std::unique_ptr<crosapi::AccountManagerMojoService>
       account_manager_mojo_service_;
+  // Owned by `account_manager_mojo_service_`:
+  account_manager::AccountManagerUI* account_manager_ui_ = nullptr;
 };
 
 AccountManagerFacadeFactoryLacros* GetAccountManagerFacadeFactoryLacros() {
@@ -105,11 +117,12 @@ AccountManagerFacadeFactoryLacros* GetAccountManagerFacadeFactoryLacros() {
 
 }  // namespace
 
-ScopedAshAccountManagerForTests::ScopedAshAccountManagerForTests() {
+ScopedAshAccountManagerForTests::ScopedAshAccountManagerForTests(
+    std::unique_ptr<account_manager::AccountManagerUI> account_manager_ui) {
   DCHECK(!MaybeGetAshAccountManagerForTests())  // IN-TEST
       << "Nested ScopedAshAccountManagerForTests are not supported.";
-  GetAccountManagerFacadeFactoryLacros()
-      ->CreateAshAccountManagerForTests();  // IN-TEST
+  GetAccountManagerFacadeFactoryLacros()->CreateAshAccountManagerForTests(
+      std::move(account_manager_ui));  // IN-TEST
 }
 
 ScopedAshAccountManagerForTests::~ScopedAshAccountManagerForTests() {
@@ -126,3 +139,8 @@ account_manager::AccountManager* MaybeGetAshAccountManagerForTests() {
   return GetAccountManagerFacadeFactoryLacros()
       ->MaybeGetAshAccountManagerForTests();  // IN-TEST
 }
+
+account_manager::AccountManagerUI* MaybeGetAshAccountManagerUIForTests() {
+  return GetAccountManagerFacadeFactoryLacros()
+      ->MaybeGetAshAccountManagerUIForTests();  // IN-TEST
+}
diff --git a/components/account_manager_core/chromeos/account_manager_mojo_service_unittest.cc b/components/account_manager_core/chromeos/account_manager_mojo_service_unittest.cc
@@ -22,6 +22,7 @@
 #include "components/account_manager_core/chromeos/access_token_fetcher.h"
 #include "components/account_manager_core/chromeos/account_manager.h"
 #include "components/account_manager_core/chromeos/account_manager_ui.h"
+#include "components/account_manager_core/chromeos/fake_account_manager_ui.h"
 #include "components/prefs/testing_pref_service.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "google_apis/gaia/google_service_auth_error.h"
@@ -110,65 +111,6 @@ class TestAccountManagerObserver
   mojo::Receiver<mojom::AccountManagerObserver> receiver_;
 };
 
-class FakeAccountManagerUI : public account_manager::AccountManagerUI {
- public:
-  FakeAccountManagerUI() = default;
-  FakeAccountManagerUI(const FakeAccountManagerUI&) = delete;
-  FakeAccountManagerUI& operator=(const FakeAccountManagerUI&) = delete;
-  ~FakeAccountManagerUI() override = default;
-
-  void SetIsDialogShown(bool is_dialog_shown) {
-    is_dialog_shown_ = is_dialog_shown;
-  }
-
-  void CloseDialog() {
-    if (!close_dialog_closure_.is_null()) {
-      std::move(close_dialog_closure_).Run();
-    }
-    is_dialog_shown_ = false;
-  }
-
-  int show_account_addition_dialog_calls() const {
-    return show_account_addition_dialog_calls_;
-  }
-
-  int show_account_reauthentication_dialog_calls() const {
-    return show_account_reauthentication_dialog_calls_;
-  }
-
-  int show_manage_accounts_settings_calls() const {
-    return show_manage_accounts_settings_calls_;
-  }
-
- private:
-  // AccountManagerUI overrides:
-  void ShowAddAccountDialog(base::OnceClosure close_dialog_closure) override {
-    close_dialog_closure_ = std::move(close_dialog_closure);
-    show_account_addition_dialog_calls_++;
-    is_dialog_shown_ = true;
-  }
-
-  void ShowReauthAccountDialog(
-      const std::string& email,
-      base::OnceClosure close_dialog_closure) override {
-    close_dialog_closure_ = std::move(close_dialog_closure);
-    show_account_reauthentication_dialog_calls_++;
-    is_dialog_shown_ = true;
-  }
-
-  bool IsDialogShown() override { return is_dialog_shown_; }
-
-  void ShowManageAccountsSettings() override {
-    show_manage_accounts_settings_calls_++;
-  }
-
-  base::OnceClosure close_dialog_closure_;
-  bool is_dialog_shown_ = false;
-  int show_account_addition_dialog_calls_ = 0;
-  int show_account_reauthentication_dialog_calls_ = 0;
-  int show_manage_accounts_settings_calls_ = 0;
-};
-
 // A test spy for intercepting AccountManager calls.
 class AccountManagerSpy : public account_manager::AccountManager {
  public:

diff --git a/components/account_manager_core/chromeos/fake_account_manager_ui.cc b/components/account_manager_core/chromeos/fake_account_manager_ui.cc
@@ -0,0 +1,61 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/account_manager_core/chromeos/fake_account_manager_ui.h"
+
+#include "base/callback.h"
+
+FakeAccountManagerUI::FakeAccountManagerUI() = default;
+FakeAccountManagerUI::~FakeAccountManagerUI() = default;
+
+void FakeAccountManagerUI::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void FakeAccountManagerUI::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void FakeAccountManagerUI::SetIsDialogShown(bool is_dialog_shown) {
+  is_dialog_shown_ = is_dialog_shown;
+}
+
+void FakeAccountManagerUI::CloseDialog() {
+  if (!close_dialog_closure_.is_null()) {
+    std::move(close_dialog_closure_).Run();
+  }
+  is_dialog_shown_ = false;
+}
+
+void FakeAccountManagerUI::ShowAddAccountDialog(
+    base::OnceClosure close_dialog_closure) {
+  close_dialog_closure_ = std::move(close_dialog_closure);
+  show_account_addition_dialog_calls_++;
+  is_dialog_shown_ = true;
+
+  for (auto& obs : observers_)
+    obs.OnAddAccountDialogShown();
+}
+
+void FakeAccountManagerUI::ShowReauthAccountDialog(
+    const std::string& email,
+    base::OnceClosure close_dialog_closure) {
+  close_dialog_closure_ = std::move(close_dialog_closure);
+  show_account_reauthentication_dialog_calls_++;
+  is_dialog_shown_ = true;
+
+  for (auto& obs : observers_)
+    obs.OnReauthAccountDialogShown();
+}
+
+bool FakeAccountManagerUI::IsDialogShown() {
+  return is_dialog_shown_;
+}
+
+void FakeAccountManagerUI::ShowManageAccountsSettings() {
+  show_manage_accounts_settings_calls_++;
+
+  for (auto& obs : observers_)
+    obs.OnManageAccountsSettingsShown();
+}
diff --git a/components/account_manager_core/chromeos/fake_account_manager_ui.h b/components/account_manager_core/chromeos/fake_account_manager_ui.h
@@ -0,0 +1,65 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_ACCOUNT_MANAGER_CORE_CHROMEOS_FAKE_ACCOUNT_MANAGER_UI_H_
+#define COMPONENTS_ACCOUNT_MANAGER_CORE_CHROMEOS_FAKE_ACCOUNT_MANAGER_UI_H_
+
+#include <string>
+
+#include "base/callback_forward.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
+#include "components/account_manager_core/chromeos/account_manager_ui.h"
+
+// Fake implementation of `AccountManagerUI` for tests.
+class FakeAccountManagerUI : public account_manager::AccountManagerUI {
+ public:
+  class Observer : public base::CheckedObserver {
+   public:
+    virtual void OnAddAccountDialogShown() {}
+    virtual void OnReauthAccountDialogShown() {}
+    virtual void OnManageAccountsSettingsShown() {}
+  };
+
+  FakeAccountManagerUI();
+  FakeAccountManagerUI(const FakeAccountManagerUI&) = delete;
+  FakeAccountManagerUI& operator=(const FakeAccountManagerUI&) = delete;
+  ~FakeAccountManagerUI() override;
+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  void SetIsDialogShown(bool is_dialog_shown);
+  void CloseDialog();
+
+  int show_account_addition_dialog_calls() const {
+    return show_account_addition_dialog_calls_;
+  }
+
+  int show_account_reauthentication_dialog_calls() const {
+    return show_account_reauthentication_dialog_calls_;
+  }
+
+  int show_manage_accounts_settings_calls() const {
+    return show_manage_accounts_settings_calls_;
+  }
+
+  // AccountManagerUI overrides:
+  void ShowAddAccountDialog(base::OnceClosure close_dialog_closure) override;
+  void ShowReauthAccountDialog(const std::string& email,
+                               base::OnceClosure close_dialog_closure) override;
+  bool IsDialogShown() override;
+  void ShowManageAccountsSettings() override;
+
+ private:
+  base::OnceClosure close_dialog_closure_;
+  bool is_dialog_shown_ = false;
+  int show_account_addition_dialog_calls_ = 0;
+  int show_account_reauthentication_dialog_calls_ = 0;
+  int show_manage_accounts_settings_calls_ = 0;
+
+  base::ObserverList<Observer> observers_;
+};
+
+#endif  // COMPONENTS_ACCOUNT_MANAGER_CORE_CHROMEOS_FAKE_ACCOUNT_MANAGER_UI_H_