v4_update_protocol_manager: Basic implementation with TODOs

components/safe_browsing_db/v4_update_protocol_manager.h

+// Copyright 2016 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 COMPONENTS_SAFE_BROWSING_DB_V4_UPDATE_PROTOCOL_MANAGER_H_
+#define COMPONENTS_SAFE_BROWSING_DB_V4_UPDATE_PROTOCOL_MANAGER_H_
+
+// A class that implements Chrome's interface with the SafeBrowsing V4 update
+// protocol.
+//
+// The V4UpdateProtocolManager handles formatting and making requests of, and
+// handling responses from, Google's SafeBrowsing servers. The purpose of this
+// class is to get hash prefixes from the SB server for the given set of lists.
+
+#include <string>
+#include <vector>
+
+#include "base/gtest_prod_util.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+#include "components/safe_browsing_db/safebrowsing.pb.h"
+#include "components/safe_browsing_db/util.h"
+#include "components/safe_browsing_db/v4_protocol_manager_util.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "url/gurl.h"
+
+namespace net {
+class URLFetcher;
+class URLRequestContextGetter;
+}  // namespace net
+
+namespace safe_browsing {
+
+class V4UpdateProtocolManagerFactory;
+
+class V4UpdateProtocolManager : public net::URLFetcherDelegate,
+                                public base::NonThreadSafe {
+ public:
+  typedef FetchThreatListUpdatesRequest::ListUpdateRequest ListUpdateRequest;
+  typedef FetchThreatListUpdatesResponse::ListUpdateResponse ListUpdateResponse;
+
+  // UpdateCallback is invoked when GetUpdates completes.
+  // Parameters:
+  //   - The vector of update response protobufs received from the server for
+  //     each list type.
+  //  The caller can then use this vector to re-build the current_list_states.
+  typedef base::Callback<void(const std::vector<ListUpdateResponse>&)>
+      UpdateCallback;
+
+  typedef V4ProtocolManagerUtil::OperationResultType OperationResultType;
+
+  ~V4UpdateProtocolManager() override;
+
+  // Makes the passed |factory| the factory used to instantiate
+  // a V4UpdateProtocolManager. Useful for tests.
+  static void RegisterFactory(V4UpdateProtocolManagerFactory* factory) {
+    factory_ = factory;
+  }
+
+  // Create an instance of the safe browsing v4 protocol manager.
+  static V4UpdateProtocolManager* Create(
+      net::URLRequestContextGetter* request_context_getter,
+      const V4ProtocolConfig& config);
+
+  // net::URLFetcherDelegate interface.
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
+
+  // Retrieve the hash prefix update, and invoke the callback argument when the
+  // results are retrieved. The callback may be invoked synchronously.
+  // Parameters:
+  //   - The set of list to fetch the updates for.

[Nathan Parker, 2016/03/22 23:57:55]

nit: s/list/lists/

[vakh (use Gerrit instead), 2016/03/24 22:34:24]

Done.

+  //   - The last known state for each of the known lists.
+  // It is valud to have one or more lists in lists_to_update set that have no

[Nathan Parker, 2016/03/22 23:57:55]

nit: valid

[vakh (use Gerrit instead), 2016/03/24 22:34:24]

Done.

+  // corresponding value in the current_list_states map. This corresponds to the
+  // initial state for those lists.
+  virtual void GetUpdates(
+      const base::hash_set<const UpdateListIdentifier*>& lists_to_update,

[Nathan Parker, 2016/03/22 23:57:55]

Could you do without lists_to_update, and just assume all that are present in
the _states should be updated? 

Also: Could store the UpdateListIdentifiers directly rather than ptrs, so
there's no concern of ownership.  They're pretty small.

[vakh (use Gerrit instead), 2016/03/24 22:34:24]

There won't be anything in the _states on first launch.
The set is for the updates we want to fetch and the _states map is for updates
we've already fetched.
 

[Nathan Parker, 2016/03/24 22:48:05]

This one too:

[vakh (use Gerrit instead), 2016/03/25 02:42:37]

Can't store instances of UpdateListIdentifiers directly into an unordered_set
since it can't be compared to another instance directly. That would require
adding a comparison method.
Instead, to manage the memory ownership better, I have changed it to use
scoped_ptrs.

[Nathan Parker, 2016/03/25 15:53:18]

The "set" guarantees there's only one of each.  With scoped_ptrs, I suspect it's
comparing the inner pointers.  So it won't guarantee you have a unique set of
_values_ unless you have a comparison operator. I ran into a similar case with
ThreatMetadata structs and added a few methods to make it more easily comparable
(in my case just for tests)  Code:

https://code.google.com/p/chromium/codesearch#chromium/src/components/safe_br...

[vakh (use Gerrit instead), 2016/03/25 22:51:47]

Done.

+      const base::hash_map<const UpdateListIdentifier*, const std::string&>&
+          current_list_states,
+      UpdateCallback callback);
+
+  // Record an update operation result.

[Nathan Parker, 2016/03/22 23:57:55]

This could be in an anonymous namespace in the .cc, ya?

[vakh (use Gerrit instead), 2016/03/24 22:34:24]

Done.

+  static void RecordUpdateResult(OperationResultType result_type);
+
+ protected:
+  // Constructs a V4UpdateProtocolManager that issues network requests using
+  // |request_context_getter|.
+  V4UpdateProtocolManager(net::URLRequestContextGetter* request_context_getter,
+                          const V4ProtocolConfig& config);
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4UpdateProtocolManagerTest,
+                           TestGetUpdatesErrorHandlingNetwork);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4UpdateProtocolManagerTest,
+                           TestGetUpdatesErrorHandlingResponseCode);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4UpdateProtocolManagerTest,
+                           TestGetUpdatesNoError);
+  friend class V4UpdateProtocolManagerFactoryImpl;
+
+  GURL GetUpdateUrl(const std::string& request_base64) const;

[Nathan Parker, 2016/03/22 23:57:55]

add a comment

[vakh (use Gerrit instead), 2016/03/24 22:34:24]

Done.

+
+  // Fills a FetchThreatListUpdatesRequest protocol buffer for a request.
+  // Returns the serialized and base 64 encoded request as a string.
+  std::string GetUpdateRequest(
+      const base::hash_set<const UpdateListIdentifier*>& lists_to_update,
+      const base::hash_map<const UpdateListIdentifier*, const std::string&>&
+          current_list_states);
+
+  // Parses the base64 encoded response received from the server as a
+  // FetchThreatListUpdatesResponse protobuf and returns each of the
+  // ListUpdateResponse protobufs contained in it as a vector.
+  // Returns true if parsing is successful, false otherwise.
+  bool ParseUpdateResponse(
+      const std::string& data_base64,
+      std::vector<ListUpdateResponse>* list_update_responses);
+
+  // Resets the update error counter and multiplier.
+  void ResetUpdateErrors();
+
+  // Updates internal update and backoff state for each update response error,
+  // assuming that the current time is |now|.
+  void HandleUpdateError(const base::Time& now);
+
+  // The factory that controls the creation of V4UpdateProtocolManager.
+  // This is used by tests.
+  static V4UpdateProtocolManagerFactory* factory_;
+
+  // The number of HTTP response errors since the the last successful HTTP
+  // response, used for request backoff timing.
+  size_t update_error_count_;
+
+  // Multiplier for the backoff error after the second.
+  size_t update_back_off_mult_;
+
+  // The next update time is set to the backoff time is the last response was an
+  // error, or the minimum wait time if the last response was successful.

[Nathan Parker, 2016/03/22 23:57:55]

This comment is hard to grok. How about describing how it's used rather than how
it's set?

[vakh (use Gerrit instead), 2016/03/24 22:34:24]

Done.

+  base::Time next_update_time_;
+
+  // The config of the client making Pver4 requests.
+  const V4ProtocolConfig config_;
+
+  // The context we use to issue network requests.
+  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+
+  // ID for URLFetchers for testing.
+  int url_fetcher_id_;
+
+  // True if there's a request pending.
+  bool update_request_pending_;

[Nathan Parker, 2016/03/22 23:57:55]

Is this identical to request_ != null?

[vakh (use Gerrit instead), 2016/03/24 22:34:24]

Done. I had added this with some timeout handling in mind but since I am not
handling timeouts anyway, I have removed that variable and added a TODO.

+
+  // The callback that's called when GetUpdates completes.
+  UpdateCallback callback_;
+
+  // The pending update request. The request must be canceled when the object is
+  // destroyed.
+  scoped_ptr<const net::URLFetcher> request_;
+
+  DISALLOW_COPY_AND_ASSIGN(V4UpdateProtocolManager);
+};
+
+// Interface of a factory to create V4UpdateProtocolManager.  Useful for tests.
+class V4UpdateProtocolManagerFactory {
+ public:
+  V4UpdateProtocolManagerFactory() {}
+  virtual ~V4UpdateProtocolManagerFactory() {}
+  virtual V4UpdateProtocolManager* CreateProtocolManager(
+      net::URLRequestContextGetter* request_context_getter,
+      const V4ProtocolConfig& config) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(V4UpdateProtocolManagerFactory);
+};
+
+}  // namespace safe_browsing
+
+#endif  // COMPONENTS_SAFE_BROWSING_DB_V4_UPDATE_PROTOCOL_MANAGER_H_