[CrOs] Migrate ScreenLockerTest.LockScreenWhileAddingUser

Bug: 1066489
Change-Id: I5fe83470640a2e17b1ae20e99189e7995724459d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2438350
Commit-Queue: Thomas Tellier <tellier@google.com>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823737}

chrome/browser/chromeos/login/lock/screen_locker_browsertest.cc

@@ -109,14 +109,6 @@ IN_PROC_BROWSER_TEST_F(ScreenLockerTest, TestBadThenGoodPassword) {
       1, session_manager_client()->notify_lock_screen_dismissed_call_count());
 }
 
-// Makes sure Chrome doesn't crash if we lock the screen during an add-user
-// flow. Regression test for crbug.com/467111.
-IN_PROC_BROWSER_TEST_F(ScreenLockerTest, LockScreenWhileAddingUser) {
-  UserAddingScreen::Get()->Start();
-  base::RunLoop().RunUntilIdle();
-  ScreenLocker::HandleShowLockScreenRequest();
-}
-
 // Test how locking the screen affects an active fullscreen window.
 IN_PROC_BROWSER_TEST_F(ScreenLockerTest, TestFullscreenExit) {
   // 1) If the active browser window is in fullscreen and the fullscreen window

chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc

@@ -19,6 +19,7 @@
 #include "chrome/browser/chromeos/login/test/js_checker.h"
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
 #include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
+#include "chrome/browser/chromeos/login/test/user_adding_screen_utils.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/browser/chromeos/login/ui/user_adding_screen.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
@@ -71,30 +72,6 @@ void Append_en_US_InputMethods(std::vector<std::string>* out) {
   chromeos::input_method::InputMethodManager::Get()->MigrateInputMethods(out);
 }
 
-class LoginScreenWaiter : public LoginScreenShownObserver {
- public:
-  LoginScreenWaiter() {
-    LoginScreenClient::Get()->AddLoginScreenShownObserver(this);
-  }
-  ~LoginScreenWaiter() override {
-    LoginScreenClient::Get()->RemoveLoginScreenShownObserver(this);
-  }
-
-  // LoginScreenShownObserver:
-  void OnLoginScreenShown() override { run_loop_.Quit(); }
-
-  void Wait() { run_loop_.Run(); }
-
- private:
-  base::RunLoop run_loop_;
-};
-
-void ShowUserAddingScreen() {
-  LoginScreenWaiter waiter;
-  UserAddingScreen::Get()->Start();
-  waiter.Wait();
-}
-
 }  // anonymous namespace
 
 class LoginUIKeyboardTest : public chromeos::LoginManagerTest {
@@ -161,7 +138,7 @@ IN_PROC_BROWSER_TEST_F(LoginUIUserAddingKeyboardTest, CheckPODSwitches) {
   LoginUser(test_users_[2]);
   const std::string logged_user_input_method =
       lock_screen_utils::GetUserLastInputMethod(test_users_[2]);
-  ShowUserAddingScreen();
+  test::ShowUserAddingScreen();
 
   std::vector<std::string> expected_input_methods;
   expected_input_methods.push_back(user_input_methods[0]);
@@ -425,7 +402,12 @@ class LoginUIDevicePolicyUserAdding : public LoginUIKeyboardPolicy {
   LoginUIDevicePolicyUserAdding() {
     // Need at least two to run user adding screen.
     login_manager_.AppendRegularUsers(2);
+    scoped_feature_list_.InitAndEnableFeature(
+      features::kViewBasedMultiprofileLogin);
   }
+  
+ protected:
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_F(LoginUIDevicePolicyUserAdding, PolicyNotHonored) {
@@ -441,8 +423,7 @@ IN_PROC_BROWSER_TEST_F(LoginUIDevicePolicyUserAdding, PolicyNotHonored) {
   chromeos::input_method::InputMethodManager::Get()->MigrateInputMethods(
       &allowed_input_method);
 
-  UserAddingScreen::Get()->Start();
-  OobeScreenWaiter(OobeScreen::SCREEN_ACCOUNT_PICKER).Wait();
+  test::ShowUserAddingScreen();
 
   auto user_adding_ime_state = input_manager->GetActiveIMEState();
   EXPECT_NE(user_ime_state, user_adding_ime_state);

chrome/browser/chromeos/login/test/user_adding_screen_utils.cc

+// Copyright 2020 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 "chrome/browser/chromeos/login/test/user_adding_screen_utils.h"
+
+#include "base/run_loop.h"
+#include "chrome/browser/chromeos/login/ui/user_adding_screen.h"
+#include "chrome/browser/ui/ash/login_screen_client.h"
+#include "chrome/browser/ui/ash/login_screen_shown_observer.h"
+
+namespace chromeos {
+namespace test {
+
+namespace {
+
+// A waiter that blocks until the target login screen is reached.
+class LoginScreenWaiter : public LoginScreenShownObserver {
+ public:
+  LoginScreenWaiter() {
+    LoginScreenClient::Get()->AddLoginScreenShownObserver(this);
+  }
+  ~LoginScreenWaiter() override {
+    LoginScreenClient::Get()->RemoveLoginScreenShownObserver(this);
+  }
+  LoginScreenWaiter(const LoginScreenWaiter&) = delete;
+  LoginScreenWaiter& operator=(const LoginScreenWaiter&) = delete;
+
+  // LoginScreenShownObserver:
+  void OnLoginScreenShown() override { run_loop_.Quit(); }
+
+  void Wait() { run_loop_.Run(); }
+
+ private:
+  base::RunLoop run_loop_;
+};
+
+}  // namespace
+
+void ShowUserAddingScreen() {
+  LoginScreenWaiter waiter;
+  UserAddingScreen::Get()->Start();
+  waiter.Wait();
+}
+
+}  // namespace test
+}  // namespace chromeos

chrome/browser/chromeos/login/test/user_adding_screen_utils.h

+// Copyright 2020 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 CHROME_BROWSER_CHROMEOS_LOGIN_TEST_USER_ADDING_SCREEN_UTILS_H_
+#define CHROME_BROWSER_CHROMEOS_LOGIN_TEST_USER_ADDING_SCREEN_UTILS_H_
+
+namespace chromeos {
+namespace test {
+
+// Utility function for user adding screen that waits for the user adding screen
+// to be shown.
+void ShowUserAddingScreen();
+
+}  // namespace test
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_LOGIN_TEST_USER_ADDING_SCREEN_UTILS_H_

chrome/browser/chromeos/login/ui/user_adding_screen_browsertest.cc

@@ -15,13 +15,13 @@
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
 #include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
 #include "chrome/browser/chromeos/login/test/session_manager_state_waiter.h"
+#include "chrome/browser/chromeos/login/test/user_adding_screen_utils.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/browser/chromeos/login/ui/user_adding_screen.h"
 #include "chrome/browser/chromeos/login/users/multi_profile_user_controller.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/ash/login_screen_client.h"
-#include "chrome/browser/ui/ash/login_screen_shown_observer.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/constants/chromeos_features.h"
@@ -48,11 +48,15 @@ constexpr char kLoginFormatJS[] = R"({
 
 }  // namespace
 
+// TODO(1066489): Merge UserAddingScreenViewBasedTest into UserAddingScreenTest
+// once kViewBasedMultiprofileLogin is enabled by default.
 class UserAddingScreenTest : public LoginManagerTest,
                              public UserAddingScreen::Observer {
  public:
   UserAddingScreenTest() : LoginManagerTest() {
     login_mixin_.AppendRegularUsers(3);
+    feature_list_.InitAndDisableFeature(
+        chromeos::features::kViewBasedMultiprofileLogin);
   }
 
   void SetUpInProcessBrowserTestFixture() override {
@@ -104,6 +108,9 @@ class UserAddingScreenTest : public LoginManagerTest,
   std::vector<AccountId> users_in_session_order_;
   LoginManagerMixin login_mixin_{&mixin_host_};
 
+ protected:
+  base::test::ScopedFeatureList feature_list_;
+
  private:
   int user_adding_started_ = 0;
   int user_adding_finished_ = 0;
@@ -150,33 +157,13 @@ IN_PROC_BROWSER_TEST_F(UserAddingScreenTest, CancelAdding) {
             users[0].account_id);
 }
 
-class UserAddingScreenViewBasedTest : public UserAddingScreenTest,
-                                      public LoginScreenShownObserver {
+class UserAddingScreenViewBasedTest : public UserAddingScreenTest {
  public:
   UserAddingScreenViewBasedTest() : UserAddingScreenTest() {
-    feature_list_.InitWithFeatures(
-        {chromeos::features::kViewBasedMultiprofileLogin}, {});
-  }
-
-  // LoginScreenShownObserver:
-  void OnLoginScreenShown() override {
-    LoginScreenClient::Get()->RemoveLoginScreenShownObserver(this);
-    login_screen_shown_ = true;
-    if (run_loop_)
-      run_loop_->Quit();
-  }
-
-  void WaitUntilLoginScreenShown() {
-    if (login_screen_shown_)
-      return;
-    run_loop_ = std::make_unique<base::RunLoop>();
-    run_loop_->Run();
+    feature_list_.Reset();
+    feature_list_.InitAndEnableFeature(
+        chromeos::features::kViewBasedMultiprofileLogin);
   }
-
- private:
-  bool login_screen_shown_ = false;
-  std::unique_ptr<base::RunLoop> run_loop_;
-  base::test::ScopedFeatureList feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_F(UserAddingScreenViewBasedTest, CancelAdding) {
@@ -192,9 +179,7 @@ IN_PROC_BROWSER_TEST_F(UserAddingScreenViewBasedTest, CancelAdding) {
             session_manager::SessionState::ACTIVE);
 
   base::HistogramTester histogram_tester;
-  UserAddingScreen::Get()->Start();
-  LoginScreenClient::Get()->AddLoginScreenShownObserver(this);
-  WaitUntilLoginScreenShown();
+  test::ShowUserAddingScreen();
 
   EXPECT_EQ(user_adding_started(), 1);
   EXPECT_EQ(session_manager::SessionManager::Get()->session_state(),
@@ -265,9 +250,7 @@ IN_PROC_BROWSER_TEST_F(UserAddingScreenViewBasedTest, UILogin) {
   user_manager::UserManager* user_manager = user_manager::UserManager::Get();
 
   base::HistogramTester histogram_tester;
-  UserAddingScreen::Get()->Start();
-  LoginScreenClient::Get()->AddLoginScreenShownObserver(this);
-  WaitUntilLoginScreenShown();
+  test::ShowUserAddingScreen();
 
   EXPECT_EQ(user_adding_started(), 1);
   EXPECT_EQ(session_manager::SessionManager::Get()->session_state(),
@@ -416,9 +399,7 @@ IN_PROC_BROWSER_TEST_F(UserAddingScreenViewBasedTest, AddingSeveralUsers) {
 
   const int n = users.size();
   for (int i = 1; i < n; ++i) {
-    UserAddingScreen::Get()->Start();
-    LoginScreenClient::Get()->AddLoginScreenShownObserver(this);
-    WaitUntilLoginScreenShown();
+    test::ShowUserAddingScreen();
 
     EXPECT_EQ(user_adding_started(), i);
     EXPECT_EQ(session_manager::SessionManager::Get()->session_state(),
@@ -619,4 +600,30 @@ IN_PROC_BROWSER_TEST_F(UserAddingScreenViewBasedTest, InfoBubbleVisible) {
             session_manager::SessionState::ACTIVE);
 }
 
+// Makes sure Chrome doesn't crash if we lock the screen during an add-user
+// flow. Regression test for crbug.com/467111.
+// Note that this test has been moved from ScreenLockerTest because it is easier
+// to login a user here; and without any logged user on the user adding screen,
+// a OOBE dialog would appear, making the test crash.
+IN_PROC_BROWSER_TEST_F(UserAddingScreenViewBasedTest,
+                       LockScreenWhileAddingUser) {
+  const auto& users = login_mixin_.users();
+  EXPECT_EQ(users.size(), user_manager::UserManager::Get()->GetUsers().size());
+  EXPECT_EQ(user_manager::UserManager::Get()->GetLoggedInUsers().size(), 0u);
+  EXPECT_EQ(session_manager::SessionManager::Get()->session_state(),
+            session_manager::SessionState::LOGIN_PRIMARY);
+  LoginUser(users[0].account_id);
+  EXPECT_EQ(user_manager::UserManager::Get()->GetLoggedInUsers().size(), 1u);
+  EXPECT_EQ(session_manager::SessionManager::Get()->session_state(),
+            session_manager::SessionState::ACTIVE);
+
+  UserAddingScreen::Get()->Start();
+  EXPECT_EQ(user_adding_started(), 1);
+  EXPECT_EQ(session_manager::SessionManager::Get()->session_state(),
+            session_manager::SessionState::LOGIN_SECONDARY);
+  base::RunLoop().RunUntilIdle();
+
+  ScreenLocker::HandleShowLockScreenRequest();
+}
+
 }  // namespace chromeos

chrome/test/BUILD.gn

@@ -2602,6 +2602,8 @@ if (!is_android) {
         "../browser/chromeos/login/test/session_flags_manager.h",
         "../browser/chromeos/login/test/session_manager_state_waiter.cc",
         "../browser/chromeos/login/test/session_manager_state_waiter.h",
+        "../browser/chromeos/login/test/user_adding_screen_utils.cc",
+        "../browser/chromeos/login/test/user_adding_screen_utils.h",
         "../browser/chromeos/login/test/user_policy_mixin.cc",
         "../browser/chromeos/login/test/user_policy_mixin.h",
         "../browser/chromeos/login/test/webview_content_extractor.cc",