Introduce SafeBrowsingDatabase::ThreadSafeStateManager.

chrome/browser/safe_browsing/safe_browsing_database.h

Index: chrome/browser/safe_browsing/safe_browsing_database.h
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.h b/chrome/browser/safe_browsing/safe_browsing_database.h
index 64148ce8aa3a92a30db9aa0135d77a4c6e52b85b..68dea690946034b3bed9172b659946cc9643bd95 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database.h
@@ -117,7 +117,7 @@ class SafeBrowsingDatabase {
 
   // Returns false if none of |urls| are in Download database. If it returns
   // true, |prefix_hits| should contain the prefixes for the URLs that were in
-  // the database.  This function can ONLY be called from the creation thread.
+  // the database.  This function can ONLY be accessed from creation thread.
   virtual bool ContainsDownloadUrl(const std::vector<GURL>& urls,
                                    std::vector<SBPrefix>* prefix_hits) = 0;
 
@@ -140,7 +140,7 @@ class SafeBrowsingDatabase {
   // Populates |prefix_hits| with any prefixes in |prefixes| that have matches
   // in the database.
   //
-  // This function can ONLY be called from the creation thread.
+  // This function can ONLY be accessed from the creation thread.
   virtual bool ContainsExtensionPrefixes(
       const std::vector<SBPrefix>& prefixes,
       std::vector<SBPrefix>* prefix_hits) = 0;
@@ -367,11 +367,98 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // IPv6 IP prefix using SHA-1.
   typedef std::map<std::string, base::hash_set<std::string> > IPBlacklist;
 
-  bool PrefixSetContainsUrl(
-      const GURL& url,
-      scoped_ptr<const safe_browsing::PrefixSet>* prefix_set_getter,
-      std::vector<SBPrefix>* prefix_hits,
-      std::vector<SBFullHashResult>* cache_hits);
+  typedef std::map<SBPrefix, SBCachedFullHashResult> PrefixGetHashCache;
+
+  // The ThreadSafeStateManager holds the SafeBrowsingDatabase's state which
+  // must be accessed in a thread-safe fashion. It must be constructed on the
+  // SafeBrowsingDatabaseManager's main thread. The main thread will then be the
+  // only thread on which this state can be modified; allowing for unlocked
+  // reads on the main thread and thus avoiding contention while performing
+  // intensive operations such as writing that state to disk. The state can only
+  // be accessed via (Read|Write)Transactions obtained through this class which
+  // will automatically handle thread-safety.
+  class ThreadSafeStateManager {
+   public:
+    // Identifiers for stores held by the ThreadSafeStateManager. Allows helper
+    // methods to start a transaction themselves and keep it as short as
+    // possible rather than force callers to start the transaction early to pass
+    // a store pointer to the said helper methods.
+    enum class SBWhitelistId {
+      CSD,
+      DOWNLOAD,
+    };
+    enum class PrefixSetId {
+      BROWSE,
+      SIDE_EFFECT_FREE_WHITELIST,
+      UNWANTED_SOFTWARE,
+    };
+
+    // Obtained through BeginReadTransaction(NoLockOnMainThread)?(): a
+    // ReadTransaction allows read-only observations of the
+    // ThreadSafeStateManager's state. The |prefix_gethash_cache_| has a special
+    // allowance to be writable from a ReadTransaction but can't benefit from
+    // unlocked ReadTransactions. ReadTransaction should be held for the
+    // 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);
+    ~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_;
+
+    // Lock for protecting access to this class' state.
+    mutable base::Lock lock_;
+
+    SBWhitelist csd_whitelist_;
+    SBWhitelist download_whitelist_;
+
+    // The IP blacklist should be small.  At most a couple hundred IPs.
+    IPBlacklist ip_blacklist_;
+
+    // PrefixSets to speed up lookups for particularly large lists. The
+    // PrefixSet themselves are never modified, instead a new one is swapped in
+    // on update.
+    scoped_ptr<const safe_browsing::PrefixSet> browse_prefix_set_;
+    scoped_ptr<const safe_browsing::PrefixSet>
+        side_effect_free_whitelist_prefix_set_;
+    scoped_ptr<const safe_browsing::PrefixSet> unwanted_software_prefix_set_;
+
+    // Cache of gethash results for prefix stores. Entries should not be used if
+    // they are older than their expire_after field.  Cached misses will have
+    // empty full_hashes field.  Cleared on each update. The cache is "mutable"
+    // as it can be written to from any transaction holding the lock, including
+    // ReadTransactions.
+    mutable PrefixGetHashCache prefix_gethash_cache_;
+
+    DISALLOW_COPY_AND_ASSIGN(ThreadSafeStateManager);
+  };
+
+  // Forward the above inner-definitions to alleviate some verbosity in the
+  // impl.
+  using SBWhitelistId = ThreadSafeStateManager::SBWhitelistId;
+  using PrefixSetId = ThreadSafeStateManager::PrefixSetId;
+  using ReadTransaction = ThreadSafeStateManager::ReadTransaction;
+  using WriteTransaction = ThreadSafeStateManager::WriteTransaction;
+
+  bool PrefixSetContainsUrl(const GURL& url,
+                            PrefixSetId prefix_set_id,
+                            std::vector<SBPrefix>* prefix_hits,
+                            std::vector<SBFullHashResult>* cache_hits);
 
   // Exposed for testing of PrefixSetContainsUrlHashes() on the
   // PrefixSet backing kMalwareList.
@@ -380,15 +467,14 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
       std::vector<SBPrefix>* prefix_hits,
       std::vector<SBFullHashResult>* cache_hits);
 
-  bool PrefixSetContainsUrlHashes(
-      const std::vector<SBFullHash>& full_hashes,
-      scoped_ptr<const safe_browsing::PrefixSet>* prefix_set_getter,
-      std::vector<SBPrefix>* prefix_hits,
-      std::vector<SBFullHashResult>* cache_hits);
+  bool PrefixSetContainsUrlHashes(const std::vector<SBFullHash>& full_hashes,
+                                  PrefixSetId prefix_set_id,
+                                  std::vector<SBPrefix>* prefix_hits,
+                                  std::vector<SBFullHashResult>* cache_hits);
 
   // Returns true if the whitelist is disabled or if any of the given hashes
   // matches the whitelist.
-  bool ContainsWhitelistedHashes(const SBWhitelist& whitelist,
+  bool ContainsWhitelistedHashes(SBWhitelistId whitelist_id,
                                  const std::vector<SBFullHash>& hashes);
 
   // Return the browse_store_, download_store_, download_whitelist_store or
@@ -399,10 +485,15 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   bool Delete();
 
   // Load the prefix set in "|db_filename| Prefix Set" off disk, if available,
-  // and stores it in |prefix_set|.  |read_failure_type| provides a
-  // caller-specific error code to be used on failure.
+  // and stores it in the PrefixSet identified by |prefix_set_id|.
+  // |read_failure_type| provides a caller-specific error code to be used on
+  // failure.  This method should only ever be called during initialization as
+  // it performs some disk IO while holding a transaction (for the sake of
+  // avoiding uncessary back-and-forth interactions with the lock during
+  // Init()).
   void LoadPrefixSet(const base::FilePath& db_filename,
-                     scoped_ptr<const safe_browsing::PrefixSet>* prefix_set,
+                     ThreadSafeStateManager::WriteTransaction* txn,
+                     PrefixSetId prefix_set_id,
                      FailureType read_failure_type);
 
   // Writes the current prefix set "|db_filename| Prefix Set" on disk.
@@ -416,11 +507,7 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // of hashes is too large or if the kill switch URL is on the whitelist
   // we will whitelist everything.
   void LoadWhitelist(const std::vector<SBAddFullHash>& full_hashes,
-                     SBWhitelist* whitelist);
-
-  // Call this method if an error occured with the given whitelist.  This will
-  // result in all lookups to the whitelist to return true.
-  void WhitelistEverything(SBWhitelist* whitelist);
+                     SBWhitelistId whitelist_id);
 
   // Parses the IP blacklist from the given full-length hashes.
   void LoadIpBlacklist(const std::vector<SBAddFullHash>& full_hashes);
@@ -453,27 +540,31 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // to highlight the specific store who made the initial request on failure.
   // |store_full_hashes_in_prefix_set| dictates whether full_hashes from the
   // |url_store| should be cached in the |prefix_set| as well.
-  void UpdatePrefixSetUrlStore(
-      const base::FilePath& db_filename,
-      SafeBrowsingStore* url_store,
-      scoped_ptr<const safe_browsing::PrefixSet>* prefix_set,
-      FailureType finish_failure_type,
-      FailureType write_failure_type,
-      bool store_full_hashes_in_prefix_set);
+  void UpdatePrefixSetUrlStore(const base::FilePath& db_filename,
+                               SafeBrowsingStore* url_store,
+                               PrefixSetId prefix_set_id,
+                               FailureType finish_failure_type,
+                               FailureType write_failure_type,
+                               bool store_full_hashes_in_prefix_set);
 
   void UpdateUrlStore(SafeBrowsingStore* url_store,
-                      scoped_ptr<const safe_browsing::PrefixSet>* prefix_set,
+                      PrefixSetId prefix_set_id,
                       FailureType failure_type);
 
   void UpdateWhitelistStore(const base::FilePath& store_filename,
                             SafeBrowsingStore* store,
-                            SBWhitelist* whitelist);
+                            SBWhitelistId whitelist_id);
   void UpdateIpBlacklistStore();
 
-  // Used to verify that various calls are made from the thread the
-  // object was created on (i.e., the safe_browsing_thread).
+  // 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();
+
   base::ThreadChecker thread_checker_;
 
+  ThreadSafeStateManager state_manager_;
+
   // 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_;
@@ -505,21 +596,6 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // For unwanted software list.
   scoped_ptr<SafeBrowsingStore> unwanted_software_store_;
 
-  // Lock for protecting access to variables that may be used on any threads.
-  // This includes all SBWhitelist's, PrefixSet's, and caches.
-  base::Lock lookup_lock_;
-
-  SBWhitelist csd_whitelist_;
-  SBWhitelist download_whitelist_;
-
-  // The IP blacklist should be small.  At most a couple hundred IPs.
-  IPBlacklist ip_blacklist_;
-
-  // Cache of gethash results for prefix stores. Entries should not be used if
-  // they are older than their expire_after field.  Cached misses will have
-  // empty full_hashes field.  Cleared on each update.
-  std::map<SBPrefix, SBCachedFullHashResult> prefix_gethash_cache_;
-
   // 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()|.
@@ -529,17 +605,6 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // Used to optimize away database update.
   bool change_detected_;
 
-  // PrefixSets to speed up lookups for particularly large lists. The PrefixSet
-  // themselves are never modified, instead a new one is swapped in on update
-  // while holding |lookup_lock_|. Any thread other than this class' main thread
-  // (which handles updates) must hold |lookup_lock_| before reading from these
-  // sets.
-  // TODO(gab): Enforce this by design.
-  scoped_ptr<const safe_browsing::PrefixSet> browse_prefix_set_;
-  scoped_ptr<const safe_browsing::PrefixSet>
-      side_effect_free_whitelist_prefix_set_;
-  scoped_ptr<const safe_browsing::PrefixSet> unwanted_software_prefix_set_;
-
   // Used to schedule resetting the database because of corruption.
   base::WeakPtrFactory<SafeBrowsingDatabaseNew> reset_factory_;
 };