Fix threading issues in BlacklistManager, using new ChromeThread.

TEST=Covered by unit_tests and browser_tests.
BUG=21541

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

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

4 files changed, 108 insertions, 200 deletions

diff --git a/chrome/browser/privacy_blacklist/blacklist_manager.cc b/chrome/browser/privacy_blacklist/blacklist_manager.cc
index 0629806..f71a26f 100644
--- a/chrome/browser/privacy_blacklist/blacklist_manager.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_manager.cc
@@ -16,134 +16,18 @@
 #include "chrome/common/notification_source.h"
 #include "chrome/common/notification_type.h"
 
-// Base class for tasks that use BlacklistManager. It ensures that the
-// BlacklistManager won't get destroyed while we need it, and that it
-// will be destroyed always on the same thread. That's why we use
-// a custom Task instead of just NewRunnableMethod.
-class BlacklistManagerTask : public Task {
- public:
-  explicit BlacklistManagerTask(BlacklistManager* manager)
-      : original_loop_(MessageLoop::current()),
-        manager_(manager) {
-    DCHECK(original_loop_);
-    manager->AddRef();
-  }
-
-  ~BlacklistManagerTask() {
-    original_loop_->ReleaseSoon(FROM_HERE, manager_);
-  }
-
- protected:
-  BlacklistManager* blacklist_manager() const { return manager_; }
-
-  MessageLoop* original_loop_;
-
- private:
-  BlacklistManager* manager_;
-
-  DISALLOW_COPY_AND_ASSIGN(BlacklistManagerTask);
-};
-
 BlacklistPathProvider::~BlacklistPathProvider() {
 }
 
-class BlacklistManager::CompileBlacklistTask : public BlacklistManagerTask {
- public:
-  CompileBlacklistTask(BlacklistManager* manager,
-                       const FilePath& destination_blacklist,
-                       const std::vector<FilePath>& source_blacklists)
-      : BlacklistManagerTask(manager),
-        destination_blacklist_(destination_blacklist),
-        source_blacklists_(source_blacklists) {
-  }
-
-  virtual void Run() {
-    bool success = true;
-
-    Blacklist blacklist;
-    std::string error_string;
-
-    for (std::vector<FilePath>::const_iterator i = source_blacklists_.begin();
-         i != source_blacklists_.end(); ++i) {
-      if (!BlacklistIO::ReadText(&blacklist, *i, &error_string)) {
-        success = false;
-        break;
-      }
-    }
-
-    // Only overwrite the current compiled blacklist if we read all source
-    // files successfully.
-    if (success)
-      success = BlacklistIO::WriteBinary(&blacklist, destination_blacklist_);
-
-    original_loop_->PostTask(
-        FROM_HERE,
-        NewRunnableMethod(blacklist_manager(),
-                          &BlacklistManager::OnBlacklistCompilationFinished,
-                          success));
-  }
-
- private:
-  FilePath destination_blacklist_;
-
-  std::vector<FilePath> source_blacklists_;
-
-  DISALLOW_COPY_AND_ASSIGN(CompileBlacklistTask);
-};
-
-class BlacklistManager::ReadBlacklistTask : public BlacklistManagerTask {
- public:
-  ReadBlacklistTask(BlacklistManager* manager,
-                    const FilePath& compiled_blacklist,
-                    const std::vector<FilePath>& transient_blacklists)
-      : BlacklistManagerTask(manager),
-        compiled_blacklist_(compiled_blacklist),
-        transient_blacklists_(transient_blacklists) {
-  }
-
-  virtual void Run() {
-    scoped_ptr<Blacklist> blacklist(new Blacklist);
-    if (!BlacklistIO::ReadBinary(blacklist.get(), compiled_blacklist_)) {
-      ReportReadResult(NULL);
-      return;
-    }
-
-    std::string error_string;
-    std::vector<FilePath>::const_iterator i;
-    for (i = transient_blacklists_.begin();
-         i != transient_blacklists_.end(); ++i) {
-      if (!BlacklistIO::ReadText(blacklist.get(), *i, &error_string)) {
-        ReportReadResult(NULL);
-        return;
-      }
-    }
-
-    ReportReadResult(blacklist.release());
-  }
-
- private:
-  void ReportReadResult(Blacklist* blacklist) {
-    original_loop_->PostTask(
-        FROM_HERE, NewRunnableMethod(blacklist_manager(),
-                                     &BlacklistManager::OnBlacklistReadFinished,
-                                     blacklist));
-  }
-
-  FilePath compiled_blacklist_;
-  std::vector<FilePath> transient_blacklists_;
-
-  DISALLOW_COPY_AND_ASSIGN(ReadBlacklistTask);
-};
-
 BlacklistManager::BlacklistManager(Profile* profile,
-                                   BlacklistPathProvider* path_provider,
-                                   base::Thread* backend_thread)
+                                   BlacklistPathProvider* path_provider)
     : first_read_finished_(false),
       profile_(profile),
       compiled_blacklist_path_(
         profile->GetPath().Append(chrome::kPrivacyBlacklistFileName)),
-      path_provider_(path_provider),
-      backend_thread_(backend_thread) {
+      path_provider_(path_provider) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
   registrar_.Add(this,
                  NotificationType::EXTENSION_LOADED,
                  Source<Profile>(profile));
@@ -156,6 +40,8 @@ BlacklistManager::BlacklistManager(Profile* profile,
 void BlacklistManager::Observe(NotificationType type,
                                const NotificationSource& source,
                                const NotificationDetails& details) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
   switch (type.value) {
     case NotificationType::EXTENSION_LOADED:
     case NotificationType::EXTENSION_UNLOADED:
@@ -168,23 +54,42 @@ void BlacklistManager::Observe(NotificationType type,
 }
 
 void BlacklistManager::CompileBlacklist() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
-  RunTaskOnBackendThread(new CompileBlacklistTask(
-      this, compiled_blacklist_path_,
-      path_provider_->GetPersistentBlacklistPaths()));
+  ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
+      NewRunnableMethod(this, &BlacklistManager::DoCompileBlacklist,
+                        path_provider_->GetPersistentBlacklistPaths()));
 }
 
-void BlacklistManager::ReadBlacklist() {
-  DCHECK(CalledOnValidThread());
+void BlacklistManager::DoCompileBlacklist(
+    const std::vector<FilePath>& source_blacklists) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+
+  bool success = true;
+
+  Blacklist blacklist;
+  std::string error_string;
+
+  for (std::vector<FilePath>::const_iterator i = source_blacklists.begin();
+       i != source_blacklists.end(); ++i) {
+    if (!BlacklistIO::ReadText(&blacklist, *i, &error_string)) {
+      success = false;
+      break;
+    }
+  }
 
-  RunTaskOnBackendThread(new ReadBlacklistTask(
-      this, compiled_blacklist_path_,
-      path_provider_->GetTransientBlacklistPaths()));
+  // Only overwrite the current compiled blacklist if we read all source
+  // files successfully.
+  if (success)
+    success = BlacklistIO::WriteBinary(&blacklist, compiled_blacklist_path_);
+
+  ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
+      NewRunnableMethod(this, &BlacklistManager::OnBlacklistCompilationFinished,
+                        success));
 }
 
 void BlacklistManager::OnBlacklistCompilationFinished(bool success) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
   if (success) {
     ReadBlacklist();
@@ -197,8 +102,46 @@ void BlacklistManager::OnBlacklistCompilationFinished(bool success) {
   }
 }
 
+void BlacklistManager::ReadBlacklist() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
+  ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
+      NewRunnableMethod(this, &BlacklistManager::DoReadBlacklist,
+                        path_provider_->GetTransientBlacklistPaths()));
+}
+
+void BlacklistManager::DoReadBlacklist(
+    const std::vector<FilePath>& transient_blacklists) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+
+  scoped_ptr<Blacklist> blacklist(new Blacklist);
+  if (!BlacklistIO::ReadBinary(blacklist.get(), compiled_blacklist_path_)) {
+    ReportBlacklistReadResult(NULL);
+    return;
+  }
+
+  std::string error_string;
+  std::vector<FilePath>::const_iterator i;
+  for (i = transient_blacklists.begin();
+       i != transient_blacklists.end(); ++i) {
+    if (!BlacklistIO::ReadText(blacklist.get(), *i, &error_string)) {
+      ReportBlacklistReadResult(NULL);
+      return;
+    }
+  }
+
+  ReportBlacklistReadResult(blacklist.release());
+}
+
+void BlacklistManager::ReportBlacklistReadResult(Blacklist* blacklist) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
+      NewRunnableMethod(this, &BlacklistManager::OnBlacklistReadFinished,
+                        blacklist));
+}
+
 void BlacklistManager::OnBlacklistReadFinished(Blacklist* blacklist) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
   if (!blacklist) {
     if (!first_read_finished_) {
@@ -223,12 +166,3 @@ void BlacklistManager::OnBlacklistReadFinished(Blacklist* blacklist) {
       Source<Profile>(profile_),
       Details<Blacklist>(blacklist));
 }
-
-void BlacklistManager::RunTaskOnBackendThread(Task* task) {
-  if (backend_thread_) {
-    backend_thread_->message_loop()->PostTask(FROM_HERE, task);
-  } else {
-    task->Run();
-    delete task;
-  }
-}
diff --git a/chrome/browser/privacy_blacklist/blacklist_manager.h b/chrome/browser/privacy_blacklist/blacklist_manager.h
index 80faec5..764e38d 100644
--- a/chrome/browser/privacy_blacklist/blacklist_manager.h
+++ b/chrome/browser/privacy_blacklist/blacklist_manager.h
@@ -9,41 +9,34 @@
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
-#include "base/non_thread_safe.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/common/notification_registrar.h"
 
 class Blacklist;
-class MessageLoop;
 class Profile;
-class Task;
-
-namespace base {
-class Thread;
-}
 
 class BlacklistPathProvider {
  public:
   virtual ~BlacklistPathProvider();
 
+  // The methods below will be invoked on the UI thread.
   virtual std::vector<FilePath> GetPersistentBlacklistPaths() = 0;
-
   virtual std::vector<FilePath> GetTransientBlacklistPaths() = 0;
 };
 
 // Updates one compiled binary blacklist based on a list of plaintext
 // blacklists.
-class BlacklistManager : public base::RefCountedThreadSafe<BlacklistManager>,
-                         public NotificationObserver,
-                         public NonThreadSafe {
+class BlacklistManager
+    : public NotificationObserver,
+      public base::RefCountedThreadSafe<BlacklistManager,
+                                        ChromeThread::DeleteOnUIThread> {
  public:
-  // You must create and destroy BlacklistManager on the same thread.
-  BlacklistManager(Profile* profile,
-                   BlacklistPathProvider* path_provider,
-                   base::Thread* backend_thread);
+  BlacklistManager(Profile* profile, BlacklistPathProvider* path_provider);
 
   const Blacklist* GetCompiledBlacklist() const {
+    // TODO(phajdan.jr): Determine on which thread this should be invoked (IO?).
     return compiled_blacklist_.get();
   }
 
@@ -57,20 +50,22 @@ class BlacklistManager : public base::RefCountedThreadSafe<BlacklistManager>,
 #endif  // UNIT_TEST
 
  private:
-  class CompileBlacklistTask;
-  class ReadBlacklistTask;
-
-  friend class base::RefCountedThreadSafe<BlacklistManager>;
+  friend class ChromeThread;
+  friend class DeleteTask<BlacklistManager>;
 
   ~BlacklistManager() {}
 
+  // Compile all persistent blacklists to one binary blacklist stored on disk.
   void CompileBlacklist();
-  void ReadBlacklist();
-
+  void DoCompileBlacklist(const std::vector<FilePath>& source_blacklists);
   void OnBlacklistCompilationFinished(bool success);
-  void OnBlacklistReadFinished(Blacklist* blacklist);
 
-  void RunTaskOnBackendThread(Task* task);
+  // Read all blacklists from disk (the compiled one and also the transient
+  // blacklists).
+  void ReadBlacklist();
+  void DoReadBlacklist(const std::vector<FilePath>& transient_blacklists);
+  void ReportBlacklistReadResult(Blacklist* blacklist);
+  void OnBlacklistReadFinished(Blacklist* blacklist);
 
   // True after the first blacklist read has finished (regardless of success).
   // Used to avoid an infinite loop.
@@ -86,10 +81,6 @@ class BlacklistManager : public base::RefCountedThreadSafe<BlacklistManager>,
 
   BlacklistPathProvider* path_provider_;
 
-  // Backend thread we will execute I/O operations on (NULL means no separate
-  // thread).
-  base::Thread* backend_thread_;
-
   NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(BlacklistManager);
diff --git a/chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc b/chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc
index d7a71e9..22f7297 100644
--- a/chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc
@@ -33,9 +33,8 @@ class BlacklistManagerBrowserTest : public ExtensionBrowserTest {
  public:
   void InitializeBlacklistManager() {
     Profile* profile = browser()->profile();
-    blacklist_manager_ = new BlacklistManager(
-        profile, profile->GetExtensionsService(),
-        g_browser_process->io_thread());
+    blacklist_manager_ = new BlacklistManager(profile,
+                                              profile->GetExtensionsService());
     WaitForBlacklistUpdate();
   }
 
diff --git a/chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc b/chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc
index e7d9f9d..51814e7 100644
--- a/chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc
@@ -83,12 +83,16 @@ class TestBlacklistPathProvider : public BlacklistPathProvider {
 
 class BlacklistManagerTest : public testing::Test, public NotificationObserver {
  public:
-  BlacklistManagerTest() : path_provider_(&profile_) {
+  BlacklistManagerTest()
+      : path_provider_(&profile_),
+        mock_ui_thread_(ChromeThread::UI, MessageLoop::current()),
+        mock_file_thread_(ChromeThread::FILE) {
   }
 
   virtual void SetUp() {
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_));
     test_data_dir_ = test_data_dir_.AppendASCII("blacklist_samples");
+    ASSERT_TRUE(mock_file_thread_.Start());
   }
 
   virtual void TearDown() {
@@ -127,6 +131,9 @@ class BlacklistManagerTest : public testing::Test, public NotificationObserver {
 
  private:
   MessageLoop loop_;
+
+  ChromeThread mock_ui_thread_;
+  ChromeThread mock_file_thread_;
 };
 
 // Returns true if |blacklist| contains a match for |url|.
@@ -142,7 +149,7 @@ bool BlacklistHasMatch(const Blacklist* blacklist, const char* url) {
 
 TEST_F(BlacklistManagerTest, Basic) {
   scoped_refptr<BlacklistManager> manager(
-      new BlacklistManager(&profile_, &path_provider_, NULL));
+      new BlacklistManager(&profile_, &path_provider_));
   WaitForBlacklistUpdate();
 
   const Blacklist* blacklist = manager->GetCompiledBlacklist();
@@ -154,7 +161,7 @@ TEST_F(BlacklistManagerTest, Basic) {
 
 TEST_F(BlacklistManagerTest, BlacklistPathProvider) {
   scoped_refptr<BlacklistManager> manager(
-      new BlacklistManager(&profile_, &path_provider_, NULL));
+      new BlacklistManager(&profile_, &path_provider_));
   WaitForBlacklistUpdate();
 
   const Blacklist* blacklist1 = manager->GetCompiledBlacklist();
@@ -188,7 +195,7 @@ TEST_F(BlacklistManagerTest, BlacklistPathProvider) {
   path_provider_.clear();
   path_provider_.AddPersistentPath(
       test_data_dir_.AppendASCII("annoying_ads.pbl"));
-  manager = new BlacklistManager(&profile_, &path_provider_, NULL);
+  manager = new BlacklistManager(&profile_, &path_provider_);
   WaitForBlacklistUpdate();
 
   const Blacklist* blacklist4 = manager->GetCompiledBlacklist();
@@ -197,32 +204,9 @@ TEST_F(BlacklistManagerTest, BlacklistPathProvider) {
   EXPECT_FALSE(BlacklistHasMatch(blacklist4, "http://host/other_ads/ad.jpg"));
 }
 
-TEST_F(BlacklistManagerTest, RealThread) {
-  base::Thread backend_thread("backend_thread");
-  backend_thread.Start();
-
-  scoped_refptr<BlacklistManager> manager(
-      new BlacklistManager(&profile_, &path_provider_, &backend_thread));
-  WaitForBlacklistUpdate();
-
-  const Blacklist* blacklist1 = manager->GetCompiledBlacklist();
-  EXPECT_FALSE(BlacklistHasMatch(blacklist1,
-                                 "http://host/annoying_ads/ad.jpg"));
-
-  path_provider_.AddPersistentPath(
-      test_data_dir_.AppendASCII("annoying_ads.pbl"));
-  WaitForBlacklistUpdate();
-
-  const Blacklist* blacklist2 = manager->GetCompiledBlacklist();
-
-  // Added a real blacklist, the manager should recompile.
-  EXPECT_NE(blacklist1, blacklist2);
-  EXPECT_TRUE(BlacklistHasMatch(blacklist2, "http://host/annoying_ads/ad.jpg"));
-}
-
 TEST_F(BlacklistManagerTest, BlacklistPathReadError) {
   scoped_refptr<BlacklistManager> manager(
-      new BlacklistManager(&profile_, &path_provider_, NULL));
+      new BlacklistManager(&profile_, &path_provider_));
   WaitForBlacklistUpdate();
 
   FilePath bogus_path(test_data_dir_.AppendASCII("does_not_exist_randomness"));
@@ -239,7 +223,7 @@ TEST_F(BlacklistManagerTest, CompiledBlacklistReadError) {
 
   {
     scoped_refptr<BlacklistManager> manager(
-        new BlacklistManager(&profile_, &path_provider_, NULL));
+        new BlacklistManager(&profile_, &path_provider_));
     WaitForBlacklistUpdate();
 
     path_provider_.AddPersistentPath(
@@ -257,7 +241,7 @@ TEST_F(BlacklistManagerTest, CompiledBlacklistReadError) {
 
   {
     scoped_refptr<BlacklistManager> manager(
-        new BlacklistManager(&profile_, &path_provider_, NULL));
+        new BlacklistManager(&profile_, &path_provider_));
     WaitForBlacklistUpdate();
 
     // The manager should recompile the blacklist.