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..8810de79969d9b8e562120863adb589f83b3355a 100644
--- a/chrome/browser/ui/startup/startup_browser_creator.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator.cc
@@ -884,3 +884,75 @@ 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 signed
+  // out, then we should show the user manager instead. By switching the 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
+  // possible to fail.
+  if (switches::IsNewProfileManagement() &&
+      profile && !profile->IsGuestSession()) {
+    ProfileAttributesEntry* entry;
+    bool has_entry = profile_manager->GetProfileAttributesStorage()
+                                    .GetProfileAttributesWithPath(profile_path,
+                                                                  &entry);

[Peter Kasting, 2016/06/16 06:37:49]

Nit: Shorter and IMO more readable:

    auto storage = profile_manager->GetProfileAttributesStorage();
    ProfileAttributesEntry* entry;
    bool has_entry = storage.GetProfileAttributesWithPath(profile_path, &entry);

[WC Leung, 2016/06/19 17:56:19]

Done.

+    if (has_entry && entry->IsSigninRequired()) {
+      profile = profile_manager->GetProfile(

[Peter Kasting, 2016/06/16 06:37:49]

Nit: Prefer to break after '=' instead

[WC Leung, 2016/06/19 17:56:19]

Done.

+                                     ProfileManager::GetGuestProfilePath());
+    }
+  }
+
+  return profile;
+}
+
+Profile* GetFallbackStartupProfile() {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  // If the last used profile is unable to initialize, see if any of other last
+  // opened profiles can initialize successfully (yes if the profile is found in
+  // |last_opened_profiles|). At this stage we assume creation of all new

[Peter Kasting, 2016/06/16 06:37:49]

Nit: Remove parenthetical (comment should explain why, code explains what)

[WC Leung, 2016/06/19 17:56:19]

Done.

+  // profile (including guest profile) to fail.

[Peter Kasting, 2016/06/16 06:37:49]

Nit: This sentence isn't grammatical, and doesn't motivate why we would assume
this.  Explain.

[WC Leung, 2016/06/19 17:56:19]

Done.

+  std::vector<Profile*> last_opened_profiles =
+      ProfileManager::GetLastOpenedProfiles();

[Peter Kasting, 2016/06/16 06:37:49]

Nit: I'd just inline this into the loop below.

[WC Leung, 2016/06/19 17:56:19]

Done.

+  // Return the first profile that is not signed out.
+  for (Profile* last_opened_profile : last_opened_profiles) {
+    ProfileAttributesEntry* entry;
+    bool has_entry = profile_manager->GetProfileAttributesStorage()
+                                    .GetProfileAttributesWithPath(
+                                        last_opened_profile->GetPath(),
+                                        &entry);

[Peter Kasting, 2016/06/16 06:37:49]

Nit: Shorter and IMO more readable:

  auto storage = profile_manager->GetProfileAttributesStorage();
  for (Profile* last_opened_profile : last_opened_profiles) {
    ProfileAttributesEntry* entry;
    bool has_entry = storage.GetProfileAttributesWithPath(
        last_opened_profile->GetPath(), &entry);

This also lets you use |storage| below.

[WC Leung, 2016/06/19 17:56:19]

Done.

+    if (!has_entry || !entry->IsSigninRequired())

[Peter Kasting, 2016/06/16 06:37:49]

I'm confused about why we return here if !has_entry.

[WC Leung, 2016/06/19 17:56:19]

AFAIK the only way for |has_entry| to be false is to run chrome with the switch
"--profile-directory".

BTW, just checked the blame, and it links to
https://codereview.chromium.org/1021613002 and issue 421215 (sadly I don't have
access to that issue).

I have tried to start such a profile. Chromium started with the profile, but
without sync because it is logged off. Here's a screenshot:
https://drive.google.com/open?id=0BxDfZwJpDb1IRVlTWFowRWxpRnM

Anthony and Mike: WDYT, and do you know why was it implemented this way?

[WC Leung, 2016/06/20 15:44:24]

ping for this comment.

[Mike Lerman, 2016/06/20 15:50:31]

In this case, couldn't the Guest Profile also be the reason we don't have an
Entry? In which case, we'd want to go with last_opened_profile.

We should always have a case for !has_entry. I think, though, that if
(!has_entry) should attempt to use the GuestProfile.

[WC Leung, 2016/06/23 15:40:33]

Yes the guest profile can be a reason. BTW, I think returning the guest profile
in the case is good enough.
This makes sense. Thanks.

I am a bit concerned about this change may break existing users by forbidding
them to access existing profiles.

However, to have this happen the user need to copy the Chrome shortcut from
Desktop into another folder (just renaming shortcuts is not enough), or
deliberately execute chrome with a --profile-directory switch. Also, this code
path is not crashing only for 1 year 3 months (after issue 421215 is fixed). I
think it's still okay to make the proposed change. BTW, WDYT?

+      return last_opened_profile;
+  }
+
+  // If it still fails, try to create a guest profile. Show the user manager
+  // if this is successful.
+  Profile* guest_profile =
+      profile_manager->GetProfile(ProfileManager::GetGuestProfilePath());
+

[Peter Kasting, 2016/06/16 06:37:49]

Nit: Unnecessary blank line

[WC Leung, 2016/06/19 17:56:19]

Done.

+  if (guest_profile)
+    return guest_profile;

[WC Leung, 2016/06/19 17:56:19]

Anthony and Mike: do you know why we return the guest profile instead of the
system profile when we intend to show the user manager?

+
+  // If still fails, try to open any non-signin profile.

[Peter Kasting, 2016/06/16 06:37:49]

Nit: any profile not requiring authentication?

[WC Leung, 2016/06/19 17:56:19]

Thanks for reminder. I changed to "any profile that is not signed out".

[WC Leung, 2016/06/20 15:44:24]

% mike's comment, I'll change all of these "signin" / "signed out" to "locked".

+  std::vector<ProfileAttributesEntry*> entries =
+      profile_manager->GetProfileAttributesStorage()
+                     .GetAllProfilesAttributes();

[Peter Kasting, 2016/06/16 06:37:49]

Nit: If you use |storage| here from my comment above, this can probably be
inlined into the loop below.

[WC Leung, 2016/06/19 17:56:19]

Done.

+  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)