Fix crash when switching to a profile that cannot be opened

chrome/browser/profiles/profile_manager_browsertest.cc

 // Copyright (c) 2012 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 <stddef.h>
 
+#include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_window.h"
@@ skipping 12 matching lines @@
 
 #if defined(OS_CHROMEOS)
 #include "base/path_service.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chromeos/chromeos_switches.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #endif
 
+#if defined(OS_WIN)
+#include <windows.h>
+#include "base/test/test_file_util.h"
+#endif
+
 namespace {
 
 const ProfileManager::CreateCallback kOnProfileSwitchDoNothing;
 
-// An observer that returns back to test code after a new profile is
-// initialized.
-void OnUnblockOnProfileCreation(base::RunLoop* run_loop,
+// A callback that returns back to test code, and check if the result is
+// expected.

[Peter Kasting, 2016/07/11 02:35:53]

Nit: check -> checks, but really this comment doesn't seem to match very well
with the function name (e.g. the comment says nothing about profiles or
creation).  Not sure what would be a good comment.

[WC Leung, 2016/07/18 09:41:35]

Acknowledged.

+void UnblockOnProfileCreation(Profile::CreateStatus expected_final_status,
+                              base::RunLoop* run_loop,
+                              Profile* profile,
+                              Profile::CreateStatus status) {
+  if (status != Profile::CREATE_STATUS_CREATED) {

[Peter Kasting, 2016/07/11 02:35:54]

Nit: Should this explain why the conditional is here?

[WC Leung, 2016/07/18 09:41:35]

Yes. Profile::CREATE_STATUS_CREATED means the profile is created successfully
but not yet fully initialized. Another call with status
CREATE_STATUS_INITIALIZED will be made in this case. BTW, still thinking about
what to write exactly in this comment.

+    EXPECT_EQ(expected_final_status, status);
+    run_loop->Quit();
+  }
+}
+
+// A callback that returns back to test code after a new profile is initialized.
+void UnblockOnProfileInitialized(base::RunLoop* run_loop,
                                 Profile* profile,
                                 Profile::CreateStatus status) {
-  if (status == Profile::CREATE_STATUS_INITIALIZED)
-    run_loop->Quit();
+  UnblockOnProfileCreation(Profile::CREATE_STATUS_INITIALIZED, run_loop,
+                           profile, status);
 }
 
 void ProfileCreationComplete(Profile* profile, Profile::CreateStatus status) {
   ASSERT_NE(status, Profile::CREATE_STATUS_LOCAL_FAIL);
   ASSERT_NE(status, Profile::CREATE_STATUS_REMOTE_FAIL);
   // No browser should have been created for this profile yet.
   EXPECT_EQ(chrome::GetBrowserCount(profile), 0U);
   EXPECT_EQ(chrome::GetTotalBrowserCount(), 1U);
   if (status == Profile::CREATE_STATUS_INITIALIZED)
     base::MessageLoop::current()->QuitWhenIdle();
@@ skipping 81 matching lines @@
 
 // TODO(jeremy): crbug.com/103355 - These tests should be enabled on all
 // platforms.
 class ProfileManagerBrowserTest : public InProcessBrowserTest {
  protected:
   void SetUpCommandLine(base::CommandLine* command_line) override {
 #if defined(OS_CHROMEOS)
     command_line->AppendSwitch(
         chromeos::switches::kIgnoreUserProfileMappingForTests);
 #endif
+    command_line->AppendSwitch(switches::kNoErrorDialogs);
   }
 };
 
 #if defined(OS_MACOSX)
 
 // Delete single profile and make sure a new one is created.
 IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, DeleteSingletonProfile) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   ProfileAttributesStorage& storage =
       profile_manager->GetProfileAttributesStorage();
   ProfileRemovalObserver observer;
 
   // We should start out with 1 profile.
   ASSERT_EQ(1u, storage.GetNumberOfProfiles());
 
   // Delete singleton profile.
   base::FilePath singleton_profile_path =
       storage.GetAllProfilesAttributes().front()->GetPath();
   EXPECT_FALSE(singleton_profile_path.empty());
   base::RunLoop run_loop;
   profile_manager->ScheduleProfileForDeletion(
       singleton_profile_path,
-      base::Bind(&OnUnblockOnProfileCreation, &run_loop));
+      base::Bind(&UnblockOnProfileInitialized, &run_loop));
 
   // Run the message loop until the profile is actually deleted (as indicated
   // by the callback above being called).
   run_loop.Run();
 
   // Make sure a new profile was created automatically.
   EXPECT_EQ(1u, storage.GetNumberOfProfiles());
   base::FilePath new_profile_path =
       storage.GetAllProfilesAttributes().front()->GetPath();
   EXPECT_NE(new_profile_path.value(), singleton_profile_path.value());
@@ skipping 14 matching lines @@
 // Crashes/CHECKs. See crbug.com/104851
 IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, DISABLED_DeleteAllProfiles) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   ProfileAttributesStorage& storage =
       profile_manager->GetProfileAttributesStorage();
 
   // Create an additional profile.
   base::FilePath new_path = profile_manager->GenerateNextProfileDirectoryPath();
   base::RunLoop run_loop;
   profile_manager->CreateProfileAsync(
-      new_path, base::Bind(&OnUnblockOnProfileCreation, &run_loop),
+      new_path, base::Bind(&UnblockOnProfileInitialized, &run_loop),
       base::string16(), std::string(), std::string());
 
   // Run the message loop to allow profile creation to take place; the loop is
-  // terminated by OnUnblockOnProfileCreation when the profile is created.
+  // terminated by UnblockOnProfileInitialized when the profile is created.
   run_loop.Run();
 
   ASSERT_EQ(2u, storage.GetNumberOfProfiles());
 
   // Delete all profiles.
   std::vector<ProfileAttributesEntry*> entries =
       storage.GetAllProfilesAttributes();
   std::vector<base::FilePath> old_profile_paths;
   for (ProfileAttributesEntry* entry : entries) {
     base::FilePath profile_path = entry->GetPath();
@@ skipping 67 matching lines @@
   EXPECT_EQ(chrome::GetTotalBrowserCount(), 2U);
 
   // Now close all browser windows.
   std::vector<Profile*> profiles = profile_manager->GetLoadedProfiles();
   for (std::vector<Profile*>::const_iterator it = profiles.begin();
        it != profiles.end(); ++it) {
     BrowserList::CloseAllBrowsersWithProfile(*it);
   }
 }
 
-IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest,
-                       SwitchToProfile) {
+IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, SwitchToProfile) {
   // If multiprofile mode is not enabled, you can't switch between profiles.
   if (!profiles::IsMultipleProfilesEnabled())
     return;
 
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   ProfileAttributesStorage& storage =
       profile_manager->GetProfileAttributesStorage();
   size_t initial_profile_count = profile_manager->GetNumberOfProfiles();
   base::FilePath path_profile1 = GetFirstNonSigninProfile(storage);
 
   ASSERT_NE(0U, initial_profile_count);
   EXPECT_EQ(1U, chrome::GetTotalBrowserCount());
 
   // Create an additional profile.
   base::FilePath path_profile2 =
       profile_manager->GenerateNextProfileDirectoryPath();
   base::RunLoop run_loop;
   profile_manager->CreateProfileAsync(
-      path_profile2, base::Bind(&OnUnblockOnProfileCreation, &run_loop),
+      path_profile2, base::Bind(&UnblockOnProfileInitialized, &run_loop),
       base::string16(), std::string(), std::string());
 
   // Run the message loop to allow profile creation to take place; the loop is
-  // terminated by OnUnblockOnProfileCreation when the profile is created.
+  // terminated by UnblockOnProfileInitialized when the profile is created.
   run_loop.Run();
 
   BrowserList* browser_list = BrowserList::GetInstance();
   ASSERT_EQ(initial_profile_count + 1U, storage.GetNumberOfProfiles());
   EXPECT_EQ(1U, browser_list->size());
 
   // Open a browser window for the first profile.
   profiles::SwitchToProfile(path_profile1, false, kOnProfileSwitchDoNothing,
                             ProfileMetrics::SWITCH_PROFILE_ICON);
   EXPECT_EQ(1U, chrome::GetTotalBrowserCount());
@@ skipping 10 matching lines @@
   // Switch to the first profile without opening a new window.
   profiles::SwitchToProfile(path_profile1, false, kOnProfileSwitchDoNothing,
                             ProfileMetrics::SWITCH_PROFILE_ICON);
   EXPECT_EQ(2U, chrome::GetTotalBrowserCount());
   EXPECT_EQ(2U, browser_list->size());
 
   EXPECT_EQ(path_profile1, browser_list->get(0)->profile()->GetPath());
   EXPECT_EQ(path_profile2, browser_list->get(1)->profile()->GetPath());
 }
 
+#if defined(OS_WIN)
+IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, CreateProfileAsyncFail) {
+  base::FilePath user_data_dir =
+      g_browser_process->profile_manager()->user_data_dir();
+  base::DenyFilePermission(user_data_dir, FILE_ADD_SUBDIRECTORY);
+  // Switch to a profile that cannot be loaded or created. This must fail
+  // gracefully with no crash.
+  base::RunLoop run_loop;
+  g_browser_process->profile_manager()->CreateProfileAsync(
+      user_data_dir.AppendASCII("Profile 1"),
+      base::Bind(&UnblockOnProfileCreation,
+                 Profile::CREATE_STATUS_LOCAL_FAIL,  // expected result
+                 &run_loop),
+      base::string16(),
+      std::string(),
+      std::string());
+  run_loop.Run();
+
+  base::RunLoop run_loop2;

[Peter Kasting, 2016/07/11 02:35:53]

Nit: Should probably explain why you need a second runloop.

[Bernhard Bauer, 2016/07/11 08:40:14]

I'd go further than that: I don't think you need to run the current message loop
until it's idle when you've just run it. There should be no task left, unless
something gets posted from a background thread between the time the first run
loop quits and the second run loop runs, which would be a race condition.

(If there was need for the second runloop, you could use it inline, BTW:
base::RunLoop().RunUntilIdle()).

[WC Leung, 2016/07/18 09:41:35]

I'll wait until the code does really not to crash before the final decision.
BTW, agree that the solution we're proposing does not require this run loop.

+  run_loop2.RunUntilIdle();
+}
+#endif  // defined(OS_WIN)
+
 // Flakes on Windows: http://crbug.com/314905
 #if defined(OS_WIN)
 #define MAYBE_EphemeralProfile DISABLED_EphemeralProfile
 #else
 #define MAYBE_EphemeralProfile EphemeralProfile
 #endif
 IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, MAYBE_EphemeralProfile) {
   // If multiprofile mode is not enabled, you can't switch between profiles.
   if (!profiles::IsMultipleProfilesEnabled())
     return;
@@ skipping 73 matching lines @@
 
   password_store->AddLogin(form);
   PasswordStoreConsumerVerifier verify_add;
   password_store->GetAutofillableLogins(&verify_add);
   verify_add.Wait();
   EXPECT_EQ(1u, verify_add.GetPasswords().size());
 
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   base::RunLoop run_loop;
   profile_manager->ScheduleProfileForDeletion(
-      profile->GetPath(), base::Bind(&OnUnblockOnProfileCreation, &run_loop));
+      profile->GetPath(), base::Bind(&UnblockOnProfileInitialized, &run_loop));
   run_loop.Run();
 
   PasswordStoreConsumerVerifier verify_delete;
   password_store->GetAutofillableLogins(&verify_delete);
   verify_delete.Wait();
   EXPECT_EQ(0u, verify_delete.GetPasswords().size());
 }
 #endif  // !defined(OS_WIN) && !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
 
 // Tests Profile::HasOffTheRecordProfile, Profile::IsValidProfile and the
@@ skipping 33 matching lines @@
 
   EXPECT_FALSE(profile->HasOffTheRecordProfile());
   EXPECT_FALSE(profile_manager->IsValidProfile(incognito_profile));
   EXPECT_EQ(initial_profile_count, profile_manager->GetNumberOfProfiles());
   // After destroying the incognito profile incognito preferences should be
   // cleared so the default save path should be taken from the main profile.
   EXPECT_FALSE(profile->GetOffTheRecordPrefs()
                    ->GetFilePath(prefs::kSaveFileDefaultDirectory)
                    .empty());
 }