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 37dcf8f06f9ce0f8ce197e6512c2e64dad285981..9d381778077a8b51bd3557acaf2225438e58c541 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database.h
@@ -367,11 +367,95 @@ 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,
+    };
+
+    // 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;
+
+    // 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;
+
+    ThreadSafeStateManager();
+    ~ThreadSafeStateManager();
+
+    scoped_ptr<ReadTransaction> BeginReadTransaction();
+    scoped_ptr<ReadTransaction> BeginReadTransactionNoLockOnMainThread();
+    scoped_ptr<WriteTransaction> BeginWriteTransaction();
+
+   private:
+    // Used to verify that writes are only made on this object's main thread.
+    // This is important as this class allows reading from the main thread
+    // without holding the lock.
+    base::ThreadChecker thread_checker_;
+
+    // Lock for protecting access to this class' state.
+    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.
+    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 +464,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 +482,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 +504,11 @@ 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);
+                     SBWhitelistId whitelist_id);
 
   // 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);
+  void WhitelistEverything(SBWhitelistId whitelist_id);
 
   // Parses the IP blacklist from the given full-length hashes.
   void LoadIpBlacklist(const std::vector<SBAddFullHash>& full_hashes);
@@ -453,26 +541,28 @@ 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).
-  base::ThreadChecker thread_checker_;
+  // 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();
+
+  ThreadSafeStateManager state_manager_;
 
   // The base filename passed to Init(), used to generate the store and prefix
   // set filenames used to store data on disk.
@@ -505,21 +595,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 +604,6 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // Used to optimize away database update.
   bool change_detected_;

[mattm, 2014/12/12 23:20:31]

Should all the non-thread-safe members be moved to a another sub-class which is
NonThreadSafe? Otherwise there is still risk of accessing those ones improperly.

[gab, 2014/12/15 23:02:30]

Good point, do you think the SafeBrowsingStores should be in such a
NonThreadSafeStateManager (since they're already NonThreadSafe anyways) or just
these two bools (in which case I can easily make a small utils class for the two
bools).

[mattm, 2014/12/23 02:09:45]

I guess to make it more consistent putting them in the nonthreadsafe class would
be preferred, even if it's not necessary. Guess it depends how it ends up
looking though.

[gab, 2014/12/23 21:39:54]

SG, how about we do this in a follow-up CL since this CL is already getting
pretty big?
Here's a proposal: https://codereview.chromium.org/814993003/

[mattm, 2014/12/24 00:29:53]

separate CL sounds good

 
-  // 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_;
 };