Fix crash when switching to a profile that cannot be opened

chrome/browser/profiles/profile_manager_browsertest.cc

Index: chrome/browser/profiles/profile_manager_browsertest.cc
diff --git a/chrome/browser/profiles/profile_manager_browsertest.cc b/chrome/browser/profiles/profile_manager_browsertest.cc
index 06f06dc2d8fa4b9e2843c2751ce1d1f706ab6001..00650374193ef443a132c8c6ffbd596e08343fdd 100644
--- a/chrome/browser/profiles/profile_manager_browsertest.cc
+++ b/chrome/browser/profiles/profile_manager_browsertest.cc
@@ -4,6 +4,7 @@
 
 #include <stddef.h>
 
+#include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/macros.h"
@@ -36,17 +37,33 @@
 #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/06/16 07:02:11]

Nit: Not grammatical, and mostly restates the code anyway.

Does this get called repeatedly, once with CREATE_STATUS_CREATED and then again
after that?  If so maybe:

A callback that does not return control to the test until profile creation has
returned the expected final status, or failed with some unexpected status along
the way.

+void UnblockOnProfileCreation(Profile::CreateStatus expected_final_status,
+                              base::RunLoop* run_loop,
+                              Profile* profile,
+                              Profile::CreateStatus status) {
+  if (status != Profile::CREATE_STATUS_CREATED) {
+    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) {
@@ -148,6 +165,7 @@ class ProfileManagerBrowserTest : public InProcessBrowserTest {
     command_line->AppendSwitch(
         chromeos::switches::kIgnoreUserProfileMappingForTests);
 #endif
+    command_line->AppendSwitch(switches::kNoErrorDialogs);
   }
 };
 
@@ -170,7 +188,7 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, DeleteSingletonProfile) {
   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).
@@ -205,11 +223,11 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, DISABLED_DeleteAllProfiles) {
   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());
@@ -297,8 +315,7 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest,
   }
 }
 
-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;
@@ -317,11 +334,11 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest,
       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();
@@ -352,6 +369,29 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest,
   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(),

[Peter Kasting, 2016/06/16 07:02:11]

Nit: Could combine all these on one line, but whatever

+      std::string(),
+      std::string());
+  run_loop.Run();
+
+  base::RunLoop run_loop2;
+  run_loop2.RunUntilIdle();

[Peter Kasting, 2016/06/16 07:02:11]

What is this doing?

[WC Leung, 2016/06/16 10:04:10]

To wait for async initialization of profile is complete. Otherwise by finishing
the test we delete the profile immediately and this leads to a crash.

Anyway, I forgot about the actual stack trace of the crash. (Need to retry and
see it again.)

And well, hopefully nothing big is happening on non-UI threads. If there is
indeed something big happening then RunUntilIdle makes tests flaky.

[Peter Kasting, 2016/06/16 10:59:59]

Isn't that what the first run loop does?
This sounds like it's related to the other issue.

We really should ensure that you can immediately delete a profile that wasn't
initialized.

+}
+#endif  // defined(OS_WIN)
+
 // Flakes on Windows: http://crbug.com/314905
 #if defined(OS_WIN)
 #define MAYBE_EphemeralProfile DISABLED_EphemeralProfile
@@ -445,7 +485,7 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, DeletePasswords) {
   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;