Loading the Launcher with a Locked Profile launched UserManager.

chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc

Index: chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
diff --git a/chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc b/chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
index f8690d71d2cfab2defa6a590b125ddb1b68890ef..5751771c739d9d46cd554e86860770b6f3a7c1c1 100644
--- a/chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
+++ b/chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
@@ -12,17 +12,21 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/test/chrome_app_list_test_support.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
+#include "chrome/browser/ui/user_manager.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile_manager.h"
 #include "content/public/test/test_utils.h"
 #include "ui/app_list/app_list_model.h"
 #include "ui/app_list/search_box_model.h"
@@ -39,6 +43,9 @@ class AppListServiceInteractiveTest : public InProcessBrowserTest {
 
  protected:
   Profile* profile2_;
+  ProfileInfoCache* profile_info_cache() {
+    return &(g_browser_process->profile_manager()->GetProfileInfoCache());
+  }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AppListServiceInteractiveTest);
@@ -49,6 +56,7 @@ class AppListServiceInteractiveTest : public InProcessBrowserTest {
 #if defined(OS_CHROMEOS)
 #define MAYBE_ShowAndDismiss DISABLED_ShowAndDismiss
 #define MAYBE_SwitchAppListProfiles DISABLED_SwitchAppListProfiles
+#define MAYBE_SwitchAppListLockedProfile DISABLED_SwitchAppListLockedProfile
 #define MAYBE_SwitchAppListProfilesDuringSearch \
     DISABLED_SwitchAppListProfilesDuringSearch
 #define MAYBE_ShowAppListNonDefaultProfile \
@@ -57,6 +65,7 @@ class AppListServiceInteractiveTest : public InProcessBrowserTest {
 #else
 #define MAYBE_ShowAndDismiss ShowAndDismiss
 #define MAYBE_SwitchAppListProfiles SwitchAppListProfiles
+#define MAYBE_SwitchAppListLockedProfile SwitchAppListLockedProfile
 #define MAYBE_SwitchAppListProfilesDuringSearch \
     SwitchAppListProfilesDuringSearch
 #define MAYBE_ShowAppListNonDefaultProfile ShowAppListNonDefaultProfile
@@ -107,6 +116,48 @@ IN_PROC_BROWSER_TEST_F(AppListServiceInteractiveTest,
   controller->DismissView();
 }
 
+// Switch profiles on the app list while it is showing.
+IN_PROC_BROWSER_TEST_F(AppListServiceInteractiveTest,
+                       MAYBE_SwitchAppListLockedProfile) {
+  InitSecondProfile();
+
+  AppListService* service = test::GetAppListService();
+  ASSERT_TRUE(service);
+
+  AppListControllerDelegate* controller(service->GetControllerDelegate());
+  ASSERT_TRUE(controller);
+
+  // Open the app list with the browser's profile.
+  ASSERT_FALSE(service->IsAppListVisible());

[tapted, 2015/01/21 22:47:20]

nit: ASSERT -> EXPECT (here and all following)

I think the surrounding tests have ASSERT rather than EXPECT because the tests
could historically get in a weird state / timeout when they would unexpectedly
flake. But they're stable now and I think the preference would be to have this
ASSERT and all the ones following be EXPECT. (the ASSERTs above are good though
- null pointer dereferences tend not to give nice stackdumps in the bots)

[Mike Lerman, 2015/01/23 16:58:33]

Done - all the NULL tests are ASSERTs and all the method / boolean checks are
EXPECTs.

+  controller->ShowForProfileByPath(browser()->profile()->GetPath());
+  app_list::AppListModel* model = test::GetAppListModel(service);
+  ASSERT_TRUE(model);
+
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_TRUE(service->IsAppListVisible());
+  ASSERT_EQ(browser()->profile(), service->GetCurrentAppListProfile());
+
+  // Lock the second profile.
+  profile_info_cache()->SetProfileSigninRequiredAtIndex(
+      profile_info_cache()->GetIndexOfProfileWithPath(profile2_->GetPath()),
+      true);
+
+  // Attempt to open the app list with the second profile.
+  controller->ShowForProfileByPath(profile2_->GetPath());
+  model = test::GetAppListModel(service);
+  ASSERT_TRUE(model);

[tapted, 2015/01/21 22:47:20]

note I think this would mean that the app launcher is still configured for the
initial/default profile.

It's subtle, so perhaps a check to that effect, like 

EXPECT_TRUE(service->GetCurrentAppListProfile());
EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());


Maybe there are corner cases for locked profiles where this would be a problem,
but that's orthogonal to this CL (hint: It's tricky - but stuff in
AppListServiceImpl::OnProfileWillBeRemoved() is probably a lot like what you
might want).

[Mike Lerman, 2015/01/23 16:58:33]

Ah, too much blind copy-pasting. Thanks!

[tapted, 2015/01/27 02:52:54]

Sorry, I meant that if that expectation is true it would mean that the app
launcher is still configured for the initial/default profile. (i.e. deleting the
expectation in the test won't change that).

My review comments actually first said "remove `app_list::AppListModel* model`
entirely since it's not really used". However, the fact that it is still
non-NULL here is "interesting", since it means the profile switch failed. I
think that's a valid test result, and it would be good to keep this in to check
for regressions. (but with the extra lines and a comment so it's clear to a
later reader what's going on).

It might be the case that, in a different context, having a non-NULL model here
is *not* a valid test result. e.g. locking the *current* profile rather than
switching *to* a locked profile. We can worry about that context in a follow-up
(if we need to at all), but it would be good to capture what's going on
currently.

[Mike Lerman, 2015/01/27 14:59:49]

You mention that the fact that it's not-NULL is "interesting", but I don't see
how it means that the ProfileSwitch failed. For an AppListService, there doesn't
seem to be a mechanism for removing the model, no matter what happens. Perhaps
that's worth noting, so have done so, but leave to your discretion if I should
change the comment, if I'm reading the code wrong.

[tapted, 2015/01/27 22:55:01]

`AppListServiceImpl::OnProfileWillBeRemoved()` has a mechanism by which the
model is removed, and all references to the Profile* are removed from
AppListFoo*

by "the profile switch failed" I mean it failed to load the locked profile. In
this test, it means the app list is a hidden window still loaded with the
original profile that InProcessBrowserTest made, and isn't locked. Since there
was no successful Show, the app list will still be configured to use that
profile if it is shown again by a mechanism like AppListService::Show() rather
than AppListService::ShowForProfile()

+  base::RunLoop().RunUntilIdle();
+
+  // App list stays hidden; the UserManager shows instead.
+  ASSERT_FALSE(service->IsAppListVisible());
+  ASSERT_TRUE(UserManager::IsShowing());
+
+  controller->DismissView();
+  // We need to hide the User Manager or else the process can't die.
+  UserManager::Hide();
+}
+
 // Test switching app list profiles while search results are visibile.
 IN_PROC_BROWSER_TEST_F(AppListServiceInteractiveTest,
                        MAYBE_SwitchAppListProfilesDuringSearch) {