Fix Windows logoff race.

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 0cc502f4ea4b0aa0a8587afc44ff0970963d1ab5..c2c301ef755ca0aed70f7fe06ad0a481501003ee 100644
--- a/chrome/browser/profiles/profile_browsertest.cc
+++ b/chrome/browser/profiles/profile_browsertest.cc
@@ -8,8 +8,10 @@
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/prefs/pref_service.h"
+#include "base/sequenced_task_runner.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/version.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/chrome_version_service.h"
 #include "chrome/browser/profiles/profile_impl.h"
@@ -42,20 +44,6 @@ void CreatePrefsFileInDirectory(const base::FilePath& directory_path) {
   ASSERT_TRUE(base::WriteFile(pref_path, data.c_str(), data.size()));
 }
 
-scoped_ptr<Profile> CreateProfile(
-    const base::FilePath& path,
-    Profile::Delegate* delegate,
-    Profile::CreateMode create_mode) {
-  scoped_ptr<Profile> profile(Profile::CreateProfile(
-      path, delegate, create_mode));
-  EXPECT_TRUE(profile.get());
-  // This is necessary to avoid a memleak from BookmarkModel::Load.
-  // Unfortunately, this also results in warnings during debug runs.

[gab, 2014/07/03 18:56:56]

As mentioned offline, make sure ASAN bots pass and add memory leak suppressions
with an explanatory bug if not.

-  StartupTaskRunnerServiceFactory::GetForProfile(profile.get())->
-      StartDeferredTaskRunners();
-  return profile.Pass();
-}
-
 void CheckChromeVersion(Profile *profile, bool is_new) {
   std::string created_by_version;
   if (is_new) {
@@ -77,6 +65,15 @@ void BlockThread(
   unblock->Wait();
 }
 
+void FlushTaskRunner(base::SequencedTaskRunner* runner) {
+  base::WaitableEvent unblock(false, false);
+
+  runner->PostTask(FROM_HERE,
+      base::Bind(&base::WaitableEvent::Signal, base::Unretained(&unblock)));
+
+  unblock.Wait();
+}
+
 void SpinThreads() {
   // Give threads a chance to do their stuff before shutting down (i.e.
   // deleting scoped temp dir etc).
@@ -97,14 +94,33 @@ class ProfileBrowserTest : public InProcessBrowserTest {
         chromeos::switches::kIgnoreUserProfileMappingForTests);
 #endif
   }
+
+  scoped_ptr<Profile> CreateProfile(
+      const base::FilePath& path,
+      Profile::Delegate* delegate,
+      Profile::CreateMode create_mode) {
+    scoped_ptr<Profile> profile(Profile::CreateProfile(
+        path, delegate, create_mode));
+    EXPECT_TRUE(profile.get());
+
+    // Store the Profile's IO task runner so we can wind it down.
+    profile_io_task_runner_ = profile->GetIOTaskRunner();
+
+    return profile.Pass();
+  }
+
+  void FlushIoTaskRunnerAndSpinThreads() {
+    FlushTaskRunner(profile_io_task_runner_);
+
+    SpinThreads();
+  }
+
+  scoped_refptr<base::SequencedTaskRunner> profile_io_task_runner_;
 };
 
 // Test OnProfileCreate is called with is_new_profile set to true when
 // creating a new profile synchronously.
-//
-// Flaky (sometimes timeout): http://crbug.com/141141
-IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
-                       DISABLED_CreateNewProfileSynchronous) {
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, CreateNewProfileSynchronous) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
@@ -117,14 +133,12 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
     CheckChromeVersion(profile.get(), true);
   }
 
-  SpinThreads();
+  FlushIoTaskRunnerAndSpinThreads();

[noms (inactive), 2014/07/03 18:07:11]

Is this used by every test? Could it go in the TearDown() function?

[Sigurður Ásgeirsson, 2014/07/03 18:14:46]

Sadly no, the last test has different rundown requirements.

[noms (inactive), 2014/07/03 18:23:42]

Ah, I see. Thanks!

On 2014/07/03 18:14:46, Sigurður Ásgeirsson wrote:

 }
 
 // Test OnProfileCreate is called with is_new_profile set to false when
 // creating a profile synchronously with an existing prefs file.
-// Flaky: http://crbug.com/141517
-IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
-                       DISABLED_CreateOldProfileSynchronous) {
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, CreateOldProfileSynchronous) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   CreatePrefsFileInDirectory(temp_dir.path());
@@ -138,14 +152,12 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
     CheckChromeVersion(profile.get(), false);
   }
 
-  SpinThreads();
+  FlushIoTaskRunnerAndSpinThreads();
 }
 
 // Test OnProfileCreate is called with is_new_profile set to true when
 // creating a new profile asynchronously.
-// This test is flaky on Linux, Win and Mac.  See crbug.com/142787
-IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
-                       DISABLED_CreateNewProfileAsynchronous) {
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, CreateNewProfileAsynchronous) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
@@ -165,14 +177,12 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
     CheckChromeVersion(profile.get(), true);
   }
 
-  SpinThreads();
+  FlushIoTaskRunnerAndSpinThreads();
 }
 
 // Test OnProfileCreate is called with is_new_profile set to false when
 // creating a profile asynchronously with an existing prefs file.
-// Flaky: http://crbug.com/141517
-IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
-                       DISABLED_CreateOldProfileAsynchronous) {
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, CreateOldProfileAsynchronous) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   CreatePrefsFileInDirectory(temp_dir.path());
@@ -193,12 +203,11 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
     CheckChromeVersion(profile.get(), false);
   }
 
-  SpinThreads();
+  FlushIoTaskRunnerAndSpinThreads();
 }
 
 // Test that a README file is created for profiles that didn't have it.
-// Flaky: http://crbug.com/140882
-IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, DISABLED_ProfileReadmeCreated) {
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, ProfileReadmeCreated) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
@@ -227,7 +236,7 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, DISABLED_ProfileReadmeCreated) {
         temp_dir.path().Append(chrome::kReadmeFilename)));
   }
 
-  SpinThreads();
+  FlushIoTaskRunnerAndSpinThreads();
 }
 
 // Test that Profile can be deleted before README file is created.
@@ -262,17 +271,11 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, ProfileDeletedBeforeReadmeCreated) {
   // task for README creation).
   unblock->Signal();
 
-  SpinThreads();
+  FlushIoTaskRunnerAndSpinThreads();
 }
 
 // Test that repeated setting of exit type is handled correctly.
-#if defined(OS_WIN)
-// Flaky on Windows: http://crbug.com/163713
-#define MAYBE_ExitType DISABLED_ExitType
-#else
-#define MAYBE_ExitType ExitType
-#endif
-IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, MAYBE_ExitType) {
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, ExitType) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
@@ -302,5 +305,23 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, MAYBE_ExitType) {
     EXPECT_EQ(crash_value, final_value);
   }
 
+  FlushIoTaskRunnerAndSpinThreads();
+}
+
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, WritesProfilesOnEndSession) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  MockProfileDelegate delegate;
+  EXPECT_CALL(delegate, OnProfileCreated(testing::NotNull(), true, true));
+  {
+    scoped_ptr<Profile> profile(CreateProfile(
+        temp_dir.path(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
+
+    // This should not return until the profile data has been written to disk.
+    // If this test flakes, then logoff on Windows has probably broken again.

[erikwright (departed), 2014/07/03 19:06:41]

I suggest actually loading (synchronously) the contents of the prefs file on
disk and asserting that before this line they indicate an unclean shutdown
(might require you to flush/spin _before_ EndSession) and after this line they
don't.

If there is anything else that we want to be absolutely sure happens during
shutdown we should add that explicitly here, too, if at all possible.

[Sigurður Ásgeirsson, 2014/07/04 13:23:37]

Done.

+    g_browser_process->EndSession();

[gab, 2014/07/03 18:56:56]

IMO we should test that g_browser_process->EndSession() completes faster then
|kEndSessionTimeoutSeconds|.

Otherwise if the shutdown code has an error such that we always wait for the
timeout (10 seconds), this test might still pass (test timeout is 30s for
Release and 45s for Debug IIRC) -- you can try this by introducing a bug in
RundownTaskCounter by starting the count at 2 and verifying that this test still
passes...

[Sigurður Ásgeirsson, 2014/07/04 13:23:37]

I'm done with regression tests for this, if you want more tests then I'd be more
than happy to review the CL :).

+  }
+
   SpinThreads();

[noms (inactive), 2014/07/03 18:07:11]

Hmm, why does this call SpinThreads() where all the other tests use
FlushIoTaskRunnerAndSpinThreads()?

[Sigurður Ásgeirsson, 2014/07/03 18:14:46]

Because the EndSession function is required to block for the end of the IO. This
is the only way to test this that I can think of - e.g. this test should only
flake if EndSession ceases to synchronize correctly, which is not to say that it
will reliably fail in that instance.

[noms (inactive), 2014/07/03 18:23:42]

Would you mind adding this as a comment? Thanks! 

On 2014/07/03 18:14:46, Sigurður Ásgeirsson wrote:

[Sigurður Ásgeirsson, 2014/07/03 18:28:58]

Per our chat, the comment on line 321 serves the purpose...

[Sigurður Ásgeirsson, 2014/07/03 18:51:09]

Ugh - I misunderstood your request. Added a comment for maintainabilityisms.

[erikwright (departed), 2014/07/03 19:06:42]

Presumably, stuff still _can_ be running on the other threads. EndSession only
promises that the most important stuff will complete.

Is it safe to exit here before those things finish? Presumably it is risky. So
If you follow my recommendation above to explicitly check that prefs were
written you can just flush/spin as everywhere else.

[Sigurður Ásgeirsson, 2014/07/04 13:23:37]

Done.

 }