cros: Send primary user session after profile load

Fix a regression that sends primary user session too early,
which breaks the assumption for callers of IsActiveUserSessionStarted
and causes SessionObserver::OnActiveUserPrefServiceChanged not
fired.


Bug: 968825, 848230
Change-Id: I18b042a2ee5801b66e175897c0704d8dde989113
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1660772Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710059}

ash/BUILD.gn

@@ -2207,9 +2207,12 @@ static_library("test_support") {
   public = [
     "public/cpp/shelf_test_api.h",
     "public/cpp/system_tray_test_api.h",
+    "public/cpp/test/accessibility_controller_test_api.h",
     "public/cpp/test/assistant_test_api.h",
   ]
   sources = [
+    "accessibility/accessibility_controller_test_api_impl.cc",
+    "accessibility/accessibility_controller_test_api_impl.h",
     "accessibility/test_accessibility_controller_client.cc",
     "accessibility/test_accessibility_controller_client.h",
     "app_list/test/app_list_test_helper.cc",

ash/accessibility/accessibility_controller_test_api_impl.cc

+// Copyright 2019 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 "ash/accessibility/accessibility_controller_test_api_impl.h"
+
+#include "ash/accelerators/accelerator_controller_impl.h"
+#include "ash/accessibility/accessibility_controller_impl.h"
+#include "ash/shell.h"
+
+namespace ash {
+
+namespace {
+
+AccessibilityControllerImpl* GetController() {
+  return Shell::Get()->accessibility_controller();
+}
+
+}  // namespace
+
+AccessibilityControllerTestApiImpl::AccessibilityControllerTestApiImpl() =
+    default;
+
+AccessibilityControllerTestApiImpl::~AccessibilityControllerTestApiImpl() =
+    default;
+
+void AccessibilityControllerTestApiImpl::SetLargeCursorEnabled(bool enabled) {
+  GetController()->SetLargeCursorEnabled(enabled);
+}
+
+bool AccessibilityControllerTestApiImpl::IsLargeCursorEnabled() const {
+  return GetController()->large_cursor_enabled();
+}
+
+// static
+std::unique_ptr<AccessibilityControllerTestApi>
+AccessibilityControllerTestApi::Create() {
+  return std::make_unique<AccessibilityControllerTestApiImpl>();
+}
+
+}  // namespace ash

ash/accessibility/accessibility_controller_test_api_impl.h

+// Copyright 2019 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 ASH_ACCESSIBILITY_ACCESSIBILITY_CONTROLLER_TEST_API_IMPL_H_
+#define ASH_ACCESSIBILITY_ACCESSIBILITY_CONTROLLER_TEST_API_IMPL_H_
+
+#include "ash/public/cpp/test/accessibility_controller_test_api.h"
+#include "base/macros.h"
+
+namespace ash {
+
+// Implementation of AccessibilityControllerTestApi.
+class AccessibilityControllerTestApiImpl
+    : public AccessibilityControllerTestApi {
+ public:
+  AccessibilityControllerTestApiImpl();
+  ~AccessibilityControllerTestApiImpl() override;
+
+  // AccessibilityControllerTestApi:
+  void SetLargeCursorEnabled(bool enabled) override;
+  bool IsLargeCursorEnabled() const override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AccessibilityControllerTestApiImpl);
+};
+
+}  // namespace ash
+
+#endif  // ASH_ACCESSIBILITY_ACCESSIBILITY_CONTROLLER_TEST_API_IMPL_H_

ash/public/cpp/test/accessibility_controller_test_api.h

+// Copyright 2019 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 ASH_PUBLIC_CPP_TEST_ACCESSIBILITY_CONTROLLER_TEST_API_H_
+#define ASH_PUBLIC_CPP_TEST_ACCESSIBILITY_CONTROLLER_TEST_API_H_
+
+#include <memory>
+
+#include "ash/ash_export.h"
+
+namespace ash {
+
+// Test API for AccessibilityController in ash. It is intended for
+// integration browser tests that need to verify AccessibilityController
+// in ash works as expected.
+class ASH_EXPORT AccessibilityControllerTestApi {
+ public:
+  AccessibilityControllerTestApi() = default;
+  virtual ~AccessibilityControllerTestApi() = default;
+
+  static std::unique_ptr<AccessibilityControllerTestApi> Create();
+
+  virtual void SetLargeCursorEnabled(bool enabled) = 0;
+  virtual bool IsLargeCursorEnabled() const = 0;
+};
+
+}  // namespace ash
+
+#endif  // ASH_PUBLIC_CPP_TEST_ACCESSIBILITY_CONTROLLER_TEST_API_H_

ash/session/session_controller_impl.cc

@@ -507,15 +507,17 @@ void SessionControllerImpl::SetSessionState(SessionState state) {
 }
 
 void SessionControllerImpl::AddUserSession(const UserSession& user_session) {
-  const AccountId account_id(user_session.user_info.account_id);
-
   if (primary_session_id_ == 0u)
     primary_session_id_ = user_session.session_id;
 
   user_sessions_.push_back(std::make_unique<UserSession>(user_session));
 
-  OnProfilePrefServiceInitialized(account_id,
-                                  GetUserPrefServiceForUser(account_id));
+  const AccountId account_id(user_session.user_info.account_id);
+  PrefService* user_prefs = GetUserPrefServiceForUser(account_id);
+  // |user_prefs| could be null in tests.
+  if (user_prefs)
+    OnProfilePrefServiceInitialized(account_id, user_prefs);
+
   UpdateLoginStatus();
   for (auto& observer : observers_)
     observer.OnUserSessionAdded(account_id);

chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/accessibility/accessibility_manager.h"
 
 #include "ash/public/cpp/ash_pref_names.h"
+#include "ash/public/cpp/test/accessibility_controller_test_api.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ -548,7 +549,10 @@ class AccessibilityManagerLoginTest : public OobeBaseTest {
         user_manager::UserManager::Get()
             ->FindUser(account_id)
             ->username_hash());
-    session_manager::SessionManager::Get()->SessionStarted();
+
+    auto* session_manager = session_manager::SessionManager::Get();
+    session_manager->NotifyUserProfileLoaded(account_id);
+    session_manager->SessionStarted();
   }
 
   void SetBrailleDisplayAvailability(bool available) {
@@ -638,6 +642,30 @@ IN_PROC_BROWSER_TEST_F(AccessibilityManagerLoginTest, MAYBE_Login) {
   EXPECT_TRUE(IsMonoAudioEnabled());
 }
 
+// Tests that ash and browser process has the same states after sign-in.
+IN_PROC_BROWSER_TEST_F(AccessibilityManagerLoginTest, AshState) {
+  WaitForSigninScreen();
+  CreateSession(test_account_id_);
+  StartUserSession(test_account_id_);
+
+  auto ash_a11y_controller_test_api =
+      ash::AccessibilityControllerTestApi::Create();
+
+  // Ash and browser has the same state.
+  EXPECT_FALSE(IsLargeCursorEnabled());
+  EXPECT_FALSE(ash_a11y_controller_test_api->IsLargeCursorEnabled());
+
+  // Changes from the browser side is reflected in both browser and ash.
+  SetLargeCursorEnabled(true);
+  EXPECT_TRUE(IsLargeCursorEnabled());
+  EXPECT_TRUE(ash_a11y_controller_test_api->IsLargeCursorEnabled());
+
+  // Changes from ash is also reflect in both browser and ash.
+  ash_a11y_controller_test_api->SetLargeCursorEnabled(false);
+  EXPECT_FALSE(IsLargeCursorEnabled());
+  EXPECT_FALSE(ash_a11y_controller_test_api->IsLargeCursorEnabled());
+}
+
 class AccessibilityManagerUserTypeTest
     : public AccessibilityManagerTest,
       public WithParamInterface<user_manager::UserType> {

chrome/browser/chromeos/accessibility/magnification_manager_browsertest.cc

@@ -99,12 +99,14 @@ void PrepareNonNewProfile(const AccountId& account_id) {
 }
 
 // Simulates how UserSessionManager starts a user session by loading user
-// profile and mark session as started.
+// profile, notify user profile is loaded, and mark session as started.
 void StartUserSession(const AccountId& account_id) {
   ProfileHelper::GetProfileByUserIdHashForTest(
       user_manager::UserManager::Get()->FindUser(account_id)->username_hash());
 
-  session_manager::SessionManager::Get()->SessionStarted();
+  auto* session_manager = session_manager::SessionManager::Get();
+  session_manager->NotifyUserProfileLoaded(account_id);
+  session_manager->SessionStarted();
 }
 
 }  // namespace

chrome/browser/chromeos/login/session/user_session_manager.cc

@@ -1492,6 +1492,9 @@ void UserSessionManager::UserProfileInitialized(Profile* profile,
         content::NotificationService::AllSources(),
         content::Details<Profile>(profile));
 
+    session_manager::SessionManager::Get()->NotifyUserProfileLoaded(
+        ProfileHelper::Get()->GetUserByProfile(profile)->GetAccountId());
+
     if (delegate_)
       delegate_->OnProfilePrepared(profile, false);
 

chrome/browser/ui/ash/session_controller_client_impl.cc

@@ -15,7 +15,6 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -87,8 +86,10 @@ std::unique_ptr<ash::UserSession> UserToUserSession(const User& user) {
   if (user_session_id == 0u)
     return nullptr;
 
-  auto session = std::make_unique<ash::UserSession>();
   Profile* profile = chromeos::ProfileHelper::Get()->GetProfileByUser(&user);
+  DCHECK(profile);
+
+  auto session = std::make_unique<ash::UserSession>();
   session->session_id = user_session_id;
   session->user_info.type = user.GetType();
   session->user_info.account_id = user.GetAccountId();
@@ -99,11 +100,9 @@ std::unique_ptr<ash::UserSession> UserToUserSession(const User& user) {
   session->user_info.has_gaia_account = user.has_gaia_account();
   session->user_info.should_display_managed_ui =
       profile && chrome::ShouldDisplayManagedUi(profile);
-  if (profile) {
-    session->user_info.service_instance_group =
-        content::BrowserContext::GetServiceInstanceGroupFor(profile);
-    session->user_info.is_new_profile = profile->IsNewProfile();
-  }
+  session->user_info.service_instance_group =
+      content::BrowserContext::GetServiceInstanceGroupFor(profile);
+  session->user_info.is_new_profile = profile->IsNewProfile();
 
   session->user_info.avatar.image = user.GetImage();
   if (session->user_info.avatar.image.isNull()) {
@@ -113,13 +112,10 @@ std::unique_ptr<ash::UserSession> UserToUserSession(const User& user) {
   }
 
   if (user.IsSupervised()) {
-    if (profile) {
-      SupervisedUserService* service =
-          SupervisedUserServiceFactory::GetForProfile(profile);
-      session->custodian_email = service->GetCustodianEmailAddress();
-      session->second_custodian_email =
-          service->GetSecondCustodianEmailAddress();
-    }
+    SupervisedUserService* service =
+        SupervisedUserServiceFactory::GetForProfile(profile);
+    session->custodian_email = service->GetCustodianEmailAddress();
+    session->second_custodian_email = service->GetSecondCustodianEmailAddress();
   }
 
   chromeos::UserFlow* const user_flow =
@@ -170,8 +166,6 @@ SessionControllerClientImpl::SessionControllerClientImpl() {
 
   registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
                  content::NotificationService::AllSources());
-  registrar_.Add(this, chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED,
-                 content::NotificationService::AllSources());
 
   local_state_registrar_ = std::make_unique<PrefChangeRegistrar>();
   local_state_registrar_->Init(g_browser_process->local_state());
@@ -348,19 +342,12 @@ bool SessionControllerClientImpl::IsMultiProfileAvailable() {
 void SessionControllerClientImpl::ActiveUserChanged(User* active_user) {
   SendSessionInfoIfChanged();
 
-  // UserAddedToSession is not called for the primary user session so its meta
-  // data here needs to be sent to ash before setting user session order.
-  // However, ActiveUserChanged happens at different timing for primary user
-  // and secondary users. For primary user, it happens before user profile load.
-  // For secondary users, it happens after user profile load. This caused
-  // confusing down the path. Bail out here to defer the primary user session
-  // metadata  sent until it becomes active so that ash side could expect a
-  // consistent state.
-  // TODO(xiyuan): Get rid of this after http://crbug.com/657149 refactoring.
-  if (!primary_user_session_sent_ &&
-      UserManager::Get()->GetPrimaryUser() == active_user) {
+  // Try to send user session before updating the order. Skip sending session
+  // order if user session ends up to be pending (due to user profile loading).
+  // TODO(crbug.com/657149): Get rid of this after refactoring.
+  SendUserSession(*active_user);
+  if (pending_users_.find(active_user->GetAccountId()) != pending_users_.end())
     return;
-  }
 
   SendUserSessionOrder();
 }
@@ -483,20 +470,22 @@ void SessionControllerClientImpl::DoCycleActiveUser(
 }
 
 void SessionControllerClientImpl::OnSessionStateChanged() {
-  // Sent the primary user metadata and user session order that are deferred
-  // from ActiveUserChanged before update session state.
-  if (!primary_user_session_sent_ &&
-      SessionManager::Get()->session_state() == SessionState::ACTIVE) {
-    DCHECK_EQ(UserManager::Get()->GetPrimaryUser(),
-              UserManager::Get()->GetActiveUser());
-    primary_user_session_sent_ = true;
-    SendUserSession(*UserManager::Get()->GetPrimaryUser());
-    SendUserSessionOrder();
+  if (SessionManager::Get()->session_state() == SessionState::ACTIVE) {
+    // The active user should not be pending when the session becomes active.
+    DCHECK(pending_users_.find(
+               UserManager::Get()->GetActiveUser()->GetAccountId()) ==
+           pending_users_.end());
   }
 
   SendSessionInfoIfChanged();
 }
 
+void SessionControllerClientImpl::OnUserProfileLoaded(
+    const AccountId& account_id) {
+  OnLoginUserProfilePrepared(
+      chromeos::ProfileHelper::Get()->GetProfileByAccountId(account_id));
+}
+
 void SessionControllerClientImpl::OnCustodianInfoChanged() {
   DCHECK(supervised_user_profile_);
   User* user = chromeos::ProfileHelper::Get()->GetUserByProfile(
@@ -513,11 +502,6 @@ void SessionControllerClientImpl::Observe(
     case chrome::NOTIFICATION_APP_TERMINATING:
       session_controller_->NotifyChromeTerminating();
       break;
-    case chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED: {
-      Profile* profile = content::Details<Profile>(details).ptr();
-      OnLoginUserProfilePrepared(profile);
-      break;
-    }
     default:
       NOTREACHED() << "Unexpected notification " << type;
       break;
@@ -550,25 +534,13 @@ void SessionControllerClientImpl::OnLoginUserProfilePrepared(Profile* profile) {
                              session_info_changed_closure);
   pref_change_registrars_.push_back(std::move(pref_change_registrar));
 
-  // Needed because the user-to-profile mapping isn't available until later,
-  // which is needed in UserToUserSession().
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&SessionControllerClientImpl::SendUserSessionForProfile,
-                     weak_ptr_factory_.GetWeakPtr(), profile));
+  SendUserSession(*user);
 }
 
 void SessionControllerClientImpl::OnOffHoursEndTimeChanged() {
   SendSessionLengthLimit();
 }
 
-void SessionControllerClientImpl::SendUserSessionForProfile(Profile* profile) {
-  DCHECK(profile);
-  const User* user = chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
-  DCHECK(user);
-  SendUserSession(*user);
-}
-
 void SessionControllerClientImpl::SendSessionInfoIfChanged() {
   SessionManager* const session_manager = SessionManager::Get();
 
@@ -589,6 +561,14 @@ void SessionControllerClientImpl::SendSessionInfoIfChanged() {
 }
 
 void SessionControllerClientImpl::SendUserSession(const User& user) {
+  // Check user profile via GetProfileByUser() instead of is_profile_created()
+  // flag because many tests have only setup testing user profile in
+  // ProfileHelper but do not have the flag updated.
+  if (!chromeos::ProfileHelper::Get()->GetProfileByUser(&user)) {
+    pending_users_.insert(user.GetAccountId());
+    return;
+  }
+
   auto user_session = UserToUserSession(user);
 
   // Bail if the user has no session. Currently the only code path that hits
@@ -603,6 +583,12 @@ void SessionControllerClientImpl::SendUserSession(const User& user) {
 
   last_sent_user_session_ = std::move(user_session);
   session_controller_->UpdateUserSession(*last_sent_user_session_);
+
+  if (!pending_users_.empty()) {
+    pending_users_.erase(user.GetAccountId());
+    if (pending_users_.empty())
+      SendUserSessionOrder();
+  }
 }
 
 void SessionControllerClientImpl::SendUserSessionOrder() {

chrome/browser/ui/ash/session_controller_client_impl.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_ASH_SESSION_CONTROLLER_CLIENT_IMPL_H_
 
 #include <memory>
+#include <set>
 #include <vector>
 
 #include "ash/public/cpp/session/session_controller_client.h"
@@ -95,6 +96,7 @@ class SessionControllerClientImpl
 
   // session_manager::SessionManagerObserver:
   void OnSessionStateChanged() override;
+  void OnUserProfileLoaded(const AccountId& account_id) override;
 
   // SupervisedUserServiceObserver:
   void OnCustodianInfoChanged() override;
@@ -128,9 +130,6 @@ class SessionControllerClientImpl
   // Called when the login profile is ready.
   void OnLoginUserProfilePrepared(Profile* profile);
 
-  // Sends the user session info for a given profile.
-  void SendUserSessionForProfile(Profile* profile);
-
   // Sends session info to ash.
   void SendSessionInfoIfChanged();
 
@@ -152,8 +151,8 @@ class SessionControllerClientImpl
   // SessionController instance in ash.
   ash::SessionController* session_controller_ = nullptr;
 
-  // Whether the primary user session info is sent to ash.
-  bool primary_user_session_sent_ = false;
+  // Tracks users whose profiles are being loaded.
+  std::set<AccountId> pending_users_;
 
   // If the session is for a supervised user, the profile of that user.
   // Chrome OS only supports a single supervised user in a session.

chrome/browser/ui/ash/session_controller_client_impl_unittest.cc

@@ -144,12 +144,17 @@ class SessionControllerClientImplTest : public testing::Test {
 
   // Add and log in a user to the session.
   void UserAddedToSession(const AccountId& account_id) {
-    user_manager()->AddUser(account_id);
+    const user_manager::User* user = user_manager()->AddUser(account_id);
     session_manager_.CreateSession(
         account_id,
         chromeos::ProfileHelper::GetUserIdHashByUserIdForTesting(
             account_id.GetUserEmail()),
         false);
+
+    // Simulate that user profile is loaded.
+    CreateTestingProfile(user);
+    session_manager_.NotifyUserProfileLoaded(account_id);
+
     session_manager_.SetSessionState(SessionState::ACTIVE);
   }
 
@@ -462,12 +467,6 @@ TEST_F(SessionControllerClientImplTest, SupervisedUser) {
       chromeos::ProfileHelper::GetUserIdHashByUserIdForTesting(
           "child@test.com"),
       false);
-  session_manager_.SetSessionState(SessionState::ACTIVE);
-
-  // The session controller received session info and user session.
-  EXPECT_LT(0u, session_controller.last_user_session()->session_id);
-  EXPECT_EQ(user_manager::USER_TYPE_SUPERVISED,
-            session_controller.last_user_session()->user_info.type);
 
   // Simulate profile creation after login.
   TestingProfile* user_profile = CreateTestingProfile(user);
@@ -480,8 +479,15 @@ TEST_F(SessionControllerClientImplTest, SupervisedUser) {
                    "parent2@test.com");
 
   // Simulate the notification that the profile is ready.
-  client.OnLoginUserProfilePrepared(user_profile);
-  base::RunLoop().RunUntilIdle();  // For PostTask and mojo interface.
+  session_manager_.NotifyUserProfileLoaded(account_id);
+
+  // User session could only be made active after user profile is loaded.
+  session_manager_.SetSessionState(SessionState::ACTIVE);
+
+  // The session controller received session info and user session.
+  EXPECT_LT(0u, session_controller.last_user_session()->session_id);
+  EXPECT_EQ(user_manager::USER_TYPE_SUPERVISED,
+            session_controller.last_user_session()->user_info.type);
 
   // The custodians were sent over the mojo interface.
   EXPECT_EQ("parent1@test.com",
@@ -513,11 +519,13 @@ TEST_F(SessionControllerClientImplTest, UserPrefsChange) {
       chromeos::ProfileHelper::GetUserIdHashByUserIdForTesting(
           account_id.GetUserEmail()),
       false);
-  session_manager_.SetSessionState(SessionState::ACTIVE);
 
   // Simulate the notification that the profile is ready.
   TestingProfile* const user_profile = CreateTestingProfile(user);
-  client.OnLoginUserProfilePrepared(user_profile);
+  session_manager_.NotifyUserProfileLoaded(account_id);
+
+  // User session could only be made active after user profile is loaded.
+  session_manager_.SetSessionState(SessionState::ACTIVE);
 
   // Manipulate user prefs and verify SessionController is updated.
   PrefService* const user_prefs = user_profile->GetPrefs();