Remove NOTIFICATION_SESSION_STARTED from AccessibilityManager

Previously, this code used NOTIFICATION_SESSION_STARTED for initial
login, and then UserManager::SessionStateObserver::ActiveUserChanged
for swapping between users. It should be possible just to always
rely on ActiveUserChanged. If the profile isn't loaded yet (as with
initial login), add a profile created observer.

This should mean SetProfile gets called slightly earlier as
SESSION_STARTED comes rather late. This seems like an improvement
because SetProfile will not be delayed until after the post-login
steps (e.g. when the profile picture is being chosen).

ChromeUserManager is changed slightly, listening to
NOTIFICATION_PROFILE_ADDED rather than NOTIFICATION_PROFILE_CREATED.
For async profile creation, NOTIFICATION_PROFILE_ADDED comes after
NOTIFICATION_PROFILE_CREATED, but for synchronous profile creation
(e.g. the startup profile), it's inverted; in both cases the two
notifications are sent synchronously, i.e. as part of the same
task. To make sure the profile is both initialized and
registered with ProfileManager by the time ProfileCreatedObservers
get notified, ChromeUserManager should make use of
NOTIFICATION_PROFILE_ADDED.

Bug: 268984
Change-Id: I53c1793f2a14642fb1e4cd32066e2a57eeebd1a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1769227
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692231}

chrome/browser/chromeos/accessibility/accessibility_manager.cc

@@ -77,7 +77,6 @@
 #include "content/public/browser/tts_controller.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/common/content_switches.h"
-#include "extensions/browser/extension_registry.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_messages.h"
@@ -238,26 +237,10 @@ void AccessibilityManager::ShowAccessibilityHelp(Browser* browser) {
   ShowSingletonTab(browser, GURL(chrome::kChromeAccessibilityHelpURL));
 }
 
-AccessibilityManager::AccessibilityManager()
-    : profile_(NULL),
-      spoken_feedback_enabled_(false),
-      select_to_speak_enabled_(false),
-      switch_access_enabled_(false),
-      autoclick_enabled_(false),
-      braille_display_connected_(false),
-      scoped_braille_observer_(this),
-      braille_ime_current_(false),
-      chromevox_panel_(nullptr),
-      switch_access_panel_(nullptr),
-      extension_registry_observer_(this) {
+AccessibilityManager::AccessibilityManager() {
   notification_registrar_.Add(this,
                               chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
                               content::NotificationService::AllSources());
-  notification_registrar_.Add(this,
-                              chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED,
-                              content::NotificationService::AllSources());
-  notification_registrar_.Add(this, chrome::NOTIFICATION_SESSION_STARTED,
-                              content::NotificationService::AllSources());
   notification_registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                               content::NotificationService::AllSources());
   notification_registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
@@ -265,6 +248,7 @@ AccessibilityManager::AccessibilityManager()
   notification_registrar_.Add(this, content::NOTIFICATION_FOCUS_CHANGED_IN_PAGE,
                               content::NotificationService::AllSources());
   input_method::InputMethodManager::Get()->AddObserver(this);
+  user_manager::UserManager::Get()->AddSessionStateObserver(this);
 
   ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
   audio::SoundsManager* manager = audio::SoundsManager::Get();
@@ -347,6 +331,7 @@ AccessibilityManager::~AccessibilityManager() {
                                           false);
   NotifyAccessibilityStatusChanged(details);
   CrasAudioHandler::Get()->RemoveAudioObserver(this);
+  user_manager::UserManager::Get()->RemoveSessionStateObserver(this);
   input_method::InputMethodManager::Get()->RemoveObserver(this);
 
   if (chromevox_panel_) {
@@ -1067,10 +1052,6 @@ void AccessibilityManager::OnActiveOutputNodeChanged() {
 }
 
 void AccessibilityManager::SetProfile(Profile* profile) {
-  // Do nothing if this is called for the current profile. This can happen. For
-  // example, ChromeSessionManager fires both
-  // NOTIFICATION_LOGIN_USER_PROFILE_PREPARED and NOTIFICATION_SESSION_STARTED,
-  // and we are observing both events.
   if (profile_ == profile)
     return;
 
@@ -1178,10 +1159,28 @@ void AccessibilityManager::SetProfile(Profile* profile) {
   OnAutoclickChanged();
 }
 
+void AccessibilityManager::SetProfileByUser(const user_manager::User* user) {
+  Profile* profile = ProfileHelper::Get()->GetProfileByUser(user);
+  DCHECK(profile);
+  SetProfile(profile);
+}
+
 void AccessibilityManager::ActiveUserChanged(
     const user_manager::User* active_user) {
-  if (active_user && active_user->is_profile_created())
-    SetProfile(ProfileManager::GetActiveUserProfile());
+  if (!active_user)
+    return;
+
+  if (active_user->is_profile_created()) {
+    SetProfileByUser(active_user);
+  } else {
+    // |active_user| is unfortunately const.
+    user_manager::User* user =
+        user_manager::UserManager::Get()->GetActiveUser();
+    DCHECK_EQ(user, active_user);
+    user->AddProfileCreatedObserver(
+        base::BindOnce(&AccessibilityManager::SetProfileByUser,
+                       weak_ptr_factory_.GetWeakPtr(), user));
+  }
 }
 
 base::TimeDelta AccessibilityManager::PlayShutdownSound() {
@@ -1287,26 +1286,11 @@ void AccessibilityManager::Observe(
         SetProfile(profile);
       break;
     }
-    case chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED:
-      // Update |profile_| when login user profile is prepared.
-      // NOTIFICATION_SESSION_STARTED is not fired from UserSessionManager, but
-      // profile may be changed by UserSessionManager in OOBE flow.
-      SetProfile(ProfileManager::GetActiveUserProfile());
-      break;
-    case chrome::NOTIFICATION_SESSION_STARTED:
-      // Update |profile_| when entering a session.
-      SetProfile(ProfileManager::GetActiveUserProfile());
-
-      // Add a session state observer to be able to monitor session changes.
-      if (!session_state_observer_.get())
-        session_state_observer_.reset(
-            new user_manager::ScopedUserSessionStateObserver(this));
-      break;
     case chrome::NOTIFICATION_PROFILE_DESTROYED: {
       // Update |profile_| when exiting a session or shutting down.
       Profile* profile = content::Source<Profile>(source).ptr();
       if (profile_ == profile)
-        SetProfile(NULL);
+        SetProfile(nullptr);
       break;
     }
     case chrome::NOTIFICATION_APP_TERMINATING: {

chrome/browser/chromeos/accessibility/accessibility_manager.h

@@ -26,6 +26,8 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "extensions/browser/event_router.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_registry_observer.h"
 #include "extensions/browser/extension_system.h"
 #include "services/media_session/public/mojom/audio_focus.mojom.h"
 #include "ui/accessibility/ax_enums.mojom-forward.h"
@@ -393,6 +395,8 @@ class AccessibilityManager
 
   void SetProfile(Profile* profile);
 
+  void SetProfileByUser(const user_manager::User* user);
+
   void UpdateChromeOSAccessibilityHistograms();
 
   void PlayVolumeAdjustSound();
@@ -425,43 +429,41 @@ class AccessibilityManager
   void OnActiveOutputNodeChanged() override;
 
   // Profile which has the current a11y context.
-  Profile* profile_;
+  Profile* profile_ = nullptr;
 
   content::NotificationRegistrar notification_registrar_;
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
   std::unique_ptr<PrefChangeRegistrar> local_state_pref_change_registrar_;
-  std::unique_ptr<user_manager::ScopedUserSessionStateObserver>
-      session_state_observer_;
 
-  bool spoken_feedback_enabled_;
-  bool select_to_speak_enabled_;
-  bool switch_access_enabled_;
-  bool autoclick_enabled_;
+  bool spoken_feedback_enabled_ = false;
+  bool select_to_speak_enabled_ = false;
+  bool switch_access_enabled_ = false;
+  bool autoclick_enabled_ = false;
 
   AccessibilityStatusCallbackList callback_list_;
 
-  bool braille_display_connected_;
+  bool braille_display_connected_ = false;
   ScopedObserver<extensions::api::braille_display_private::BrailleController,
-                 AccessibilityManager>
-      scoped_braille_observer_;
+                 extensions::api::braille_display_private::BrailleObserver>
+      scoped_braille_observer_{this};
 
-  bool braille_ime_current_;
+  bool braille_ime_current_ = false;
 
-  ChromeVoxPanel* chromevox_panel_;
+  ChromeVoxPanel* chromevox_panel_ = nullptr;
   std::unique_ptr<AccessibilityPanelWidgetObserver>
       chromevox_panel_widget_observer_;
 
-  SwitchAccessPanel* switch_access_panel_;
+  SwitchAccessPanel* switch_access_panel_ = nullptr;
   std::unique_ptr<AccessibilityPanelWidgetObserver>
       switch_access_panel_widget_observer_;
 
   std::string keyboard_listener_extension_id_;
-  bool keyboard_listener_capture_;
+  bool keyboard_listener_capture_ = false;
 
   // Listen to extension unloaded notifications.
   ScopedObserver<extensions::ExtensionRegistry,
                  extensions::ExtensionRegistryObserver>
-      extension_registry_observer_;
+      extension_registry_observer_{this};
 
   std::unique_ptr<AccessibilityExtensionLoader> autoclick_extension_loader_;
 

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -371,7 +371,7 @@ ChromeUserManagerImpl::ChromeUserManagerImpl()
 
   registrar_.Add(this, chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED,
                  content::NotificationService::AllSources());
-  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
+  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED,
                  content::NotificationService::AllSources());
 
   // Since we're in ctor postpone any actions till this is fully created.
@@ -650,7 +650,7 @@ void ChromeUserManagerImpl::Observe(
       UpdateUserTimeZoneRefresher(profile);
       break;
     }
-    case chrome::NOTIFICATION_PROFILE_CREATED: {
+    case chrome::NOTIFICATION_PROFILE_ADDED: {
       Profile* profile = content::Source<Profile>(source).ptr();
       user_manager::User* user =
           ProfileHelper::Get()->GetUserByProfile(profile);
@@ -663,14 +663,7 @@ void ChromeUserManagerImpl::Observe(
 
       // If there is pending user switch, do it now.
       if (GetPendingUserSwitchID().is_valid()) {
-        // Call SwitchActiveUser async because otherwise it may cause
-        // ProfileManager::GetProfile before the profile gets registered
-        // in ProfileManager. It happens in case of sync profile load when
-        // NOTIFICATION_PROFILE_CREATED is called synchronously.
-        base::ThreadTaskRunnerHandle::Get()->PostTask(
-            FROM_HERE, base::BindOnce(&ChromeUserManagerImpl::SwitchActiveUser,
-                                      weak_factory_.GetWeakPtr(),
-                                      GetPendingUserSwitchID()));
+        SwitchActiveUser(GetPendingUserSwitchID());
         SetPendingUserSwitchId(EmptyAccountId());
       }
       break;

chrome/browser/profiles/profile_manager.cc

@@ -1119,10 +1119,9 @@ void ProfileManager::OnProfileCreated(Profile* profile,
 
   // Perform initialization.
   if (success) {
-    DoFinalInit(profile, go_off_the_record);
+    DoFinalInit(info, go_off_the_record);
     if (go_off_the_record)
       profile = profile->GetOffTheRecordProfile();
-    info->created = true;
   } else {
     profile = NULL;
     profiles_info_.erase(iter);
@@ -1145,13 +1144,33 @@ void ProfileManager::OnProfileCreated(Profile* profile,
                          Profile::CREATE_STATUS_LOCAL_FAIL);
 }
 
-void ProfileManager::DoFinalInit(Profile* profile, bool go_off_the_record) {
+Profile* ProfileManager::CreateProfileHelper(const base::FilePath& path) {
+  TRACE_EVENT0("browser", "ProfileManager::CreateProfileHelper");
+
+  return Profile::CreateProfile(path, nullptr, Profile::CREATE_MODE_SYNCHRONOUS)
+      .release();
+}
+
+std::unique_ptr<Profile> ProfileManager::CreateProfileAsyncHelper(
+    const base::FilePath& path,
+    Delegate* delegate) {
+  return Profile::CreateProfile(path, delegate,
+                                Profile::CREATE_MODE_ASYNCHRONOUS);
+}
+
+void ProfileManager::DoFinalInit(ProfileInfo* profile_info,
+                                 bool go_off_the_record) {
   TRACE_EVENT0("browser", "ProfileManager::DoFinalInit");
 
+  Profile* profile = profile_info->profile.get();
   DoFinalInitForServices(profile, go_off_the_record);
   AddProfileToStorage(profile);
   DoFinalInitLogging(profile);
 
+  // Set the |created| flag now so that PROFILE_ADDED handlers can use
+  // GetProfileByPath().
+  profile_info->created = true;
+
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_PROFILE_ADDED,
       content::Source<Profile>(profile),
@@ -1166,6 +1185,9 @@ void ProfileManager::DoFinalInit(Profile* profile, bool go_off_the_record) {
 
 void ProfileManager::DoFinalInitForServices(Profile* profile,
                                             bool go_off_the_record) {
+  if (!do_final_services_init_)
+    return;
+
   TRACE_EVENT0("browser", "ProfileManager::DoFinalInitForServices");
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -1264,6 +1286,9 @@ void ProfileManager::DoFinalInitForServices(Profile* profile,
 }
 
 void ProfileManager::DoFinalInitLogging(Profile* profile) {
+  if (!do_final_services_init_)
+    return;
+
   TRACE_EVENT0("browser", "ProfileManager::DoFinalInitLogging");
   // Count number of extensions in this profile.
   int enabled_app_count = -1;
@@ -1280,20 +1305,6 @@ void ProfileManager::DoFinalInitLogging(Profile* profile) {
       base::TimeDelta::FromSeconds(112));
 }
 
-Profile* ProfileManager::CreateProfileHelper(const base::FilePath& path) {
-  TRACE_EVENT0("browser", "ProfileManager::CreateProfileHelper");
-
-  return Profile::CreateProfile(path, NULL, Profile::CREATE_MODE_SYNCHRONOUS)
-      .release();
-}
-
-std::unique_ptr<Profile> ProfileManager::CreateProfileAsyncHelper(
-    const base::FilePath& path,
-    Delegate* delegate) {
-  return Profile::CreateProfile(path, delegate,
-                                Profile::CREATE_MODE_ASYNCHRONOUS);
-}
-
 ProfileManager::ProfileInfo::ProfileInfo(std::unique_ptr<Profile> profile,
                                          bool created)
     : profile(std::move(profile)), created(created) {}
@@ -1343,20 +1354,19 @@ bool ProfileManager::AddProfile(std::unique_ptr<Profile> profile) {
   TRACE_EVENT0("browser", "ProfileManager::AddProfile");
 
   DCHECK(profile);
-  Profile* profile_ptr = profile.get();
 
   // Make sure that we're not loading a profile with the same ID as a profile
   // that's already loaded.
-  if (GetProfileByPathInternal(profile_ptr->GetPath())) {
+  if (GetProfileByPathInternal(profile->GetPath())) {
     NOTREACHED() << "Attempted to add profile with the same path ("
-                 << profile_ptr->GetPath().value()
+                 << profile->GetPath().value()
                  << ") as an already-loaded profile.";
     return false;
   }
 
-  RegisterProfile(std::move(profile), true);
-  InitProfileUserPrefs(profile_ptr);
-  DoFinalInit(profile_ptr, ShouldGoOffTheRecord(profile_ptr));
+  ProfileInfo* profile_info = RegisterProfile(std::move(profile), true);
+  InitProfileUserPrefs(profile_info->profile.get());
+  DoFinalInit(profile_info, ShouldGoOffTheRecord(profile_info->profile.get()));
   return true;
 }
 
@@ -1854,4 +1864,5 @@ void ProfileManager::ScheduleForcedEphemeralProfileForDeletion(
 
 ProfileManagerWithoutInit::ProfileManagerWithoutInit(
     const base::FilePath& user_data_dir) : ProfileManager(user_data_dir) {
+  set_do_final_services_init(false);
 }

chrome/browser/profiles/profile_manager.h

@@ -246,11 +246,6 @@ class ProfileManager : public content::NotificationObserver,
                         bool is_new_profile) override;
 
  protected:
-  // Does final initial actions.
-  virtual void DoFinalInit(Profile* profile, bool go_off_the_record);
-  virtual void DoFinalInitForServices(Profile* profile, bool go_off_the_record);
-  virtual void DoFinalInitLogging(Profile* profile);
-
   // Creates a new profile by calling into the profile's profile creation
   // method. Virtual so that unittests can return a TestingProfile instead
   // of the Profile's result. Returns null if creation fails.
@@ -263,6 +258,10 @@ class ProfileManager : public content::NotificationObserver,
       const base::FilePath& path,
       Delegate* delegate);
 
+  void set_do_final_services_init(bool do_final_services_init) {
+    do_final_services_init_ = do_final_services_init;
+  }
+
  private:
   friend class TestingProfileManager;
   FRIEND_TEST_ALL_PREFIXES(ProfileManagerBrowserTest, DeleteAllProfiles);
@@ -286,6 +285,11 @@ class ProfileManager : public content::NotificationObserver,
     DISALLOW_COPY_AND_ASSIGN(ProfileInfo);
   };
 
+  // Does final initial actions.
+  void DoFinalInit(ProfileInfo* profile_info, bool go_off_the_record);
+  void DoFinalInitForServices(Profile* profile, bool go_off_the_record);
+  void DoFinalInitLogging(Profile* profile);
+
   // Returns the profile of the active user and / or the off the record profile
   // if needed. This adds the profile to the ProfileManager if it doesn't
   // already exist. The method will return NULL if the profile doesn't exist
@@ -438,6 +442,9 @@ class ProfileManager : public content::NotificationObserver,
   std::vector<Profile*> active_profiles_;
   bool closing_all_browsers_ = false;
 
+  // Controls whether to initialize some services. Only disabled for testing.
+  bool do_final_services_init_ = true;
+
   // TODO(chrome/browser/profiles/OWNERS): Usage of this in profile_manager.cc
   // should likely be turned into DCHECK_CURRENTLY_ON(BrowserThread::UI) for
   // consistency with surrounding code in the same file but that wasn't trivial
@@ -454,9 +461,8 @@ class ProfileManagerWithoutInit : public ProfileManager {
  public:
   explicit ProfileManagerWithoutInit(const base::FilePath& user_data_dir);
 
- protected:
-  void DoFinalInitForServices(Profile*, bool) override {}
-  void DoFinalInitLogging(Profile*) override {}
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ProfileManagerWithoutInit);
 };
 
 #endif  // CHROME_BROWSER_PROFILES_PROFILE_MANAGER_H_

chrome/browser/ui/app_list/app_list_client_impl.cc

@@ -297,6 +297,7 @@ void AppListClientImpl::ActiveUserChanged(
     const user_manager::User* active_user) {
   if (!active_user->is_profile_created())
     return;
+
   UpdateProfile();
 }
 
@@ -304,8 +305,9 @@ void AppListClientImpl::UpdateProfile() {
   Profile* profile = ProfileManager::GetActiveUserProfile();
   app_list::AppListSyncableService* syncable_service =
       app_list::AppListSyncableServiceFactory::GetForProfile(profile);
-  DCHECK(syncable_service);
-  SetProfile(profile);
+  // AppListSyncableService is null in tests.
+  if (syncable_service)
+    SetProfile(profile);
 }
 
 void AppListClientImpl::SetProfile(Profile* new_profile) {

chrome/browser/ui/app_list/app_list_client_impl_browsertest.cc

@@ -24,7 +24,6 @@
 #include "chrome/browser/apps/platform_apps/app_browsertest_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/demo_mode/demo_session.h"
-#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -33,7 +32,6 @@
 #include "chrome/browser/ui/app_list/app_list_client_impl.h"
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/app_list_model_updater.h"
-#include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
 #include "chrome/browser/ui/app_list/chrome_app_list_item.h"
 #include "chrome/browser/ui/app_list/search/search_controller.h"
 #include "chrome/browser/ui/app_list/test/chrome_app_list_test_support.h"
@@ -48,7 +46,6 @@
 #include "chromeos/constants/chromeos_switches.h"
 #include "components/browser_sync/browser_sync_switches.h"
 #include "components/prefs/pref_service.h"
-#include "components/user_manager/scoped_user_manager.h"
 #include "components/user_manager/user_names.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"