Add fallback behavior if the last used profile cannot initialize

chrome/browser/ui/startup/startup_browser_creator.cc

Index: chrome/browser/ui/startup/startup_browser_creator.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator.cc b/chrome/browser/ui/startup/startup_browser_creator.cc
index 8cb76a7171ef019ee311ff43e2587668cccb3051..046226aeccfaac23568209e25e96e6ef83430305 100644
--- a/chrome/browser/ui/startup/startup_browser_creator.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator.cc
@@ -884,3 +884,85 @@ base::FilePath GetStartupProfilePath(const base::FilePath& user_data_dir,
   return g_browser_process->profile_manager()->GetLastUsedProfileDir(
       user_data_dir);
 }
+
+#if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
+Profile* GetStartupProfile(const base::FilePath& user_data_dir,
+                           const base::CommandLine& command_line) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+
+  base::FilePath profile_path =
+      GetStartupProfilePath(user_data_dir, command_line);
+  Profile* profile = profile_manager->GetProfile(profile_path);
+
+  // If we're using the --new-profile-management flag and this profile is
+  // locked, then we should show the user manager instead. By switching the

[brettw, 2016/07/01 23:13:22]

The old comment had the wording "signed out". You added "locked" but I think we
should preserve the signed out wording also since I think that will actually be
the common case when we hit this code path.

[WC Leung, 2016/07/07 17:01:54]

Not sure you've read Mike's comment in
https://codereview.chromium.org/2047483003/diff/180001/chrome/browser/ui/star....
Anyway the exact action in the UI to "sign out" is called "Exit and Childlock".

I do not remember exactly how to access that function though. Here are the steps
if I remember correctly:
1. Create a profile linked to gmail.
2. Create supervised accounts under the profile in (1).
3. Left click the profile button in the title bar.
4. Select "Exit and Childlock".

+  // active profile to the guest profile we ensure that no browser windows will
+  // be opened for the guest profile. The initialization of guest profile is

[Peter Kasting, 2016/07/01 00:53:23]

Do you mean "for the active profile"?  I'm not sure how switching to guest would
prevent windows being opened for guest.

[WC Leung, 2016/07/07 17:01:53]

Oh... that sentence was not written by me. It was in the unpatched code instead.
BTW, I suggest modifying the sentence to "This is done by setting the profile to
the guest profile."

+  // possible to fail.

[Peter Kasting, 2016/07/01 00:53:23]

Nit: is possible to -> may

But what is the import of the guest profile failing to init?

[WC Leung, 2016/07/07 17:01:53]

By design, I think it is not important at all. However, currently if the guest
profile fail to initialize then we're returning a null pointer, so the failure
is currently significant.

+  // If there is no entry in profile attributes storage, it means the profile is
+  // deleted. In this case we should show the user manager.
+  auto& storage = profile_manager->GetProfileAttributesStorage();

[Peter Kasting, 2016/07/01 00:53:23]

Nit: Either use const auto& or auto* (2 places)

[WC Leung, 2016/07/07 17:01:53]

Not sure here.

The declaration of GetProfileAttributesStroage() is currently a
ProfileAttributesStroage&. And I've heard that the rationale was "this function
cannot return a null pointer".

I know that this is contradicting from google c++ guidelines, so sorry for the
confusion. There's even a very interesting workaround in the testing code:
https://cs.chromium.org/chromium/src/chrome/test/base/testing_profile_manager...

WDYT?

[Peter Kasting, 2016/07/11 02:58:58]

This should be changed.  If callers need a non-const object, it should return a
pointer, else it should return a const ref.  If it returns a pointer, the API
comment can note it never returns null.  Plenty of code in our codebase returns
pointers from functions that never return null.

It is OK to fix that in a separate CL, but until that's done I would use "auto*
storage = &..." here.

[Mike Lerman, 2016/07/11 10:01:08]

Changing the return type of this function would be a massive change, whose
discussion and implementation would definitely not happen in this CL.

[WC Leung, 2016/07/18 17:56:18]

Done.
I see.

+  ProfileAttributesEntry* entry;
+  bool has_entry = storage.GetProfileAttributesWithPath(profile_path, &entry);
+
+  if (!has_entry ||
+      (switches::IsNewProfileManagement() && profile &&
+       entry->IsSigninRequired())) {

[Peter Kasting, 2016/07/01 00:53:23]

Nit: Reverse conditional and early-return |profile|, so the rest can be
unindented

[WC Leung, 2016/07/07 17:01:54]

Done.

+    // To show the user manager, return the guest profile. However, we also
+    // need to know that the system profile (where the user manager lives on)

[Peter Kasting, 2016/07/01 00:53:23]

Nit: where...on -> in which

[WC Leung, 2016/07/07 17:01:53]

Done.

+    // exists or is creatable. If either profile is failing, return null to
+    // denote failure.
+    profile =
+        profile_manager->GetProfile(ProfileManager::GetGuestProfilePath());
+    if (!profile_manager->GetProfile(ProfileManager::GetSystemProfilePath()))
+      profile = nullptr;

[Peter Kasting, 2016/07/01 00:53:23]

Nit: Once early-return suggestion above is followed, can become:

  return profile_manager->GetProfile(ProfileManager::GetSystemProfilePath()) ?
      profile_manager->GetProfile(ProfileManager::GetGuestProfilePath()) :
      nullptr;

[WC Leung, 2016/07/07 17:01:53]

Done.

+  }
+
+  return profile;
+}
+
+Profile* GetFallbackStartupProfile() {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  // The only known reason for profiles to fail initialization is unable to
+  // create the profile directory. This has already happened in the last used
+  // profile before calling this function. Therefore, creation of all new
+  // profiles is expected to fail. So only existing profiles are attempted for

[Peter Kasting, 2016/07/01 00:53:23]

If "directory creation" is the only reason for init failure, how could existing
profiles (which already have directories) ever fail?

[WC Leung, 2016/07/07 17:01:53]

Appears that the only way is to have some users delete the profile, and have the
user-data-directory set to "deny creating new directories". Sadly this is
discovered not the cause of 614753. (Frankly I have no clue how the crash
happens.)

BTW, if we disable writing rights on the directory, chromium simply crash some
time after startup. This is not related to this CL though.

[Peter Kasting, 2016/07/11 02:58:59]

I don't understand.  Wouldn't these still be cases where the profiles would be
deleted and thus not in the "existing profiles" list?  Or is the "existing
profiles" list sourced from somewhere else, and may include things that have
since been deleted on disk?  If so it would be nice if the comments could make
this clearer.

[WC Leung, 2016/07/18 17:56:18]

In this case we show the user manager. See
https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star...
for the discussion.
And in this case we try to create the profile. Anyway, if profile creation fails
GetFallbackStartupProfile() may be called.

BTW, both cases should be handled in GetStartupProfile() instead of
GetFallbackStartupProfile(). And anyway, the comment just state that we do not
attempt to create new profiles because that is expected to fail.

+  // fallback.
+
+  // If the last used profile is unable to initialize, see if any of other last
+  // opened profiles can initialize successfully.
+  std::vector<Profile*> last_opened_profiles =
+      ProfileManager::GetLastOpenedProfiles();
+  auto& storage = profile_manager->GetProfileAttributesStorage();
+  for (Profile* last_opened_profile : last_opened_profiles) {
+    // Return any profile that is not signed out.
+    ProfileAttributesEntry* entry;
+    bool has_entry = storage.GetProfileAttributesWithPath(
+                         last_opened_profile->GetPath(), &entry);
+    if (!has_entry || !entry->IsSigninRequired())
+      return last_opened_profile;
+  }
+
+  // If it still fails, try to initialize a guest profile and a system profile.
+  // Show the user manager if this is successful.
+  Profile* guest_profile =
+      profile_manager->GetProfile(ProfileManager::GetGuestProfilePath());
+  Profile* system_profile =

[Peter Kasting, 2016/07/01 00:53:23]

Nit: Can inline into next statement

[WC Leung, 2016/07/07 17:01:53]

I'd rather not to change. With the extra braces in the if statement, we don't
reduce any lines of code by the change.

[Peter Kasting, 2016/07/11 02:58:58]

Really?:

  if (guest_profile &&
      profile_manager->GetProfile(ProfileManager::GetSystemProfilePath())
    return guest_profile;

Not sure what you mean by extra braces unless you mean to add braces on the
above, which wouldn't be necessary, since it has a one-line body (the length of
the conditional is irrelevant when making this determination).

To be clear, it's fine to leave it the way you have it too.

[WC Leung, 2016/07/18 17:56:18]

I've been asked to add the braces when the conditions are longer than one line.
Anyway I'm not sure about the rationale: the google c++ style guide did not
discuss about this case. Not sure whether this is discussed in the internal
guide though.

[Peter Kasting, 2016/07/19 21:56:06]

Both style guides are mute (the Chrome one intentionally so) on this issue.  So
both forms are legal.  It is thus up to author + reviewer discretion whether to
use one or the other.  Both forms are common in the codebase.  While my personal
preference is to omit the braces, use your judgment on what is readable.

[WC Leung, 2016/07/27 18:00:05]

I see. Thanks!

+      profile_manager->GetProfile(ProfileManager::GetSystemProfilePath());
+  if (guest_profile && system_profile)
+    return guest_profile;
+
+  // If still fails, try to open any profile that is not signed out.
+  std::vector<ProfileAttributesEntry*> entries =
+      storage.GetAllProfilesAttributes();
+  for (ProfileAttributesEntry* entry : entries) {
+    if (!entry->IsSigninRequired()) {
+      Profile* profile = profile_manager->GetProfile(entry->GetPath());
+      if (profile)
+        return profile;
+    }
+  }
+
+  return nullptr;
+}
+#endif  // !defined(OS_CHROMEOS) && !defined(OS_ANDROID)