Move SafeBrowsing to the blocking pool via an experiment.

chrome/browser/safe_browsing/safe_browsing_database.cc

Index: chrome/browser/safe_browsing/safe_browsing_database.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index 4690bf37b5755d669ed1f72c740c200ba8c2a986..4ccb144fabb5b93f1e6a582ef125d0803a3a3533 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -317,12 +317,21 @@ bool GetCachedFullHash(std::map<SBPrefix, SBCachedFullHashResult>* cache,
   return true;
 }
 
+SafeBrowsingStoreFile* CreateStore(
+    bool enable,
+    scoped_refptr<base::SequencedTaskRunner> task_runner) {
+  if (!enable)
+    return nullptr;
+  return new SafeBrowsingStoreFile(task_runner);
+}
+
 }  // namespace
 
 // The default SafeBrowsingDatabaseFactory.
 class SafeBrowsingDatabaseFactoryImpl : public SafeBrowsingDatabaseFactory {
  public:
   SafeBrowsingDatabase* CreateSafeBrowsingDatabase(
+      scoped_refptr<base::SequencedTaskRunner> db_task_runner,
       bool enable_download_protection,
       bool enable_client_side_whitelist,
       bool enable_download_whitelist,
@@ -331,15 +340,15 @@ class SafeBrowsingDatabaseFactoryImpl : public SafeBrowsingDatabaseFactory {
       bool enable_ip_blacklist,
       bool enable_unwanted_software_list) override {
     return new SafeBrowsingDatabaseNew(
-        new SafeBrowsingStoreFile,  // browse_store
-        enable_download_protection ? new SafeBrowsingStoreFile : NULL,
-        enable_client_side_whitelist ? new SafeBrowsingStoreFile : NULL,
-        enable_download_whitelist ? new SafeBrowsingStoreFile : NULL,
-        new SafeBrowsingStoreFile,  // inclusion_whitelist_store
-        enable_extension_blacklist ? new SafeBrowsingStoreFile : NULL,
-        enable_side_effect_free_whitelist ? new SafeBrowsingStoreFile : NULL,
-        enable_ip_blacklist ? new SafeBrowsingStoreFile : NULL,
-        enable_unwanted_software_list ? new SafeBrowsingStoreFile : NULL);
+        db_task_runner, CreateStore(true, db_task_runner),  // browse_store
+        CreateStore(enable_download_protection, db_task_runner),
+        CreateStore(enable_client_side_whitelist, db_task_runner),
+        CreateStore(enable_download_whitelist, db_task_runner),
+        CreateStore(true, db_task_runner),  // inclusion_whitelist_store
+        CreateStore(enable_extension_blacklist, db_task_runner),
+        CreateStore(enable_side_effect_free_whitelist, db_task_runner),
+        CreateStore(enable_ip_blacklist, db_task_runner),
+        CreateStore(enable_unwanted_software_list, db_task_runner));
   }
 
   SafeBrowsingDatabaseFactoryImpl() { }
@@ -357,6 +366,7 @@ SafeBrowsingDatabaseFactory* SafeBrowsingDatabase::factory_ = NULL;
 // SafeBrowsingDatabaseNew to SafeBrowsingDatabase, and have Create()
 // callers just construct things directly.
 SafeBrowsingDatabase* SafeBrowsingDatabase::Create(
+    scoped_refptr<base::SequencedTaskRunner> current_task_runner,
     bool enable_download_protection,
     bool enable_client_side_whitelist,
     bool enable_download_whitelist,
@@ -366,13 +376,11 @@ SafeBrowsingDatabase* SafeBrowsingDatabase::Create(
     bool enable_unwanted_software_list) {
   if (!factory_)
     factory_ = new SafeBrowsingDatabaseFactoryImpl();
-  return factory_->CreateSafeBrowsingDatabase(enable_download_protection,
-                                              enable_client_side_whitelist,
-                                              enable_download_whitelist,
-                                              enable_extension_blacklist,
-                                              enable_side_effect_free_whitelist,
-                                              enable_ip_blacklist,
-                                              enable_unwanted_software_list);
+  return factory_->CreateSafeBrowsingDatabase(
+      current_task_runner, enable_download_protection,
+      enable_client_side_whitelist, enable_download_whitelist,
+      enable_extension_blacklist, enable_side_effect_free_whitelist,
+      enable_ip_blacklist, enable_unwanted_software_list);
 }
 
 SafeBrowsingDatabase::~SafeBrowsingDatabase() {
@@ -446,7 +454,7 @@ base::FilePath SafeBrowsingDatabase::UnwantedSoftwareDBFilename(
 
 SafeBrowsingStore* SafeBrowsingDatabaseNew::GetStore(const int list_id) {
   // Stores are not thread safe.
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   if (list_id == safe_browsing_util::PHISH ||
       list_id == safe_browsing_util::MALWARE) {
@@ -535,7 +543,7 @@ class SafeBrowsingDatabaseNew::ThreadSafeStateManager::ReadTransaction {
     if (auto_lock_requirement == AutoLockRequirement::LOCK)
       transaction_lock_.reset(new base::AutoLock(outer_->lock_));
     else
-      DCHECK(outer_->thread_checker_.CalledOnValidThread());
+      DCHECK(outer_->db_task_runner_->RunsTasksOnCurrentThread());
   }
 
   const ThreadSafeStateManager* outer_;
@@ -591,7 +599,7 @@ class SafeBrowsingDatabaseNew::ThreadSafeStateManager::WriteTransaction {
   explicit WriteTransaction(ThreadSafeStateManager* outer)
       : outer_(outer), transaction_lock_(outer_->lock_) {
     DCHECK(outer_);
-    DCHECK(outer_->thread_checker_.CalledOnValidThread());
+    DCHECK(outer_->db_task_runner_->RunsTasksOnCurrentThread());
   }
 
   SBWhitelist* SBWhitelistForId(SBWhitelistId id) {
@@ -614,8 +622,8 @@ class SafeBrowsingDatabaseNew::ThreadSafeStateManager::WriteTransaction {
 };
 
 SafeBrowsingDatabaseNew::ThreadSafeStateManager::ThreadSafeStateManager(
-    const base::ThreadChecker& thread_checker)
-    : thread_checker_(thread_checker) {
+    scoped_refptr<base::SequencedTaskRunner> db_task_runner)
+    : db_task_runner_(db_task_runner) {
 }
 
 SafeBrowsingDatabaseNew::ThreadSafeStateManager::~ThreadSafeStateManager() {
@@ -638,28 +646,8 @@ SafeBrowsingDatabaseNew::ThreadSafeStateManager::BeginWriteTransaction() {
   return make_scoped_ptr(new WriteTransaction(this));
 }
 
-SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew()
-    : SafeBrowsingDatabaseNew(new SafeBrowsingStoreFile,  // browse_store
-                              NULL,    // download_store
-                              NULL,    // csd_whitelist_store
-                              NULL,    // download_whitelist_store
-                              NULL,    // inclusion_whitelist_store
-                              NULL,    // extension_blacklist_store
-                              NULL,    // side_effect_free_whitelist_store
-                              NULL,    // ip_blacklist_store
-                              NULL) {  // unwanted_software_store
-  DCHECK(browse_store_.get());
-  DCHECK(!download_store_.get());
-  DCHECK(!csd_whitelist_store_.get());
-  DCHECK(!download_whitelist_store_.get());
-  DCHECK(!inclusion_whitelist_store_.get());
-  DCHECK(!extension_blacklist_store_.get());
-  DCHECK(!side_effect_free_whitelist_store_.get());
-  DCHECK(!ip_blacklist_store_.get());
-  DCHECK(!unwanted_software_store_.get());
-}
-
 SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew(
+    scoped_refptr<base::SequencedTaskRunner> db_task_runner,
     SafeBrowsingStore* browse_store,
     SafeBrowsingStore* download_store,
     SafeBrowsingStore* csd_whitelist_store,
@@ -669,8 +657,9 @@ SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew(
     SafeBrowsingStore* side_effect_free_whitelist_store,
     SafeBrowsingStore* ip_blacklist_store,
     SafeBrowsingStore* unwanted_software_store)
-    : state_manager_(thread_checker_),
-      db_state_manager_(thread_checker_),
+    : db_task_runner_(db_task_runner),
+      state_manager_(db_task_runner_),
+      db_state_manager_(db_task_runner_),
       browse_store_(browse_store),
       download_store_(download_store),
       csd_whitelist_store_(csd_whitelist_store),
@@ -686,11 +675,11 @@ SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew(
 
 SafeBrowsingDatabaseNew::~SafeBrowsingDatabaseNew() {
   // The DCHECK is disabled due to crbug.com/338486 .
-  // DCHECK(thread_checker_.CalledOnValidThread());
+  // DDCHECK(db_task_runner_->RunsTasksOnCurrentThread());

[gab, 2015/02/19 14:38:23]

s/DDCHECK/DCHECK

[Alexei Svitkine (slow), 2015/02/20 15:42:44]

Done.

 }
 
 void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   db_state_manager_.init_filename_base(filename_base);
 
@@ -840,7 +829,7 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
 }
 
 bool SafeBrowsingDatabaseNew::ResetDatabase() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   // Delete files on disk.
   // TODO(shess): Hard to see where one might want to delete without a
@@ -876,6 +865,16 @@ bool SafeBrowsingDatabaseNew::ContainsUnwantedSoftwareUrl(
                               cache_hits);
 }
 
+SafeBrowsingDatabaseNew::DatabaseStateManager::DatabaseStateManager(

[gab, 2015/02/19 14:38:23]

Please insert these two just below ~ThreadSafeStateManager(){} above on line 630
(I know this is not exactly where they line up in the header, but when I added
those I decided to put them between SafeBrowsingDatabase:: and
SafeBrowsingDatabaseNew:: implementation as otherwise having an implementation
in the middle of another one felt weird).

(and of course I just realized that SafeBrowsingDatabaseNew::GetStore() is
misaligned, screwing most of that up anyways...)

[Alexei Svitkine (slow), 2015/02/20 15:42:44]

Done.

+    scoped_refptr<base::SequencedTaskRunner> db_task_runner)
+    : db_task_runner_(db_task_runner),
+      corruption_detected_(false),
+      change_detected_(false) {
+}
+
+SafeBrowsingDatabaseNew::DatabaseStateManager::~DatabaseStateManager() {
+}
+
 bool SafeBrowsingDatabaseNew::PrefixSetContainsUrl(
     const GURL& url,
     PrefixSetId prefix_set_id,
@@ -940,7 +939,7 @@ bool SafeBrowsingDatabaseNew::PrefixSetContainsUrlHashes(
 bool SafeBrowsingDatabaseNew::ContainsDownloadUrl(
     const std::vector<GURL>& urls,
     std::vector<SBPrefix>* prefix_hits) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   // Ignore this check when download checking is not enabled.
   if (!download_store_.get())
@@ -975,7 +974,7 @@ bool SafeBrowsingDatabaseNew::ContainsInclusionWhitelistedUrl(const GURL& url) {
 bool SafeBrowsingDatabaseNew::ContainsExtensionPrefixes(
     const std::vector<SBPrefix>& prefixes,
     std::vector<SBPrefix>* prefix_hits) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   if (!extension_blacklist_store_)
     return false;
@@ -1072,7 +1071,7 @@ void SafeBrowsingDatabaseNew::InsertAddChunk(
     SafeBrowsingStore* store,
     const safe_browsing_util::ListType list_id,
     const SBChunkData& chunk_data) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(store);
 
   // The server can give us a chunk that we already have because
@@ -1101,7 +1100,7 @@ void SafeBrowsingDatabaseNew::InsertSubChunk(
     SafeBrowsingStore* store,
     const safe_browsing_util::ListType list_id,
     const SBChunkData& chunk_data) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(store);
 
   // The server can give us a chunk that we already have because
@@ -1134,7 +1133,7 @@ void SafeBrowsingDatabaseNew::InsertSubChunk(
 void SafeBrowsingDatabaseNew::InsertChunks(
     const std::string& list_name,
     const std::vector<SBChunkData*>& chunks) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   if (db_state_manager_.corruption_detected() || chunks.empty())
     return;
@@ -1169,7 +1168,7 @@ void SafeBrowsingDatabaseNew::InsertChunks(
 
 void SafeBrowsingDatabaseNew::DeleteChunks(
     const std::vector<SBChunkDelete>& chunk_deletes) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   if (db_state_manager_.corruption_detected() || chunk_deletes.empty())
     return;
@@ -1220,7 +1219,7 @@ void SafeBrowsingDatabaseNew::CacheHashResults(
 
 bool SafeBrowsingDatabaseNew::UpdateStarted(
     std::vector<SBListChunkRanges>* lists) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(lists);
 
   // If |BeginUpdate()| fails, reset the database.
@@ -1326,7 +1325,7 @@ bool SafeBrowsingDatabaseNew::UpdateStarted(
 }
 
 void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   // The update may have failed due to corrupt storage (for instance,
   // an excessive number of invalid add_chunks and sub_chunks).
@@ -1458,7 +1457,7 @@ void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
     SBWhitelistId whitelist_id) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   if (!store)
     return;
@@ -1486,7 +1485,7 @@ void SafeBrowsingDatabaseNew::UpdateHashPrefixStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
     FailureType failure_type) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   // These results are not used after this call. Simply ignore the
   // returned value after FinishUpdate(...).
@@ -1510,7 +1509,7 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
     FailureType finish_failure_type,
     FailureType write_failure_type,
     bool store_full_hashes_in_prefix_set) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(url_store);
 
   // Measure the amount of IO during the filter build.
@@ -1590,7 +1589,7 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
 }
 
 void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   // Note: prefixes will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
@@ -1615,20 +1614,20 @@ void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
 }
 
 void SafeBrowsingDatabaseNew::HandleCorruptDatabase() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   // Reset the database after the current task has unwound (but only
   // reset once within the scope of a given task).
   if (!reset_factory_.HasWeakPtrs()) {
     RecordFailure(FAILURE_DATABASE_CORRUPT);
-    base::MessageLoop::current()->PostTask(FROM_HERE,
-        base::Bind(&SafeBrowsingDatabaseNew::OnHandleCorruptDatabase,
-                   reset_factory_.GetWeakPtr()));
+    db_task_runner_->PostTask(

[gab, 2015/02/19 14:41:20]

Oh also, interesting that this posts back to itself, I thought the comments on
safe_browsing_thread_ said this wasn't allowed.. (just an observation, nothing
you're making worse FWIW).

[Alexei Svitkine (slow), 2015/02/20 15:42:44]

Acknowledged.

+        FROM_HERE, base::Bind(&SafeBrowsingDatabaseNew::OnHandleCorruptDatabase,
+                              reset_factory_.GetWeakPtr()));
   }
 }
 
 void SafeBrowsingDatabaseNew::OnHandleCorruptDatabase() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   RecordFailure(FAILURE_DATABASE_CORRUPT_HANDLER);
   db_state_manager_.set_corruption_detected();  // Stop updating the database.
@@ -1647,7 +1646,7 @@ void SafeBrowsingDatabaseNew::LoadPrefixSet(const base::FilePath& db_filename,
                                             WriteTransaction* txn,
                                             PrefixSetId prefix_set_id,
                                             FailureType read_failure_type) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(txn);
   DCHECK(!db_state_manager_.filename_base().empty());
 
@@ -1671,7 +1670,7 @@ void SafeBrowsingDatabaseNew::LoadPrefixSet(const base::FilePath& db_filename,
 }
 
 bool SafeBrowsingDatabaseNew::Delete() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(!db_state_manager_.filename_base().empty());
 
   // TODO(shess): This is a mess.  SafeBrowsingFileStore::Delete() closes the
@@ -1760,7 +1759,7 @@ bool SafeBrowsingDatabaseNew::Delete() {
 void SafeBrowsingDatabaseNew::WritePrefixSet(const base::FilePath& db_filename,
                                              PrefixSetId prefix_set_id,
                                              FailureType write_failure_type) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   // Do not grab the lock to avoid contention while writing to disk. This is
   // safe as only this thread can ever modify |state_manager_|'s prefix sets
@@ -1791,7 +1790,7 @@ void SafeBrowsingDatabaseNew::WritePrefixSet(const base::FilePath& db_filename,
 void SafeBrowsingDatabaseNew::LoadWhitelist(
     const std::vector<SBAddFullHash>& full_hashes,
     SBWhitelistId whitelist_id) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   if (full_hashes.size() > kMaxWhitelistSize) {
     state_manager_.BeginWriteTransaction()->WhitelistEverything(whitelist_id);
@@ -1819,7 +1818,7 @@ void SafeBrowsingDatabaseNew::LoadWhitelist(
 
 void SafeBrowsingDatabaseNew::LoadIpBlacklist(
     const std::vector<SBAddFullHash>& full_hashes) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 
   IPBlacklist new_blacklist;
   for (std::vector<SBAddFullHash>::const_iterator it = full_hashes.begin();