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..fb74446efcb403cb7bd28ef11f5d40171e2b1a64 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,259 @@ 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;

[Scott Hess - ex-Googler, 2016/09/12 23:31:35]

I'm slightly unnerved by this use of "ttl" with an absolute time, as I'm more
used to "ttl" being used with a time delta.  I think I see what you mean, I'm
just nervous about whether this will someday lead to mistakes.

[vakh (use Gerrit instead), 2016/09/13 00:12:07]

I understand your concern but I feel like there are two reasons why it's not
very likely to happen:
1. Anyone changing this code in any meaningful way would/should know what this
"ttl" stands for. I'd consider that a minimum requirement.
2. This stored as base::Time so assigning or comparing against TimeDelta would
lead to compilation failure.

That being said, I'm totally willing to change it to something else that conveys
the meaning just as (or more) clearly. If you have a term in mind, please let me
know.

[Scott Hess - ex-Googler, 2016/09/13 00:31:34]

Sometimes the code uses "expire" for this concept, though "expiry" would be the
better sense.  I guess maybe if nobody else has remarked on it it's not an
issue.

[vakh (use Gerrit instead), 2016/09/13 01:18:56]

Done.

+
+  // Any metadata for this full hash for a particular store.
+  ThreatMetadata metadata;
+
+  FullHashInfo(const FullHash& full_hash,
+               const UpdateListIdentifier& list_id,
+               const base::Time& positive_ttl);
+  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.

[Scott Hess - ex-Googler, 2016/09/12 23:31:35]

Is this going to acquire additional parameters?  Or did it lose parameters
during development?

[vakh (use Gerrit instead), 2016/09/13 00:12:07]

Are you asking this because of "Parameters:"?
I thought that was the convention but I did not bother to look up what happens
in case of single arguments. I just looked it up and "Parameter:" occurs only 6
times in the entire codebase so I'd consider that uncommon.
(No parameters expected to be added in the near future.)

[Scott Hess - ex-Googler, 2016/09/13 00:31:34]

Mostly because the comment is setup like "Here's the list of things that will
happen", and then there's only a single item.  It might flow better (and be
easier to rewrap in Emacs) if it just reads like a paragraph without structure.

If the concern is having the anonymous parameter be confusing, you can always
name it.  void(const std::vec<FHI>& results) should parse fine.  But I don't
think it's necessary if there's just a single parameter, "the parameter" is
sufficient to describe it.

[vakh (use Gerrit instead), 2016/09/13 01:18:56]

Done.

[vakh (use Gerrit instead), 2016/09/13 01:18:56]

Done.

+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();
+  FullHashCallbackInfo(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,
+                       const FullHashCallback& callback);
+  ~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. This is stored
+  // here as a unique pointer to be able to reason about its lifetime easily.
+  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.
+  // GetFullHashesWithApis is a special case of GetFullHashes. It is here
+  // primarily for legacy reasons: so that DatabaseManager, which speaks PVer3,
+  // and V4LocalDatabaseManager, which speaks PVer4, can both use this class to
+  // perform API lookups. Once PVer4 migration is complete, DatabaseManager
+  // should be deleted and then this method can be moved to the
+  // V4LocalDatabaseManager class.
+  // TODO(vakh): Move this method to V4LocalDatabaseManager after launching
+  // PVer4 in Chromium.
+  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 an object of ThreatMetadata that contains
+  // permission API metadata for full hashes in those |full_hash_infos| that
+  // have a full hash in |full_hashes|.
+  void OnFullHashForApi(const ThreatMetadataForApiCallback& api_callback,
+                        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|. |response_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& response_data,
+                         std::vector<FullHashInfo>* full_hash_infos,
                          base::Time* negative_cache_expire);
 
+  // Parses the store specific |metadata| information from |match|. Returns
+  // true if the metadata information was parsed correctly and was consistent
+  // with what's expected from that corresponding store; false otherwise.
+  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 +307,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 +326,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 +344,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_