Safebrowsing: change gethash caching to match api 2.3 rules, fix some corner cases.

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 912216021a7be2456033136a4525068a519e7243..dc454b936abbe4d626bc471cc1d78267691118aa 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database.h
@@ -93,16 +93,14 @@ class SafeBrowsingDatabase {
   // Deletes the current database and creates a new one.
   virtual bool ResetDatabase() = 0;
 
-  // Returns false if |url| is not in the browse database.  If it
-  // returns true, then either |matching_list| is the name of the matching
-  // list, or |prefix_hits| and |full_hits| contains the matching hash
-  // prefixes.  This function is safe to call from threads other than
-  // the creation thread.
+  // Returns false if |url| is not in the browse database or already was cached
+  // as a miss.  If it returns true, |prefix_hits| contains matching hash
+  // prefixes which had no cached results and |cache_hits| contains any matching
+  // cached gethash results.  This function is safe to call from threads other
+  // than the creation thread.
   virtual bool ContainsBrowseUrl(const GURL& url,
-                                 std::string* matching_list,
                                  std::vector<SBPrefix>* prefix_hits,
-                                 std::vector<SBFullHashResult>* full_hits,
-                                 base::Time last_update) = 0;
+                                 std::vector<SBFullHashResult>* cache_hits) = 0;
 
   // 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
@@ -172,9 +170,9 @@ class SafeBrowsingDatabase {
   // Store the results of a GetHash response. In the case of empty results, we
   // cache the prefixes until the next update so that we don't have to issue
   // further GetHash requests we know will be empty.
-  virtual void CacheHashResults(
-      const std::vector<SBPrefix>& prefixes,
-      const std::vector<SBFullHashResult>& full_hits) = 0;
+  virtual void CacheHashResults(const std::vector<SBPrefix>& prefixes,
+                                const std::vector<SBFullHashResult>& full_hits,
+                                const base::TimeDelta& cache_lifetime) = 0;
 
   // Returns true if the malware IP blacklisting killswitch URL is present
   // in the csd whitelist.
@@ -291,10 +289,9 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   virtual void Init(const base::FilePath& filename) OVERRIDE;
   virtual bool ResetDatabase() OVERRIDE;
   virtual bool ContainsBrowseUrl(const GURL& url,
-                                 std::string* matching_list,
                                  std::vector<SBPrefix>* prefix_hits,
-                                 std::vector<SBFullHashResult>* full_hits,
-                                 base::Time last_update) OVERRIDE;
+                                 std::vector<SBFullHashResult>* cache_hits)
+      OVERRIDE;
   virtual bool ContainsDownloadUrl(const std::vector<GURL>& urls,
                                    std::vector<SBPrefix>* prefix_hits) OVERRIDE;
   virtual bool ContainsCsdWhitelistedUrl(const GURL& url) OVERRIDE;
@@ -312,9 +309,9 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   virtual void DeleteChunks(
       const std::vector<SBChunkDelete>& chunk_deletes) OVERRIDE;
   virtual void UpdateFinished(bool update_succeeded) OVERRIDE;
-  virtual void CacheHashResults(
-      const std::vector<SBPrefix>& prefixes,
-      const std::vector<SBFullHashResult>& full_hits) OVERRIDE;
+  virtual void CacheHashResults(const std::vector<SBPrefix>& prefixes,
+                                const std::vector<SBFullHashResult>& full_hits,
+                                const base::TimeDelta& cache_lifetime) OVERRIDE;
 
   // Returns the value of malware_kill_switch_;
   virtual bool IsMalwareIPMatchKillSwitchOn() OVERRIDE;
@@ -333,6 +330,12 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // IPv6 IP prefix using SHA-1.
   typedef std::map<std::string, base::hash_set<std::string> > IPBlacklist;
 
+  // Like ContainsBrowseUrl, but takes a vector of fullhashes instead of an URL.
+  // |full_hashes| must be sorted.

[Scott Hess - ex-Googler, 2014/04/01 22:08:36]

Perhaps describe this as a helper for ContainsBrowseUrl.  Also reference the
sort required for full_hashes.

[mattm, 2014/04/03 01:38:12]

Done.
Changed the wording to make it more obvious.

+  bool ContainsBrowseUrlHashes(const std::vector<SBFullHash>& full_hashes,
+                               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,
@@ -399,7 +402,7 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
 
   // Lock for protecting access to variables that may be used on the
   // IO thread.  This includes |prefix_set_|, |full_browse_hashes_|,
-  // |pending_browse_hashes_|, |prefix_miss_cache_|, |csd_whitelist_|.
+  // |browse_gethash_cache_|, |csd_whitelist_|.
   base::Lock lookup_lock_;
 
   // Underlying persistent store for chunk data.
@@ -440,18 +443,13 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // The IP blacklist should be small.  At most a couple hundred IPs.
   IPBlacklist ip_blacklist_;
 
-  // Cached browse store related full-hash items, ordered by prefix for
-  // efficient scanning.
-  // |full_browse_hashes_| are items from |browse_store_|,
-  // |pending_browse_hashes_| are items from |CacheHashResults()|, which
-  // will be pushed to the store on the next update.
+  // Full-hashes from browse store, ordered by prefix for efficient scanning.
   std::vector<SBAddFullHash> full_browse_hashes_;
-  std::vector<SBAddFullHash> pending_browse_hashes_;
 
-  // Cache of prefixes that returned empty results (no full hash
-  // match) to |CacheHashResults()|.  Cached to prevent asking for
-  // them every time.  Cleared on next update.
-  std::set<SBPrefix> prefix_miss_cache_;
+  // Cache of gethash results for browse store. 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> browse_gethash_cache_;
 
   // Used to schedule resetting the database because of corruption.
   base::WeakPtrFactory<SafeBrowsingDatabaseNew> reset_factory_;