Try to fix ProfileBrowserTest.

chrome/browser/profiles/profile_browsertest.cc

Index: chrome/browser/profiles/profile_browsertest.cc
diff --git a/chrome/browser/profiles/profile_browsertest.cc b/chrome/browser/profiles/profile_browsertest.cc
index aa5039d3251af16e20a8941785952a134161626b..1d376011c276326d7316c95ddebdacbdf6e41763 100644
--- a/chrome/browser/profiles/profile_browsertest.cc
+++ b/chrome/browser/profiles/profile_browsertest.cc
@@ -7,12 +7,11 @@
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/prefs/pref_service.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/version.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/chrome_version_service.h"
 #include "chrome/browser/profiles/profile_impl.h"
-#include "chrome/browser/profiles/startup_task_runner_service.h"
-#include "chrome/browser/profiles/startup_task_runner_service_factory.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
@@ -50,6 +49,13 @@ void CheckChromeVersion(Profile *profile, bool is_new) {
   EXPECT_EQ(created_by_version, pref_version);
 }
 
+void BlockThread(
+    base::WaitableEvent* is_blocked,
+    base::WaitableEvent* unblock) {
+  is_blocked->Signal();
+  unblock->Wait();
+}
+
 }  // namespace
 
 typedef InProcessBrowserTest ProfileBrowserTest;
@@ -66,12 +72,19 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
   MockProfileDelegate delegate;
   EXPECT_CALL(delegate, OnProfileCreated(testing::NotNull(), true, true));
 
-  scoped_ptr<Profile> profile(Profile::CreateProfile(
-      temp_dir.path(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
-  ASSERT_TRUE(profile.get());
-  CheckChromeVersion(profile.get(), true);
-  StartupTaskRunnerServiceFactory::GetForProfile(profile.get())->
-              StartDeferredTaskRunners();
+  {
+    scoped_ptr<Profile> profile(Profile::CreateProfile(
+        temp_dir.path(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
+    ASSERT_TRUE(profile.get());
+    CheckChromeVersion(profile.get(), true);
+  }
+
+  // Give threads a chance to do their stuff before deleting scoped temp dir.
+  // Should not be necessary anymore once Profile deletion is fixed
+  // (see crbug.com/88586).
+  content::RunAllPendingInMessageLoop();
+  content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
+  content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);

[rpetterson, 2014/04/09 20:05:11]

Can you use a ThreadBundle and eliminate all this special casing?

[Marc Treib, 2014/04/10 09:25:09]

You mean a TestBrowserThreadBundle? As I understand it, that's meant only for
unit tests, not browser tests? Here, all the threads already exist.
However, I could replicate the full shutdown behavior from
TestBrowserThreadBundle.

[rpetterson, 2014/04/16 17:50:44]

Ah, I didn't realize that that was just for browser tests. No worries. Although
you could group them into a helper function to cut down on repeated code. Same
with the profile creation. But I'm fine doing that in a cleanup CL.

 }
 
 // Test OnProfileCreate is called with is_new_profile set to false when
@@ -86,12 +99,19 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
   MockProfileDelegate delegate;
   EXPECT_CALL(delegate, OnProfileCreated(testing::NotNull(), true, false));
 
-  scoped_ptr<Profile> profile(Profile::CreateProfile(
-      temp_dir.path(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
-  ASSERT_TRUE(profile.get());
-  CheckChromeVersion(profile.get(), false);
-  StartupTaskRunnerServiceFactory::GetForProfile(profile.get())->
-              StartDeferredTaskRunners();
+  {
+    scoped_ptr<Profile> profile(Profile::CreateProfile(
+        temp_dir.path(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
+    ASSERT_TRUE(profile.get());
+    CheckChromeVersion(profile.get(), false);
+  }
+
+  // Give threads a chance to do their stuff before deleting scoped temp dir.
+  // Should not be necessary anymore once Profile deletion is fixed
+  // (see crbug.com/88586).
+  content::RunAllPendingInMessageLoop();
+  content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
+  content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
 }
 
 // Test OnProfileCreate is called with is_new_profile set to true when
@@ -105,18 +125,26 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
   MockProfileDelegate delegate;
   EXPECT_CALL(delegate, OnProfileCreated(testing::NotNull(), true, true));
 
-  scoped_ptr<Profile> profile(Profile::CreateProfile(
-      temp_dir.path(), &delegate, Profile::CREATE_MODE_ASYNCHRONOUS));
-  ASSERT_TRUE(profile.get());
-  StartupTaskRunnerServiceFactory::GetForProfile(profile.get())->
-              StartDeferredTaskRunners();
-
-  // Wait for the profile to be created.
-  content::WindowedNotificationObserver observer(
-      chrome::NOTIFICATION_PROFILE_CREATED,
-      content::Source<Profile>(profile.get()));
-  observer.Wait();
-  CheckChromeVersion(profile.get(), true);
+  {
+    content::WindowedNotificationObserver observer(
+        chrome::NOTIFICATION_PROFILE_CREATED,
+        content::NotificationService::AllSources());
+
+    scoped_ptr<Profile> profile(Profile::CreateProfile(
+        temp_dir.path(), &delegate, Profile::CREATE_MODE_ASYNCHRONOUS));
+    ASSERT_TRUE(profile.get());
+
+    // Wait for the profile to be created.
+    observer.Wait();
+    CheckChromeVersion(profile.get(), true);
+  }
+
+  // Give threads a chance to do their stuff before deleting scoped temp dir.
+  // Should not be necessary anymore once Profile deletion is fixed
+  // (see crbug.com/88586).
+  content::RunAllPendingInMessageLoop();
+  content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
+  content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
 }
 
 // Test OnProfileCreate is called with is_new_profile set to false when
@@ -130,18 +158,27 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
 
   MockProfileDelegate delegate;
   EXPECT_CALL(delegate, OnProfileCreated(testing::NotNull(), true, false));
-  scoped_ptr<Profile> profile(Profile::CreateProfile(
-      temp_dir.path(), &delegate, Profile::CREATE_MODE_ASYNCHRONOUS));
-  ASSERT_TRUE(profile.get());
-  StartupTaskRunnerServiceFactory::GetForProfile(profile.get())->
-              StartDeferredTaskRunners();
-
-  // Wait for the profile to be created.
-  content::WindowedNotificationObserver observer(
-      chrome::NOTIFICATION_PROFILE_CREATED,
-      content::Source<Profile>(profile.get()));
-  observer.Wait();
-  CheckChromeVersion(profile.get(), false);
+
+  {
+    content::WindowedNotificationObserver observer(
+        chrome::NOTIFICATION_PROFILE_CREATED,
+        content::NotificationService::AllSources());
+
+    scoped_ptr<Profile> profile(Profile::CreateProfile(
+        temp_dir.path(), &delegate, Profile::CREATE_MODE_ASYNCHRONOUS));
+    ASSERT_TRUE(profile.get());
+
+    // Wait for the profile to be created.
+    observer.Wait();
+    CheckChromeVersion(profile.get(), false);
+  }
+
+  // Give threads a chance to do their stuff before deleting scoped temp dir.
+  // Should not be necessary anymore once Profile deletion is fixed
+  // (see crbug.com/88586).
+  content::RunAllPendingInMessageLoop();
+  content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
+  content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
 }
 
 // Test that a README file is created for profiles that didn't have it.
@@ -156,23 +193,32 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, DISABLED_ProfileReadmeCreated) {
   // No delay before README creation.
   ProfileImpl::create_readme_delay_ms = 0;
 
-  scoped_ptr<Profile> profile(Profile::CreateProfile(
-      temp_dir.path(), &delegate, Profile::CREATE_MODE_ASYNCHRONOUS));
-  ASSERT_TRUE(profile.get());
-  StartupTaskRunnerServiceFactory::GetForProfile(profile.get())->
-              StartDeferredTaskRunners();
+  {
+    content::WindowedNotificationObserver observer(
+        chrome::NOTIFICATION_PROFILE_CREATED,
+        content::NotificationService::AllSources());
 
-  // Wait for the profile to be created.
-  content::WindowedNotificationObserver observer(
-      chrome::NOTIFICATION_PROFILE_CREATED,
-      content::Source<Profile>(profile.get()));
-  observer.Wait();
+    scoped_ptr<Profile> profile(Profile::CreateProfile(
+        temp_dir.path(), &delegate, Profile::CREATE_MODE_ASYNCHRONOUS));
+    ASSERT_TRUE(profile.get());
 
-  content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
+    // Wait for the profile to be created.
+    observer.Wait();
+
+    // Wait for file thread to create the README.
+    content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
 
-  // Verify that README exists.
-  EXPECT_TRUE(base::PathExists(
-      temp_dir.path().Append(chrome::kReadmeFilename)));
+    // Verify that README exists.
+    EXPECT_TRUE(base::PathExists(
+        temp_dir.path().Append(chrome::kReadmeFilename)));
+  }
+
+  // Give threads a chance to do their stuff before deleting scoped temp dir.
+  // Should not be necessary anymore once Profile deletion is fixed
+  // (see crbug.com/88586).
+  content::RunAllPendingInMessageLoop();
+  content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
+  content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
 }
 
 // Test that Profile can be deleted before README file is created.
@@ -186,15 +232,27 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, ProfileDeletedBeforeReadmeCreated) {
   // No delay before README creation.
   ProfileImpl::create_readme_delay_ms = 0;
 
+  base::WaitableEvent is_blocked(false, false);
+  base::WaitableEvent* unblock = new base::WaitableEvent(false, false);
+
+  // Block file thread.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&BlockThread, &is_blocked, base::Owned(unblock)));
+  // Wait for file thread to actually be blocked.
+  is_blocked.Wait();
+
   scoped_ptr<Profile> profile(Profile::CreateProfile(
       temp_dir.path(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
   ASSERT_TRUE(profile.get());
-  StartupTaskRunnerServiceFactory::GetForProfile(profile.get())->
-              StartDeferredTaskRunners();
 
-  // Delete the Profile instance and run pending tasks (this includes the task
-  // for README creation).
+  // Delete the Profile instance before we give the file thread a chance to
+  // create the README.
   profile.reset();
+
+  // Now unblock the file thread again and run pending tasks (this includes the
+  // task for README creation).
+  unblock->Signal();
   content::RunAllPendingInMessageLoop();
   content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
   content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
@@ -213,36 +271,35 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, MAYBE_ExitType) {
 
   MockProfileDelegate delegate;
   EXPECT_CALL(delegate, OnProfileCreated(testing::NotNull(), true, true));
+  {
+    scoped_ptr<Profile> profile(Profile::CreateProfile(
+        temp_dir.path(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
+    ASSERT_TRUE(profile.get());
+
+    PrefService* prefs = profile->GetPrefs();
+    // The initial state is crashed; store for later reference.
+    std::string crash_value(prefs->GetString(prefs::kSessionExitType));
+
+    // The first call to a type other than crashed should change the value.
+    profile->SetExitType(Profile::EXIT_SESSION_ENDED);
+    std::string first_call_value(prefs->GetString(prefs::kSessionExitType));
+    EXPECT_NE(crash_value, first_call_value);
+
+    // Subsequent calls to a non-crash value should be ignored.
+    profile->SetExitType(Profile::EXIT_NORMAL);
+    std::string second_call_value(prefs->GetString(prefs::kSessionExitType));
+    EXPECT_EQ(first_call_value, second_call_value);
+
+    // Setting back to a crashed value should work.
+    profile->SetExitType(Profile::EXIT_CRASHED);
+    std::string final_value(prefs->GetString(prefs::kSessionExitType));
+    EXPECT_EQ(crash_value, final_value);
+  }
 
-  scoped_ptr<Profile> profile(Profile::CreateProfile(
-      temp_dir.path(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
-  ASSERT_TRUE(profile.get());
-  StartupTaskRunnerServiceFactory::GetForProfile(profile.get())->
-              StartDeferredTaskRunners();
-
-  PrefService* prefs = profile->GetPrefs();
-  // The initial state is crashed; store for later reference.
-  std::string crash_value(prefs->GetString(prefs::kSessionExitType));
-
-  // The first call to a type other than crashed should change the value.
-  profile->SetExitType(Profile::EXIT_SESSION_ENDED);
-  std::string first_call_value(prefs->GetString(prefs::kSessionExitType));
-  EXPECT_NE(crash_value, first_call_value);
-
-  // Subsequent calls to a non-crash value should be ignored.
-  profile->SetExitType(Profile::EXIT_NORMAL);
-  std::string second_call_value(prefs->GetString(prefs::kSessionExitType));
-  EXPECT_EQ(first_call_value, second_call_value);
-
-  // Setting back to a crashed value should work.
-  profile->SetExitType(Profile::EXIT_CRASHED);
-  std::string final_value(prefs->GetString(prefs::kSessionExitType));
-  EXPECT_EQ(crash_value, final_value);
-
-  // This test runs fast enough that the WebDataService may still be
-  // initializing (which uses the temp directory) when the test
-  // ends. Give it a chance to complete.
-  profile.reset();
+  // Give threads a chance to do their stuff before deleting scoped temp dir.
+  // Should not be necessary anymore once Profile deletion is fixed
+  // (see crbug.com/88586).
   content::RunAllPendingInMessageLoop();
   content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
+  content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
 }