Access to Chrome via the System Tray should go through the User Manager.

chrome/browser/ui/webui/signin/user_manager_screen_handler.cc

 // Copyright 2013 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/ui/webui/signin/user_manager_screen_handler.h"
 
 #include "base/bind.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/value_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_info_cache_observer.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_metrics.h"
 #include "chrome/browser/profiles/profile_window.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/signin/local_auth.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "google_apis/gaia/gaia_auth_fetcher.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ skipping 42 matching lines @@
   if (status != Profile::CREATE_STATUS_INITIALIZED)
     return;
   profiles::FindOrCreateNewWindowForProfile(
     profile,
     chrome::startup::IS_PROCESS_STARTUP,
     chrome::startup::IS_FIRST_RUN,
     desktop_type,
     false);
 }
 
-// This callback is run after switching to a new profile has finished. This
-// means either a new browser window has been opened, or an existing one
-// has been found, which means we can safely close the User Manager without
-// accidentally terminating the browser process. The task needs to be posted,
-// as HideUserManager will end up destroying its WebContents, which will
-// destruct the UserManagerScreenHandler as well.
-void OnSwitchToProfileComplete() {
-  base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&chrome::HideUserManager));
-}
-
 std::string GetAvatarImageAtIndex(
     size_t index, const ProfileInfoCache& info_cache) {
   bool is_gaia_picture =
       info_cache.IsUsingGAIAPictureOfProfileAtIndex(index) &&
       info_cache.GetGAIAPictureOfProfileAtIndex(index);
 
   // If the avatar is too small (i.e. the old-style low resolution avatar),
   // it will be pixelated when displayed in the User Manager, so we should
   // return the placeholder avatar instead.
   gfx::Image avatar_image = info_cache.GetAvatarIconOfProfileAtIndex(index);
@@ skipping 171 matching lines @@
       g_browser_process->profile_manager()->GetProfileInfoCache();
   const size_t profile_index = GetIndexOfProfileWithEmailAndName(
       info_cache, base::UTF8ToUTF16(user_email), base::string16());
   DCHECK_LT(profile_index, info_cache.GetNumberOfProfiles());
 
   authenticating_profile_index_ = profile_index;
   ReportAuthenticationResult(true, ProfileMetrics::AUTH_LOCAL);
 }
 
 void UserManagerScreenHandler::HandleInitialize(const base::ListValue* args) {
+  // If the URL has a hash parameter, store it for later.
+  base::string16 url_hash;
+  if (args->GetString(0, &url_hash))
+    url_hash_ = url_hash;
+
   SendUserList();
   web_ui()->CallJavascriptFunction("cr.ui.Oobe.showUserManagerScreen",
       base::FundamentalValue(IsGuestModeEnabled()));
   desktop_type_ = chrome::GetHostDesktopTypeForNativeView(
       web_ui()->GetWebContents()->GetNativeView());
 
   ScreenlockBridge::Get()->SetLockHandler(this);
 }
 
 void UserManagerScreenHandler::HandleAddUser(const base::ListValue* args) {
-  profiles::CreateAndSwitchToNewProfile(desktop_type_,
-                                        base::Bind(&OnSwitchToProfileComplete),
-                                        ProfileMetrics::ADD_NEW_USER_MANAGER);
+  profiles::CreateAndSwitchToNewProfile(
+      desktop_type_,
+      base::Bind(&UserManagerScreenHandler::OnSwitchToProfileComplete,
+                 base::Unretained(this)),
+      ProfileMetrics::ADD_NEW_USER_MANAGER);
 }
 
 void UserManagerScreenHandler::HandleAuthenticatedLaunchUser(
     const base::ListValue* args) {
   base::string16 email_address;
   if (!args->GetString(0, &email_address))
     return;
 
   base::string16 display_name;
   if (!args->GetString(1, &display_name))
@@ skipping 58 matching lines @@
 
   g_browser_process->profile_manager()->ScheduleProfileForDeletion(
       profile_path,
       base::Bind(&OpenNewWindowForProfile, desktop_type_));
   ProfileMetrics::LogProfileDeleteUser(
       ProfileMetrics::DELETE_PROFILE_USER_MANAGER);
 }
 
 void UserManagerScreenHandler::HandleLaunchGuest(const base::ListValue* args) {
   if (IsGuestModeEnabled()) {
-    profiles::SwitchToGuestProfile(desktop_type_,
-                                   base::Bind(&OnSwitchToProfileComplete));
+    profiles::SwitchToGuestProfile(
+        desktop_type_,
+        base::Bind(&UserManagerScreenHandler::OnSwitchToProfileComplete,
+                   base::Unretained(this)));
     ProfileMetrics::LogProfileSwitchUser(ProfileMetrics::SWITCH_PROFILE_GUEST);
   } else {
     // The UI should have prevented the user from allowing the selection of
     // guest mode.
     NOTREACHED();
   }
 }
 
 void UserManagerScreenHandler::HandleLaunchUser(const base::ListValue* args) {
   base::string16 email_address;
@@ skipping 18 matching lines @@
   // It's possible that a user breaks into the user-manager page using the
   // JavaScript Inspector and causes a "locked" profile to call this
   // unauthenticated version of "launch" instead of the proper one.  Thus,
   // we have to validate in (secure) C++ code that it really is a profile
   // not needing authentication.  If it is, just ignore the "launch" request.
   if (info_cache.ProfileIsSigninRequiredAtIndex(profile_index))
     return;
   ProfileMetrics::LogProfileAuthResult(ProfileMetrics::AUTH_UNNECESSARY);
 
   base::FilePath path = info_cache.GetPathOfProfileAtIndex(profile_index);
-  profiles::SwitchToProfile(path,
-                            desktop_type_,
-                            false,  /* reuse any existing windows */
-                            base::Bind(&OnSwitchToProfileComplete),
-                            ProfileMetrics::SWITCH_PROFILE_MANAGER);
+  profiles::SwitchToProfile(
+      path,
+      desktop_type_,
+      false,  /* reuse any existing windows */
+      base::Bind(&UserManagerScreenHandler::OnSwitchToProfileComplete,
+                 base::Unretained(this)),
+      ProfileMetrics::SWITCH_PROFILE_MANAGER);
 }
 
 void UserManagerScreenHandler::HandleAttemptUnlock(
     const base::ListValue* args) {
   std::string email;
   CHECK(args->GetString(0, &email));
   GetScreenlockRouter(email)->OnAuthAttempted(GetAuthType(email), "");
 }
 
 void UserManagerScreenHandler::HandleHardlockUserPod(
@@ skipping 223 matching lines @@
   ProfileMetrics::LogProfileAuthResult(auth);
   password_attempt_.clear();
 
   if (success) {
     ProfileInfoCache& info_cache =
         g_browser_process->profile_manager()->GetProfileInfoCache();
     info_cache.SetProfileSigninRequiredAtIndex(
         authenticating_profile_index_, false);
     base::FilePath path = info_cache.GetPathOfProfileAtIndex(
         authenticating_profile_index_);
-    profiles::SwitchToProfile(path, desktop_type_, true,
-                              base::Bind(&OnSwitchToProfileComplete),
-                              ProfileMetrics::SWITCH_PROFILE_UNLOCK);
+    profiles::SwitchToProfile(
+        path,
+        desktop_type_,
+        true,
+        base::Bind(&UserManagerScreenHandler::OnSwitchToProfileComplete,
+                   base::Unretained(this)),

[Evan Stade, 2014/09/11 17:02:51]

I don't think unretained is right here. Are you guaranteed that this
UserManagerScreenHandler will outlive whatever is doing the profile switching? I
think not (unless this callback is run synchronously)... you probably need a
weak ptr.

[Mike Lerman, 2014/09/11 18:49:29]

The User Manager will certainly always wait around until the profiles:: code
executes (synchronously or otherwise). If not, Chrome would shut itself down
(especially on Windows) as there'd be no active Profiles or Browsers. 

noms@ can confirm this.

[Evan Stade, 2014/09/11 19:02:52]

During shutdown is the time I'd be most worried about this. Anyway, this isn't
really a question of "expected behavior", it's a question of object ownership.
If |this| mysteriously goes away, this callback needs to know about it. You need
a weak ptr.

[noms (inactive), 2014/09/11 19:05:41]

I think we could get in a weird situation where you can start switching to a
profile and provided that takes super long, or you are super fast, you could
then manually close the UserManager. That would maybe open a bit of a hellmouth.
Just to be on the safe side, I would agree with Evan and make this a weak ptr.

On 2014/09/11 18:49:29, Mike Lerman wrote:

[Mike Lerman, 2014/09/11 20:51:51]

Done.

+        ProfileMetrics::SWITCH_PROFILE_UNLOCK);
   } else {
     web_ui()->CallJavascriptFunction(
         "cr.ui.Oobe.showSignInError",
         base::FundamentalValue(0),
         base::StringValue(
             l10n_util::GetStringUTF8(IDS_LOGIN_ERROR_AUTHENTICATING)),
         base::StringValue(""),
         base::FundamentalValue(0));
   }
 }
+
+// This callback is run after switching to a new profile has finished. This
+// means either a new browser window has been opened, or an existing one
+// has been found, which means we can safely close the User Manager without
+// accidentally terminating the browser process. The task needs to be posted,
+// as HideUserManager will end up destroying its WebContents, which will
+// destruct the UserManagerScreenHandler as well.

[Evan Stade, 2014/09/11 17:02:50]

I don't quite understand. HideUserManager might destroy |this|, but it's
delayed. So you should be able to finish out this function before being
destroyed, hence you don't need to post on L702 or L710.

I don't even know why you're posting HideUserManager. Why not just put it at the
end of this function?

[Mike Lerman, 2014/09/11 18:49:29]

More in depth explanation is here, where the code was implemented this way:
https://codereview.chromium.org/149883004

This method (OnSwitchToProfileComplete) can be called synchronously (see line
380 above as an example) within this object, meaning this object needs to stick
around a bit longer.

[Evan Stade, 2014/09/11 19:02:52]

I don't see how L380 or any use is synchronous?

Even if |this| is destroyed, you just have to make sure not to reference |this|
afterwards. i.e. calling OnSwitchToProfileComplete synchronously would be OK as
long as it's the last thing you do in that method.

[Mike Lerman, 2014/09/11 20:51:51]

L380 can execute the following chain of calls synchronously:
SwitchToGuestProfile() -> CreateProfileAsync() -> If the ProfileInfo exists
(profile already existed), execute the Callback directly.

HideUserManager needs to be posted because the new browser's window hasn't been
created yet, and Chrome (racily) can close if there are no windows open.

+void UserManagerScreenHandler::OnSwitchToProfileComplete(
+    Profile* profile, Profile::CreateStatus profile_create_status) {
+  base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&chrome::HideUserManager));
+  if (url_hash_ ==
+      base::UTF8ToUTF16(profiles::kUserManagerSelectProfileTaskManager)) {
+    Browser* browser = BrowserList::GetInstance(desktop_type_)->get(0);
+    base::MessageLoop::current()->PostTask(
+          FROM_HERE, base::Bind(&chrome::ShowTaskManager, browser));

[Evan Stade, 2014/09/11 17:02:51]

I'm worried that |browser| might be invalid by the time ShowTaskManager is
called.

[Mike Lerman, 2014/09/11 18:49:29]

This is for the new browser object that's created for the just-opening profile.
Unless I call ShowTaskManager synchronously, this would always be a concern.

[Evan Stade, 2014/09/11 19:02:52]

I do think you should call ShowTaskManager synchronously. But if you had to do
this async, you could still post a task which first gets the browser then
synchronously calls showTaskManager with it.

[noms (inactive), 2014/09/11 19:05:41]

I'm not sure why we need to PostTask this one. What happens if you don't, and
just call ShowTaskManager?

On 2014/09/11 18:49:29, Mike Lerman wrote:

[Mike Lerman, 2014/09/11 20:51:51]

The TaskManager requires the browser's window to be created. At the time of
killing OnSwitchToProfileComplete, I always have a Browser pointer, but don't
always have a Window. Thus, I post. (updated the comment).

[Evan Stade, 2014/09/11 21:30:00]

Did you consider listening for NOTIFICATION_BROWSER_WINDOW_READY?
Passing |browser| like this is still not safe.

[Mike Lerman, 2014/09/12 14:26:42]

New implementation! Huzzah.

I now look at the browser. If it already has a window that means it was open
previously, and I will call the necessary tasks synchronously (which should be
safe, no?). If not, then I listed to the browser window notification and then
synchronously execute all tasks.

What say you?

+  } else if (
+      url_hash_ ==

[Evan Stade, 2014/09/11 17:02:51]

move to same line as if

[Mike Lerman, 2014/09/11 18:49:29]

Done.

+      base::UTF8ToUTF16(profiles::kUserManagerSelectProfileAboutChrome)) {
+    // The new browser window may not have been created. Make a new one.
+    Browser* browser = chrome::FindAnyBrowser(profile, false, desktop_type_);
+    if (browser) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE, base::Bind(&chrome::ShowAboutChrome, browser));
+    } else {
+      NOTREACHED();
+    }
+  }
+}