Move full hash caching logic to v4_get_hash_protocol_manager

components/safe_browsing_db/v4_get_hash_protocol_manager.h

Index: components/safe_browsing_db/v4_get_hash_protocol_manager.h
diff --git a/components/safe_browsing_db/v4_get_hash_protocol_manager.h b/components/safe_browsing_db/v4_get_hash_protocol_manager.h
index d65e1a656db1f2c568983403c96b116198aa3f31..6deec5487bb160110e33c8e1e05112126f1879a6 100644
--- a/components/safe_browsing_db/v4_get_hash_protocol_manager.h
+++ b/components/safe_browsing_db/v4_get_hash_protocol_manager.h
@@ -37,111 +37,249 @@ class URLRequestContextGetter;
 
 namespace safe_browsing {
 
+// The matching hash prefixes and corresponding stores, for each full hash
+// generated for a given URL.
+typedef base::hash_map<FullHash, StoreAndHashPrefixes>
+    FullHashToStoreAndHashPrefixesMap;
+
+// ----------------------------------------------------------------
+
+// All information about a particular full hash i.e. negative TTL, store for
+// which it is valid, and metadata associated with that store.
+struct FullHashInfo {
+ public:
+  FullHash full_hash;
+
+  // The list for which this full hash is applicable.
+  UpdateListIdentifier list_id;
+
+  // The expiration time of the full hash for a particular store.
+  base::Time positive_ttl;
+
+  // Any metadata for this full hash for a particular store.
+  ThreatMetadata metadata;
+
+  explicit FullHashInfo(const FullHash& full_hash,

[Nathan Parker, 2016/09/09 21:26:21]

nit: I don't think you need explicit if the ctor has > 1 arg (since it couldn't 
accidentally cast from 3 args)

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Good catch. Also not for copy constructors so removed that also.
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

+                        const UpdateListIdentifier& list_id,
+                        const base::Time& positive_ttl);
+  explicit FullHashInfo(const FullHashInfo& other);
+  ~FullHashInfo();
+
+  bool operator==(const FullHashInfo& other) const;
+  bool operator!=(const FullHashInfo& other) const;
+
+ private:
+  FullHashInfo();
+};
+
+// Caches individual response from GETHASH response.
+struct CachedHashPrefixInfo {
+  // The negative ttl for the hash prefix that leads to this
+  // CachedHashPrefixInfo. The client should not send any more requests for that
+  // hash prefix until this time.
+  base::Time negative_ttl;
+
+  // The list of all full hashes (and related info) that start with a
+  // particular hash prefix and are known to be unsafe.
+  std::vector<FullHashInfo> full_hash_infos;
+
+  CachedHashPrefixInfo();
+  CachedHashPrefixInfo(const CachedHashPrefixInfo& other);
+  ~CachedHashPrefixInfo();
+};
+
+// Cached full hashes received from the server for the corresponding hash
+// prefixes.
+typedef base::hash_map<HashPrefix, CachedHashPrefixInfo> FullHashCache;
+
+// FullHashCallback is invoked when GetFullHashes completes.
+// Parameters:
+//   - The vector of full hash results. If empty, indicates that there
+//     were no matches, and that the resource is safe.
+typedef base::Callback<void(const std::vector<FullHashInfo>&)> FullHashCallback;
+
+// Information needed to update the cache and call the callback to post the
+// results.
+struct FullHashCallbackInfo {
+  FullHashCallbackInfo();
+  explicit FullHashCallbackInfo(

[Nathan Parker, 2016/09/09 21:26:21]

nit: don't need explicit

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+      const std::vector<FullHashInfo>& cached_full_hash_infos,
+      const std::vector<HashPrefix>& prefixes_requested,
+      std::unique_ptr<net::URLFetcher> fetcher,
+      const FullHashToStoreAndHashPrefixesMap&
+          full_hash_to_store_and_hash_prefixes,
+      FullHashCallback callback);

[Nathan Parker, 2016/09/09 21:26:21]

nit: Pass callbacks by const-reference (they do ref-counting internally, but you
can avoid that w/ const-ref).

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+  ~FullHashCallbackInfo();
+
+  // The FullHashInfo objects retrieved from cache. These are merged with the
+  // results received from the server before invoking the callback.
+  std::vector<FullHashInfo> cached_full_hash_infos;
+
+  // The callback method to call after collecting the full hashes for given
+  // hash prefixes.
+  FullHashCallback callback;
+
+  // The fetcher that will return the response from the server.

[Nathan Parker, 2016/09/09 21:26:21]

Maybe comment on why it's here (so you can delete it after?)

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+  std::unique_ptr<net::URLFetcher> fetcher;
+
+  // The generated full hashes and the corresponding prefixes and the stores in
+  // which to look for a full hash match.
+  FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
+
+  // The prefixes that were requested from the server.
+  std::vector<HashPrefix> prefixes_requested;
+};
+
+// ----------------------------------------------------------------
+
 class V4GetHashProtocolManagerFactory;
 
 class V4GetHashProtocolManager : public net::URLFetcherDelegate,
                                  public base::NonThreadSafe {
  public:
-  // FullHashCallback is invoked when GetFullHashes completes.
+  // Invoked when GetFullHashesWithApis completes.
   // Parameters:
-  //   - The vector of full hash results. If empty, indicates that there
-  //     were no matches, and that the resource is safe.
-  //   - The negative cache expire time of the result. This value may be
-  //     uninitialized, and the results should not be cached in this case.
-  typedef base::Callback<void(const std::vector<SBFullHashResult>&,
-                              const base::Time&)>
-      FullHashCallback;
+  //   - The API threat metadata for the given URL.
+  typedef base::Callback<void(const ThreatMetadata& md)>
+      ThreatMetadataForApiCallback;
 
   ~V4GetHashProtocolManager() override;
 
+  // Create an instance of the safe browsing v4 protocol manager.
+  static std::unique_ptr<V4GetHashProtocolManager> Create(
+      net::URLRequestContextGetter* request_context_getter,
+      const base::hash_set<UpdateListIdentifier>& stores_to_request,
+      const V4ProtocolConfig& config);
+
   // Makes the passed |factory| the factory used to instantiate
   // a V4GetHashProtocolManager. Useful for tests.
   static void RegisterFactory(
       std::unique_ptr<V4GetHashProtocolManagerFactory> factory);
 
-  // Create an instance of the safe browsing v4 protocol manager.
-  static V4GetHashProtocolManager* Create(
-      net::URLRequestContextGetter* request_context_getter,
-      const V4ProtocolConfig& config);
-
-  // net::URLFetcherDelegate interface.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  // Empties the cache.
+  void ClearCache();
 
   // Retrieve the full hash for a set of prefixes, and invoke the callback
   // argument when the results are retrieved. The callback may be invoked
   // synchronously.
-  virtual void GetFullHashes(const std::vector<SBPrefix>& prefixes,
-                             const std::vector<PlatformType>& platforms,
-                             ThreatType threat_type,
+  virtual void GetFullHashes(const FullHashToStoreAndHashPrefixesMap&
+                                 full_hash_to_matching_hash_prefixes,
                              FullHashCallback callback);
 
-  // Retrieve the full hash and API metadata for a set of prefixes, and invoke
-  // the callback argument when the results are retrieved. The callback may be
-  // invoked synchronously.
-  virtual void GetFullHashesWithApis(const std::vector<SBPrefix>& prefixes,
-                                     FullHashCallback callback);
+  // Retrieve the full hash and API metadata for a URL, and invoke the callback
+  // argument when the results are retrieved. The callback may be invoked
+  // synchronously.

[Nathan Parker, 2016/09/09 21:26:21]

Add a comment on what "apis" is and why it's different here (explaining to
yourself 12 months from now...)

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+  virtual void GetFullHashesWithApis(const GURL& url,
+                                     ThreatMetadataForApiCallback api_callback);
 
-  // Overrides the clock used to check the time.
-  void SetClockForTests(std::unique_ptr<base::Clock> clock);
+  // net::URLFetcherDelegate interface.
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
 
  protected:
   // Constructs a V4GetHashProtocolManager that issues
   // network requests using |request_context_getter|.
-  V4GetHashProtocolManager(net::URLRequestContextGetter* request_context_getter,
-                           const V4ProtocolConfig& config);
+  V4GetHashProtocolManager(
+      net::URLRequestContextGetter* request_context_getter,
+      const base::hash_set<UpdateListIdentifier>& stores_to_request,
+      const V4ProtocolConfig& config);
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
-                           TestGetHashRequest);
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
-                           TestParseHashResponse);
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest, TestGetHashRequest);
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest, TestParseHashResponse);
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest,
                            TestParseHashResponseWrongThreatEntryType);
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest,
                            TestParseHashThreatPatternType);
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest,
                            TestParseHashResponseNonPermissionMetadata);
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest,
                            TestParseHashResponseInconsistentThreatTypes);
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest,
                            TestGetHashErrorHandlingOK);
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest,
+                           TestResultsNotCachedForNegativeCacheDuration);
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest,
                            TestGetHashErrorHandlingNetwork);
-  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4GetHashProtocolManagerTest,
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest,
                            TestGetHashErrorHandlingResponseCode);
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest, GetCachedResults);
+  FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest, TestUpdatesAreMerged);
+  friend class V4GetHashProtocolManagerTest;
   friend class V4GetHashProtocolManagerFactoryImpl;
 
+  FullHashCache* full_hash_cache_for_tests() { return &full_hash_cache_; }
+
+  // Looks up the cached results for full hashes in
+  // |full_hash_to_store_and_hash_prefixes|. Fills |prefixes_to_request| with
+  // the prefixes that need to be requested. Fills |cached_full_hash_infos|
+  // with the cached results.
+  // Note: It is valid for both |prefixes_to_request| and
+  // |cached_full_hash_infos| to be empty after this function finishes.
+  void GetFullHashCachedResults(
+      const FullHashToStoreAndHashPrefixesMap&
+          full_hash_to_store_and_hash_prefixes,
+      const base::Time& now,
+      std::vector<HashPrefix>* prefixes_to_request,
+      std::vector<FullHashInfo>* cached_full_hash_infos) const;
+
+  // Fills a FindFullHashesRequest protocol buffer for a request.
+  // Returns the serialized and base 64 encoded request as a string.
+  std::string GetHashRequest(
+      const std::vector<HashPrefix>& prefixes_to_request);
+
   void GetHashUrlAndHeaders(const std::string& request_base64,
                             GURL* gurl,
                             net::HttpRequestHeaders* headers) const;
 
-  // Fills a FindFullHashesRequest protocol buffer for a request.
-  // Returns the serialized and base 64 encoded request as a string.
-  std::string GetHashRequest(const std::vector<SBPrefix>& prefixes,
-                             const std::vector<PlatformType>& platforms,
-                             ThreatType threat_type);
+  // Updates internal state for each GetHash response error, assuming that
+  // the current time is |now|.
+  void HandleGetHashError(const base::Time& now);
+
+  // Merges the results from the cache and the results from the server. The
+  // response from the server may include information for full hashes from
+  // stores other than those required by this client so it filters out those
+  // results that the client did not ask for.
+  void MergeResults(const FullHashToStoreAndHashPrefixesMap&
+                        full_hash_to_store_and_hash_prefixes,
+                    const std::vector<FullHashInfo>& full_hash_infos,
+                    std::vector<FullHashInfo>* merged_full_hash_infos);
+
+  // Calls |api_callback| with a an object of ThreatMetadata that contains
+  // permission api metadata for full hashes in those |full_hash_infos| that

[Nathan Parker, 2016/09/09 21:26:21]

nit: capitalize API (And TTL for that matter) when they are separate words

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+  // have a full hash in |full_hashes|.
+  void OnFullHashForApi(ThreatMetadataForApiCallback api_callback,

[Nathan Parker, 2016/09/09 21:26:21]

pass callbacks by const ref.

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+                        const base::hash_set<FullHash>& full_hashes,
+                        const std::vector<FullHashInfo>& full_hash_infos);
 
   // Parses a FindFullHashesResponse protocol buffer and fills the results in
-  // |full_hashes| and |negative_cache_expire|. |data| is a serialized
-  // FindFullHashes protocol buffer. |negative_cache_expire| is the cache expiry
-  // time of the response for entities that did not match the threat list.
-  // Returns true if parsing is successful, false otherwise.
-  bool ParseHashResponse(const std::string& data_base64,
-                         std::vector<SBFullHashResult>* full_hashes,
+  // |full_hash_infos| and |negative_cache_expire|. |data| is a serialized
+  // FindFullHashes protocol buffer. |negative_cache_expire| is the cache
+  // expiry time of the hash prefixes that were requested. Returns true if
+  // parsing is successful; false otherwise.
+  bool ParseHashResponse(const std::string& data,

[Nathan Parker, 2016/09/09 21:26:21]

nit: How about response_data instead of data?

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+                         std::vector<FullHashInfo>* full_hash_infos,
                          base::Time* negative_cache_expire);
 
+  // Parses the store specific |metadata| information from the |match|.

[Nathan Parker, 2016/09/09 21:26:21]

nit: describe what it returns

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+  bool ParseMetadata(const ThreatMatch& match, ThreatMetadata* metadata);
+
   // Resets the gethash error counter and multiplier.
   void ResetGetHashErrors();
 
-  // Updates internal state for each GetHash response error, assuming that
-  // the current time is |now|.
-  void HandleGetHashError(const base::Time& now);
+  // Overrides the clock used to check the time.
+  void SetClockForTests(std::unique_ptr<base::Clock> clock);
+
+  // Updates the state of the full hash cache upon receiving a valid response
+  // from the server.
+  void UpdateCache(const std::vector<HashPrefix>& prefixes_requested,
+                   const std::vector<FullHashInfo>& full_hash_infos,
+                   const base::Time& negative_cache_expire);
 
  private:
   // Map of GetHash requests to parameters which created it.
-  using HashRequests = base::hash_map<
-      const net::URLFetcher*,
-      std::pair<std::unique_ptr<net::URLFetcher>, FullHashCallback>>;
+  using PendingHashRequests =
+      base::hash_map<const net::URLFetcher*,
+                     std::unique_ptr<FullHashCallbackInfo>>;
 
   // The factory that controls the creation of V4GetHashProtocolManager.
   // This is used by tests.
@@ -159,7 +297,7 @@ class V4GetHashProtocolManager : public net::URLFetcherDelegate,
   // Multiplier for the backoff error after the second.
   size_t gethash_back_off_mult_;
 
-  HashRequests hash_requests_;
+  PendingHashRequests pending_hash_requests_;
 
   // For v4, the next gethash time is set to the backoff time is the last
   // response was an error, or the minimum wait time if the last response was
@@ -178,6 +316,16 @@ class V4GetHashProtocolManager : public net::URLFetcherDelegate,
   // The clock used to vend times.
   std::unique_ptr<base::Clock> clock_;
 
+  // A cache of full hash results.
+  FullHashCache full_hash_cache_;
+
+  // The following sets represent the combination of lists that we would always
+  // request from the server, irrespective of which list we found the hash
+  // prefix match in.
+  base::hash_set<PlatformType> platform_types_;
+  base::hash_set<ThreatEntryType> threat_entry_types_;
+  base::hash_set<ThreatType> threat_types_;
+
   DISALLOW_COPY_AND_ASSIGN(V4GetHashProtocolManager);
 };
 
@@ -186,14 +334,19 @@ class V4GetHashProtocolManagerFactory {
  public:
   V4GetHashProtocolManagerFactory() {}
   virtual ~V4GetHashProtocolManagerFactory() {}
-  virtual V4GetHashProtocolManager* CreateProtocolManager(
+  virtual std::unique_ptr<V4GetHashProtocolManager> CreateProtocolManager(
       net::URLRequestContextGetter* request_context_getter,
+      const base::hash_set<UpdateListIdentifier>& stores_to_request,
       const V4ProtocolConfig& config) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(V4GetHashProtocolManagerFactory);
 };
 
+#ifndef NDEBUG
+std::ostream& operator<<(std::ostream& os, const FullHashInfo& id);
+#endif
+
 }  // namespace safe_browsing
 
 #endif  // COMPONENTS_SAFE_BROWSING_DB_V4_GET_HASH_PROTOCOL_MANAGER_H_