Move SafeBrowsing to the blocking pool via an experiment.

chrome/browser/safe_browsing/safe_browsing_database.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_
 #define CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_
 
 #include <map>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/containers/hash_tables.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/sequenced_task_runner.h"
 #include "base/synchronization/lock.h"
-#include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/safe_browsing_store.h"
 
 namespace safe_browsing {
 class PrefixSet;
 }
 
 class GURL;
 class SafeBrowsingDatabase;
 
 // Factory for creating SafeBrowsingDatabase. Tests implement this factory
 // to create fake Databases for testing.
 class SafeBrowsingDatabaseFactory {
  public:
   SafeBrowsingDatabaseFactory() { }
   virtual ~SafeBrowsingDatabaseFactory() { }
   virtual SafeBrowsingDatabase* CreateSafeBrowsingDatabase(
+      scoped_refptr<base::SequencedTaskRunner> db_task_runner,

[grt (UTC plus 2), 2015/02/23 20:37:53]

      const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
here and everywhere else so that the refcount isn't bumped and dropped
needlessly for the function calls.

[Alexei Svitkine (slow), 2015/02/24 18:08:58]

I only added it to the function params, not the members.

I believe it's not safe to add it to the members, since SB can be started and
stopped - and we clear the refptr in database_manager.cc when stopped. So this
would make the const refs invalid.

(While tasks may still be running.) Note: See previous discussion around
SKIP_ON_SHUTDOWN upthread - for details - basically in the new code, we don't
want to synchronously execute all the remaining tasks on IO thread - which the
old code does when stopping the thread. (We can monitor the experiment to ensure
there are no problems with this semantic change - and fix any if they arise.)

[grt (UTC plus 2), 2015/02/24 18:26:55]

This is correct, thanks. Passing them around as arguments should always be done
by const ref, as the caller's reference to the object is sufficient to keep it
alive for the duration of the function call. It is the callee's responsibility
to hold its own scoped_refptr that lives beyond the function call if needed.

       bool enable_download_protection,
       bool enable_client_side_whitelist,
       bool enable_download_whitelist,
       bool enable_extension_blacklist,
       bool enable_side_effect_free_whitelist,
       bool enable_ip_blacklist,
       bool enable_unwanted_software_list) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingDatabaseFactory);
@@ skipping 10 matching lines @@
 // as phishing by the client-side phishing detection. These on-disk databases
 // are shared among all profiles, as it doesn't contain user-specific data. This
 // object is not thread-safe, i.e. all its methods should be used on the same
 // thread that it was created on, unless specified otherwise.
 class SafeBrowsingDatabase {
  public:
   // Factory method for obtaining a SafeBrowsingDatabase implementation.
   // It is not thread safe.
   // The browse list and off-domain inclusion whitelist are always on;
   // availability of other lists is controlled by the flags on this method.
-  static SafeBrowsingDatabase* Create(bool enable_download_protection,
-                                      bool enable_client_side_whitelist,
-                                      bool enable_download_whitelist,
-                                      bool enable_extension_blacklist,
-                                      bool side_effect_free_whitelist,
-                                      bool enable_ip_blacklist,
-                                      bool enable_unwanted_software_list);
+  static SafeBrowsingDatabase* Create(
+      scoped_refptr<base::SequencedTaskRunner> db_task_runner,
+      bool enable_download_protection,
+      bool enable_client_side_whitelist,
+      bool enable_download_whitelist,
+      bool enable_extension_blacklist,
+      bool side_effect_free_whitelist,
+      bool enable_ip_blacklist,
+      bool enable_unwanted_software_list);
 
   // Makes the passed |factory| the factory used to instantiate
   // a SafeBrowsingDatabase. This is used for tests.
   static void RegisterFactory(SafeBrowsingDatabaseFactory* factory) {
     factory_ = factory;
   }
 
   virtual ~SafeBrowsingDatabase();
 
   // Initializes the database with the given filename.
@@ skipping 204 matching lines @@
   // Useful for tests, so they can provide their own implementation of
   // SafeBrowsingDatabase.
   static SafeBrowsingDatabaseFactory* factory_;
 };
 
 class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
  public:
   // Create a database with the stores below. Takes ownership of all store
   // objects handed to this constructor. Ignores all future operations on lists
   // for which the store is initialized to NULL.
-  SafeBrowsingDatabaseNew(SafeBrowsingStore* browse_store,
-                          SafeBrowsingStore* download_store,
-                          SafeBrowsingStore* csd_whitelist_store,
-                          SafeBrowsingStore* download_whitelist_store,
-                          SafeBrowsingStore* inclusion_whitelist_store,
-                          SafeBrowsingStore* extension_blacklist_store,
-                          SafeBrowsingStore* side_effect_free_whitelist_store,
-                          SafeBrowsingStore* ip_blacklist_store,
-                          SafeBrowsingStore* unwanted_software_store);
-
-  // Create a database with a browse store. This is a legacy interface that
-  // useds Sqlite.
-  SafeBrowsingDatabaseNew();
+  SafeBrowsingDatabaseNew(
+      scoped_refptr<base::SequencedTaskRunner> db_task_runner,
+      SafeBrowsingStore* browse_store,
+      SafeBrowsingStore* download_store,
+      SafeBrowsingStore* csd_whitelist_store,
+      SafeBrowsingStore* download_whitelist_store,
+      SafeBrowsingStore* inclusion_whitelist_store,
+      SafeBrowsingStore* extension_blacklist_store,
+      SafeBrowsingStore* side_effect_free_whitelist_store,
+      SafeBrowsingStore* ip_blacklist_store,
+      SafeBrowsingStore* unwanted_software_store);
 
   ~SafeBrowsingDatabaseNew() override;
 
   // Implement SafeBrowsingDatabase interface.
   void Init(const base::FilePath& filename) override;
   bool ResetDatabase() override;
   bool ContainsBrowseUrl(const GURL& url,
                          std::vector<SBPrefix>* prefix_hits,
                          std::vector<SBFullHashResult>* cache_hits) override;
   bool ContainsUnwantedSoftwareUrl(
@@ skipping 79 matching lines @@
     // shortest amount of time possible (e.g., release it before computing final
     // results if possible).
     class ReadTransaction;
 
     // Obtained through BeginWriteTransaction(): a WriteTransaction allows
     // modification of the ThreadSafeStateManager's state. It should be used for
     // the shortest amount of time possible (e.g., pre-compute the new state
     // before grabbing a WriteTransaction to swap it in atomically).
     class WriteTransaction;
 
-    explicit ThreadSafeStateManager(const base::ThreadChecker& thread_checker);
+    explicit ThreadSafeStateManager(
+        scoped_refptr<const base::SequencedTaskRunner> db_task_runner);
     ~ThreadSafeStateManager();
 
     scoped_ptr<ReadTransaction> BeginReadTransaction();
     scoped_ptr<ReadTransaction> BeginReadTransactionNoLockOnMainThread();
     scoped_ptr<WriteTransaction> BeginWriteTransaction();
 
    private:
-    // The SafeBrowsingDatabase's ThreadChecker, used to verify that writes are
-    // only made on its main thread. This is important as it allows reading from
-    // the main thread without holding the lock.
-    const base::ThreadChecker& thread_checker_;
+    // The sequenced task runner for this object, used to verify that its state
+    // is only ever accessed from the runner.
+    scoped_refptr<const base::SequencedTaskRunner> db_task_runner_;
 
     // Lock for protecting access to this class' state.
     mutable base::Lock lock_;
 
     SBWhitelist csd_whitelist_;
     SBWhitelist download_whitelist_;
     SBWhitelist inclusion_whitelist_;
 
     // The IP blacklist should be small.  At most a couple hundred IPs.
     IPBlacklist ip_blacklist_;
@@ skipping 20 matching lines @@
   // impl.
   using SBWhitelistId = ThreadSafeStateManager::SBWhitelistId;
   using PrefixSetId = ThreadSafeStateManager::PrefixSetId;
   using ReadTransaction = ThreadSafeStateManager::ReadTransaction;
   using WriteTransaction = ThreadSafeStateManager::WriteTransaction;
 
   // Manages the non-thread safe (i.e. only to be accessed to the database's
   // main thread) state of this class.
   class DatabaseStateManager {
    public:
-    explicit DatabaseStateManager(const base::ThreadChecker& thread_checker)
-        : thread_checker_(thread_checker),
-          corruption_detected_(false),
-          change_detected_(false) {}
+    explicit DatabaseStateManager(
+        scoped_refptr<const base::SequencedTaskRunner> db_task_runner);
+    ~DatabaseStateManager();
 
     void init_filename_base(const base::FilePath& filename_base) {
-      DCHECK(thread_checker_.CalledOnValidThread());
+      DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
       DCHECK(filename_base_.empty()) << "filename already initialized";
       filename_base_ = filename_base;
     }
 
     const base::FilePath& filename_base() {
-      DCHECK(thread_checker_.CalledOnValidThread());
+      DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
       return filename_base_;
     }
 
     void set_corruption_detected() {
-      DCHECK(thread_checker_.CalledOnValidThread());
+      DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
       corruption_detected_ = true;
     }
 
     void reset_corruption_detected() {
-      DCHECK(thread_checker_.CalledOnValidThread());
+      DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
       corruption_detected_ = false;
     }
 
     bool corruption_detected() {
-      DCHECK(thread_checker_.CalledOnValidThread());
+      DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
       return corruption_detected_;
     }
 
     void set_change_detected() {
-      DCHECK(thread_checker_.CalledOnValidThread());
+      DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
       change_detected_ = true;
     }
 
     void reset_change_detected() {
-      DCHECK(thread_checker_.CalledOnValidThread());
+      DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
       change_detected_ = false;
     }
 
     bool change_detected() {
-      DCHECK(thread_checker_.CalledOnValidThread());
+      DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
       return change_detected_;
     }
 
    private:
-    // The SafeBrowsingDatabase's ThreadChecker, used to verify that this class'
-    // state is only ever accessed from the database's main thread.
-    const base::ThreadChecker& thread_checker_;
+    // The sequenced task runner for this object, used to verify that its state
+    // is only ever accessed from the runner.
+    scoped_refptr<const base::SequencedTaskRunner> db_task_runner_;
 
     // The base filename passed to Init(), used to generate the store and prefix
     // set filenames used to store data on disk.
     base::FilePath filename_base_;
 
     // Set if corruption is detected during the course of an update.
     // Causes the update functions to fail with no side effects, until
     // the next call to |UpdateStarted()|.
     bool corruption_detected_;
 
@@ skipping 106 matching lines @@
 
   // Returns a raw pointer to ThreadSafeStateManager's PrefixGetHashCache for
   // testing. This should only be used in unit tests (where multi-threading and
   // synchronization are not problematic).
   PrefixGetHashCache* GetUnsynchronizedPrefixGetHashCacheForTesting();
 
   // Records a file size histogram for the database or PrefixSet backed by
   // |filename|.
   void RecordFileSizeHistogram(const base::FilePath& file_path);
 
-  base::ThreadChecker thread_checker_;
+  // The sequenced task runner for this object, used to verify that its state
+  // is only ever accessed from the runner and post some tasks to it.
+  scoped_refptr<base::SequencedTaskRunner> db_task_runner_;
 
   ThreadSafeStateManager state_manager_;
 
   DatabaseStateManager db_state_manager_;
 
   // Underlying persistent stores for chunk data:
   //   - |browse_store_|: For browsing related (phishing and malware URLs)
   //     chunks and prefixes.
   //   - |download_store_|: For download related (download URL and binary hash)
   //     chunks and prefixes.
@@ skipping 25 matching lines @@
   const scoped_ptr<SafeBrowsingStore> ip_blacklist_store_;
   const scoped_ptr<SafeBrowsingStore> unwanted_software_store_;
 
   // Used to schedule resetting the database because of corruption. This factory
   // and the WeakPtrs it issues should only be used on the database's main
   // thread.
   base::WeakPtrFactory<SafeBrowsingDatabaseNew> reset_factory_;
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_