ImportantFileWriter: check return value of PostTask

when writing to the disk, so we can be sure it doesn't lose data.

Also fix tests that misused things.

TEST=none
BUG=none

Review URL: http://codereview.chromium.org/6627060

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@77695 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 103 insertions, 45 deletions

diff --git a/chrome/browser/browser_commands_unittest.cc b/chrome/browser/browser_commands_unittest.cc
index 7afd399..6df08b9 100644
--- a/chrome/browser/browser_commands_unittest.cc
+++ b/chrome/browser/browser_commands_unittest.cc
@@ -79,7 +79,6 @@ TEST_F(BrowserCommandsTest, DuplicateTab) {
 }
 
 TEST_F(BrowserCommandsTest, BookmarkCurrentPage) {
-  BrowserThread file_loop(BrowserThread::FILE, MessageLoop::current());
   // We use profile() here, since it's a TestingProfile.
   profile()->CreateBookmarkModel(true);
   profile()->BlockUntilBookmarkModelLoaded();
diff --git a/chrome/browser/extensions/extension_menu_manager_unittest.cc b/chrome/browser/extensions/extension_menu_manager_unittest.cc
index 0de6611..894d49a 100644
--- a/chrome/browser/extensions/extension_menu_manager_unittest.cc
+++ b/chrome/browser/extensions/extension_menu_manager_unittest.cc
@@ -31,8 +31,11 @@ using testing::SaveArg;
 // Base class for tests.
 class ExtensionMenuManagerTest : public testing::Test {
  public:
-  ExtensionMenuManagerTest() : next_id_(1) {}
-  ~ExtensionMenuManagerTest() {}
+  ExtensionMenuManagerTest()
+      : ui_thread_(BrowserThread::UI, &message_loop_),
+        file_thread_(BrowserThread::FILE, &message_loop_),
+        next_id_(1) {
+  }
 
   // Returns a test item.
   ExtensionMenuItem* CreateTestItem(Extension* extension) {
@@ -51,6 +54,10 @@ class ExtensionMenuManagerTest : public testing::Test {
   }
 
  protected:
+  MessageLoopForUI message_loop_;
+  BrowserThread ui_thread_;
+  BrowserThread file_thread_;
+
   ExtensionMenuManager manager_;
   ExtensionList extensions_;
   TestExtensionPrefs prefs_;
@@ -409,9 +416,6 @@ TEST_F(ExtensionMenuManagerTest, RemoveOneByOne) {
 }
 
 TEST_F(ExtensionMenuManagerTest, ExecuteCommand) {
-  MessageLoopForUI message_loop;
-  BrowserThread ui_thread(BrowserThread::UI, &message_loop);
-
   MockTestingProfile profile;
 
   scoped_ptr<MockExtensionEventRouter> mock_event_router(
diff --git a/chrome/browser/extensions/extension_prefs_unittest.cc b/chrome/browser/extensions/extension_prefs_unittest.cc
index 8ff607f..6baf3f6 100644
--- a/chrome/browser/extensions/extension_prefs_unittest.cc
+++ b/chrome/browser/extensions/extension_prefs_unittest.cc
@@ -16,6 +16,7 @@
 #include "content/common/notification_details.h"
 #include "content/common/notification_observer_mock.h"
 #include "content/common/notification_source.h"
+#include "content/browser/browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::Time;
@@ -61,7 +62,10 @@ static void AssertEqualExtents(ExtensionExtent* extent1,
 // Base class for tests.
 class ExtensionPrefsTest : public testing::Test {
  public:
-  ExtensionPrefsTest() {}
+  ExtensionPrefsTest()
+      : ui_thread_(BrowserThread::UI, &message_loop_),
+        file_thread_(BrowserThread::FILE, &message_loop_) {
+  }
 
   // This function will get called once, and is the right place to do operations
   // on ExtensionPrefs that write data.
@@ -92,6 +96,10 @@ class ExtensionPrefsTest : public testing::Test {
  protected:
   ExtensionPrefs* prefs() { return prefs_.prefs(); }
 
+  MessageLoop message_loop_;
+  BrowserThread ui_thread_;
+  BrowserThread file_thread_;
+
   TestExtensionPrefs prefs_;
 
  private:
diff --git a/chrome/browser/extensions/extension_updater_unittest.cc b/chrome/browser/extensions/extension_updater_unittest.cc
index 5a73142..c1be0e4 100644
--- a/chrome/browser/extensions/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/extension_updater_unittest.cc
@@ -315,6 +315,11 @@ class ExtensionUpdaterTest : public testing::Test {
   }
 
   static void TestExtensionUpdateCheckRequests(bool pending) {
+    MessageLoop message_loop;
+    BrowserThread file_thread(BrowserThread::FILE, &message_loop);
+    BrowserThread io_thread(BrowserThread::IO);
+    io_thread.Start();
+
     // Create an extension with an update_url.
     ServiceForManifestTests service;
     std::string update_url("http://foo.com/bar");
@@ -329,11 +334,7 @@ class ExtensionUpdaterTest : public testing::Test {
       service.set_extensions(extensions);
     }
 
-    // Setup and start the updater.
-    MessageLoop message_loop;
-    BrowserThread io_thread(BrowserThread::IO);
-    io_thread.Start();
-
+    // Set up and start the updater.
     TestURLFetcherFactory factory;
     URLFetcher::set_factory(&factory);
     scoped_refptr<ExtensionUpdater> updater(
@@ -493,13 +494,15 @@ class ExtensionUpdaterTest : public testing::Test {
   }
 
   static void TestDetermineUpdates() {
+    MessageLoop message_loop;
+    BrowserThread file_thread(BrowserThread::FILE, &message_loop);
+
     // Create a set of test extensions
     ServiceForManifestTests service;
     ExtensionList tmp;
     service.CreateTestExtensions(1, 3, &tmp, NULL, Extension::INTERNAL);
     service.set_extensions(tmp);
 
-    MessageLoop message_loop;
     scoped_refptr<ExtensionUpdater> updater(
         new ExtensionUpdater(&service, service.pref_service(),
                              kUpdateFrequencySecs));
@@ -575,9 +578,9 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcherFactory factory;
     TestURLFetcher* fetcher = NULL;
     URLFetcher::set_factory(&factory);
-    ServiceForDownloadTests service;
+    scoped_ptr<ServiceForDownloadTests> service(new ServiceForDownloadTests);
     scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(&service, service.pref_service(),
+        new ExtensionUpdater(service.get(), service->pref_service(),
                              kUpdateFrequencySecs));
     updater->Start();
 
@@ -627,8 +630,13 @@ class ExtensionUpdaterTest : public testing::Test {
     file_thread.Stop();
     io_thread.Stop();
     ui_loop.RunAllPending();
-    EXPECT_EQ("12345", service.last_inquired_extension_id());
+    EXPECT_EQ("12345", service->last_inquired_extension_id());
     xmlCleanupGlobals();
+
+    // The FILE thread is needed for |service|'s cleanup,
+    // because of ImportantFileWriter.
+    file_thread.Start();
+    service.reset();
   }
 
   static void TestSingleExtensionDownloading(bool pending) {
@@ -642,9 +650,9 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcherFactory factory;
     TestURLFetcher* fetcher = NULL;
     URLFetcher::set_factory(&factory);
-    ServiceForDownloadTests service;
+    scoped_ptr<ServiceForDownloadTests> service(new ServiceForDownloadTests);
     scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(&service, service.pref_service(),
+        new ExtensionUpdater(service.get(), service->pref_service(),
                              kUpdateFrequencySecs));
     updater->Start();
 
@@ -666,7 +674,7 @@ class ExtensionUpdaterTest : public testing::Test {
           PendingExtensionInfo(test_url, &ShouldAlwaysInstall, kIsFromSync,
                                kInstallSilently, kInitialState,
                                kInitialIncognitoEnabled, Extension::INTERNAL);
-      service.set_pending_extensions(pending_extensions);
+      service->set_pending_extensions(pending_extensions);
     }
 
     // Call back the ExtensionUpdater with a 200 response and some test data
@@ -683,14 +691,19 @@ class ExtensionUpdaterTest : public testing::Test {
 
     // Expect that ExtensionUpdater asked the mock extensions service to install
     // a file with the test data for the right id.
-    EXPECT_EQ(id, service.extension_id());
-    FilePath tmpfile_path = service.install_path();
+    EXPECT_EQ(id, service->extension_id());
+    FilePath tmpfile_path = service->install_path();
     EXPECT_FALSE(tmpfile_path.empty());
-    EXPECT_EQ(test_url, service.download_url());
+    EXPECT_EQ(test_url, service->download_url());
     std::string file_contents;
     EXPECT_TRUE(file_util::ReadFileToString(tmpfile_path, &file_contents));
     EXPECT_TRUE(extension_data == file_contents);
 
+    // The FILE thread is needed for |service|'s cleanup,
+    // because of ImportantFileWriter.
+    file_thread.Start();
+    service.reset();
+
     file_util::Delete(tmpfile_path, false);
     URLFetcher::set_factory(NULL);
   }
@@ -698,6 +711,7 @@ class ExtensionUpdaterTest : public testing::Test {
   static void TestBlacklistDownloading() {
     MessageLoop message_loop;
     BrowserThread ui_thread(BrowserThread::UI, &message_loop);
+    BrowserThread file_thread(BrowserThread::FILE, &message_loop);
     BrowserThread io_thread(BrowserThread::IO);
     io_thread.Start();
 
@@ -820,6 +834,9 @@ class ExtensionUpdaterTest : public testing::Test {
   static void TestGalleryRequests(int rollcall_ping_days,
                                   int active_ping_days,
                                   bool active_bit) {
+    MessageLoop message_loop;
+    BrowserThread file_thread(BrowserThread::FILE, &message_loop);
+
     TestURLFetcherFactory factory;
     URLFetcher::set_factory(&factory);
 
@@ -860,7 +877,6 @@ class ExtensionUpdaterTest : public testing::Test {
     if (active_bit)
       prefs->SetActiveBit(id, true);
 
-    MessageLoop message_loop;
     scoped_refptr<ExtensionUpdater> updater(
       new ExtensionUpdater(&service, service.pref_service(),
                            kUpdateFrequencySecs));
@@ -972,6 +988,9 @@ TEST(ExtensionUpdaterTest, TestBlacklistUpdateCheckRequests) {
 }
 
 TEST(ExtensionUpdaterTest, TestUpdateUrlData) {
+  MessageLoop message_loop;
+  BrowserThread file_thread(BrowserThread::FILE, &message_loop);
+
   ExtensionUpdaterTest::TestUpdateUrlDataEmpty();
   ExtensionUpdaterTest::TestUpdateUrlDataSimple();
   ExtensionUpdaterTest::TestUpdateUrlDataCompound();
@@ -1044,6 +1063,9 @@ TEST(ExtensionUpdaterTest, TestHandleManifestResults) {
 }
 
 TEST(ExtensionUpdaterTest, TestManifestFetchesBuilderAddExtension) {
+  MessageLoop message_loop;
+  BrowserThread file_thread(BrowserThread::FILE, &message_loop);
+
   MockService service;
   ManifestFetchesBuilder builder(&service);
 
@@ -1091,6 +1113,8 @@ TEST(ExtensionUpdaterTest, TestManifestFetchesBuilderAddExtension) {
 
 TEST(ExtensionUpdaterTest, TestStartUpdateCheckMemory) {
     MessageLoop message_loop;
+    BrowserThread file_thread(BrowserThread::FILE, &message_loop);
+
     ServiceForManifestTests service;
     TestURLFetcherFactory factory;
     URLFetcher::set_factory(&factory);
@@ -1110,6 +1134,8 @@ TEST(ExtensionUpdaterTest, TestStartUpdateCheckMemory) {
 
 TEST(ExtensionUpdaterTest, TestAfterStopBehavior) {
     MessageLoop message_loop;
+    BrowserThread file_thread(BrowserThread::FILE, &message_loop);
+
     ServiceForManifestTests service;
     scoped_refptr<ExtensionUpdater> updater(
         new ExtensionUpdater(&service, service.pref_service(),
diff --git a/chrome/browser/extensions/test_extension_prefs.cc b/chrome/browser/extensions/test_extension_prefs.cc
index a8c8a66..ba8d957 100644
--- a/chrome/browser/extensions/test_extension_prefs.cc
+++ b/chrome/browser/extensions/test_extension_prefs.cc
@@ -9,6 +9,7 @@
 #include "base/message_loop_proxy.h"
 #include "base/scoped_ptr.h"
 #include "base/values.h"
+#include "base/synchronization/waitable_event.h"
 #include "chrome/browser/extensions/extension_pref_store.h"
 #include "chrome/browser/extensions/extension_pref_value_map.h"
 #include "chrome/browser/extensions/extension_prefs.h"
@@ -18,6 +19,7 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/json_pref_store.h"
+#include "chrome/test/signaling_task.h"
 #include "content/browser/browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -65,10 +67,18 @@ void TestExtensionPrefs::RecreateExtensionPrefs() {
     // The PrefService writes its persistent file on the file thread, so we
     // need to wait for any pending I/O to complete before creating a new
     // PrefService.
-    MessageLoop file_loop;
-    BrowserThread file_thread(BrowserThread::FILE, &file_loop);
+    base::WaitableEvent io_finished(false, false);
     pref_service_->SavePersistentPrefs();
-    file_loop.RunAllPending();
+    EXPECT_TRUE(BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                                        new SignalingTask(&io_finished)));
+
+    // If the FILE thread is in fact the current thread (possible in testing
+    // scenarios), we have to ensure the task has a chance to run. If the FILE
+    // thread is a different thread, the test must ensure that thread is running
+    // (otherwise the Wait below will hang).
+    MessageLoop::current()->RunAllPending();
+
+    EXPECT_TRUE(io_finished.Wait());
   }
 
   extension_pref_value_map_.reset(new ExtensionPrefValueMap);
diff --git a/chrome/browser/profiles/profile_manager_unittest.cc b/chrome/browser/profiles/profile_manager_unittest.cc
index 57bba48..81c99bb 100644
--- a/chrome/browser/profiles/profile_manager_unittest.cc
+++ b/chrome/browser/profiles/profile_manager_unittest.cc
@@ -20,7 +20,9 @@
 
 class ProfileManagerTest : public testing::Test {
  protected:
-  ProfileManagerTest() : ui_thread_(BrowserThread::UI, &message_loop_) {
+  ProfileManagerTest()
+      : ui_thread_(BrowserThread::UI, &message_loop_),
+        file_thread_(BrowserThread::FILE, &message_loop_) {
   }
 
   virtual void SetUp() {
@@ -40,6 +42,7 @@ class ProfileManagerTest : public testing::Test {
 
   MessageLoopForUI message_loop_;
   BrowserThread ui_thread_;
+  BrowserThread file_thread_;
 
   // the path to temporary directory used to contain the test operations
   FilePath test_dir_;
@@ -140,8 +143,13 @@ TEST_F(ProfileManagerTest, CreateAndUseTwoProfiles) {
   EXPECT_TRUE(profile1->GetBookmarkModel());
   EXPECT_TRUE(profile2->GetBookmarkModel());
   EXPECT_TRUE(profile2->GetHistoryService(Profile::EXPLICIT_ACCESS));
+
+  // Make sure any pending tasks run before we destroy the profiles.
+  message_loop_.RunAllPending();
+
   profile1.reset();
   profile2.reset();
+
   // Make sure history cleans up correctly.
   message_loop_.RunAllPending();
 }
diff --git a/chrome/browser/sync/sync_setup_wizard_unittest.cc b/chrome/browser/sync/sync_setup_wizard_unittest.cc
index d705706..a97c3f7 100644
--- a/chrome/browser/sync/sync_setup_wizard_unittest.cc
+++ b/chrome/browser/sync/sync_setup_wizard_unittest.cc
@@ -178,8 +178,7 @@ class TestBrowserWindowForWizardTest : public TestBrowserWindow {
 class SyncSetupWizardTest : public BrowserWithTestWindowTest {
  public:
   SyncSetupWizardTest()
-      : file_thread_(BrowserThread::FILE, MessageLoop::current()),
-        test_window_(NULL),
+      : test_window_(NULL),
         wizard_(NULL) { }
   virtual ~SyncSetupWizardTest() { }
   virtual void SetUp() {
@@ -203,7 +202,6 @@ class SyncSetupWizardTest : public BrowserWithTestWindowTest {
     wizard_.reset();
   }
 
-  BrowserThread file_thread_;
   TestBrowserWindowForWizardTest* test_window_;
   scoped_ptr<SyncSetupWizard> wizard_;
   ProfileSyncServiceForWizardTest* service_;
diff --git a/chrome/browser/ui/cocoa/browser_test_helper.cc b/chrome/browser/ui/cocoa/browser_test_helper.cc
index 3e046e3..3aa8fe9 100644
--- a/chrome/browser/ui/cocoa/browser_test_helper.cc
+++ b/chrome/browser/ui/cocoa/browser_test_helper.cc
@@ -27,12 +27,15 @@ BrowserTestHelper::~BrowserTestHelper() {
   // Delete the testing profile on the UI thread. But first release the
   // browser, since it may trigger accesses to the profile upon destruction.
   browser_.reset();
+  message_loop_.DeleteSoon(FROM_HERE, profile_.release());
+
+  // Make sure any pending tasks run before we destroy other threads.
+  message_loop_.RunAllPending();
 
   // Drop any new tasks for the IO and FILE threads.
   io_thread_.reset();
   file_thread_.reset();
 
-  message_loop_.DeleteSoon(FROM_HERE, profile_.release());
   message_loop_.RunAllPending();
 }
 
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
index 8fdcaa1..b055241 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
@@ -9,16 +9,7 @@
 #include "chrome/test/testing_profile.h"
 #include "content/browser/browser_thread.h"
 
-class BookmarkBarViewTest : public BrowserWithTestWindowTest {
- public:
-  BookmarkBarViewTest()
-      : file_thread_(BrowserThread::FILE, message_loop()) {}
-
- private:
-  BrowserThread file_thread_;
-
-  DISALLOW_COPY_AND_ASSIGN(BookmarkBarViewTest);
-};
+typedef BrowserWithTestWindowTest BookmarkBarViewTest;
 
 TEST_F(BookmarkBarViewTest, SwitchProfile) {
   profile()->CreateBookmarkModel(true);
diff --git a/chrome/common/important_file_writer.cc b/chrome/common/important_file_writer.cc
index ceac3fde..661f4ee 100644
--- a/chrome/common/important_file_writer.cc
+++ b/chrome/common/important_file_writer.cc
@@ -106,10 +106,19 @@ void ImportantFileWriter::WriteNow(const std::string& data) {
   if (HasPendingWrite())
     timer_.Stop();
 
-  // TODO(sanjeevr): Add a DCHECK for the return value of PostTask.
-  // (Some tests fail if we add the DCHECK and they need to be fixed first).
-  file_message_loop_proxy_->PostTask(FROM_HERE,
-                                     new WriteToDiskTask(path_, data));
+  if (!file_message_loop_proxy_->PostTask(FROM_HERE,
+                                          new WriteToDiskTask(path_, data))) {
+    // Posting the task to background message loop is not expected
+    // to fail, but if it does, avoid losing data and just hit the disk
+    // on the current thread.
+    // TODO(phajdan.jr): Fix test failures on Win and enable code below.
+#if !defined(OS_WIN)
+    NOTREACHED();
+
+    WriteToDiskTask write_task(path_, data);
+    write_task.Run();
+#endif
+  }
 }
 
 void ImportantFileWriter::ScheduleWrite(DataSerializer* serializer) {
diff --git a/chrome/test/browser_with_test_window_test.cc b/chrome/test/browser_with_test_window_test.cc
index c99e84ec..d4ef1a3 100644
--- a/chrome/test/browser_with_test_window_test.cc
+++ b/chrome/test/browser_with_test_window_test.cc
@@ -21,6 +21,7 @@
 
 BrowserWithTestWindowTest::BrowserWithTestWindowTest()
     : ui_thread_(BrowserThread::UI, message_loop()),
+      file_thread_(BrowserThread::FILE, message_loop()),
       rph_factory_(),
       rvh_factory_(&rph_factory_) {
 #if defined(OS_WIN)
diff --git a/chrome/test/browser_with_test_window_test.h b/chrome/test/browser_with_test_window_test.h
index 2f55042..957e996 100644
--- a/chrome/test/browser_with_test_window_test.h
+++ b/chrome/test/browser_with_test_window_test.h
@@ -92,6 +92,7 @@ class BrowserWithTestWindowTest : public TestingBrowserProcessTest {
   // We need to create a MessageLoop, otherwise a bunch of things fails.
   MessageLoopForUI ui_loop_;
   BrowserThread ui_thread_;
+  BrowserThread file_thread_;
 
   scoped_ptr<TestingProfile> profile_;
   scoped_ptr<TestBrowserWindow> window_;