Introduce SafeBrowsingDatabase::ThreadSafeStateManager.

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>
@@ skipping 349 matching lines @@
   // A SafeBrowsing whitelist contains a list of whitelisted full-hashes (stored
   // in a sorted vector) as well as a boolean flag indicating whether all
   // lookups in the whitelist should be considered matches for safety.
   typedef std::pair<std::vector<SBFullHash>, bool> SBWhitelist;
 
   // This map holds a csd malware IP blacklist which maps a prefix mask
   // to a set of hashed blacklisted IP prefixes.  Each IP prefix is a hashed
   // 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.
   bool ContainsBrowseUrlHashesForTesting(
       const std::vector<SBFullHash>& full_hashes,
       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
   // csd_whitelist_store_ based on list_id.
   SafeBrowsingStore* GetStore(int list_id);
 
   // Deletes the files on disk.
   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.
   // |write_failure_type| provides a caller-specific error code to be used on
   // failure.
   void WritePrefixSet(const base::FilePath& db_filename,
                       const safe_browsing::PrefixSet* prefix_set,
                       FailureType write_failure_type);
 
   // Loads the given full-length hashes to the given whitelist.  If the number
   // 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);
 
   // Helpers for handling database corruption.
   // |OnHandleCorruptDatabase()| runs |ResetDatabase()| and sets
   // |corruption_detected_|, |HandleCorruptDatabase()| posts
   // |OnHandleCorruptDatabase()| to the current thread, to be run
   // after the current task completes.
   // TODO(shess): Wire things up to entirely abort the update
@@ skipping 12 matching lines @@
   // Returns the size in bytes of the store after the update.
   int64 UpdateHashPrefixStore(const base::FilePath& store_filename,
                                SafeBrowsingStore* store,
                                FailureType failure_type);
 
   // Updates a PrefixStore store for URLs (|url_store|) which is backed on disk
   // by a "|db_filename| Prefix Set" file. Specific failure types are provided
   // 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.
   base::FilePath filename_base_;
 
   // Underlying persistent store for chunk data.
   // For browsing related (phishing and malware URLs) chunks and prefixes.
   scoped_ptr<SafeBrowsingStore> browse_store_;
 
   // For download related (download URL and binary hash) chunks and prefixes.
@@ skipping 12 matching lines @@
 
   // For side-effect free whitelist.
   scoped_ptr<SafeBrowsingStore> side_effect_free_whitelist_store_;
 
   // For IP blacklist.
   scoped_ptr<SafeBrowsingStore> ip_blacklist_store_;
 
   // 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()|.
   bool corruption_detected_;
 
   // Set to true if any chunks are added or deleted during an update.
   // 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_;
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_