Initial implementation for CertNetFetcher.

net/cert/cert_net_fetcher.h

Index: net/cert/cert_net_fetcher.h
diff --git a/net/cert/cert_net_fetcher.h b/net/cert/cert_net_fetcher.h
new file mode 100644
index 0000000000000000000000000000000000000000..eca57f0acf367d69f54bfbfb29db78c8024fd81b
--- /dev/null
+++ b/net/cert/cert_net_fetcher.h
@@ -0,0 +1,151 @@
+// Copyright 2015 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 NET_CERT_CERT_NET_FETCHER_H_
+#define NET_CERT_CERT_NET_FETCHER_H_
+
+#include <set>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/time/time.h"
+#include "net/base/net_export.h"
+#include "url/gurl.h"
+
+namespace net {
+
+class URLRequestContext;
+
+// CertNetFetcher is a helper for fetching AIA URLs and CRL URLs.
+//
+// It manages the outstanding requests, and applies policies specific to
+// certificate network fetches. For instance:
+//   * Enforces a maximum timeout on responses
+//   * Enforces a maximum size on responses
+//   * Restricts URLs to http://
+//
+// The CertNetFetcher is initialized by giving it a URLRequestContext for the
+// network dependencies. Note that it does not maintain its own in-memory cache
+// for responses. Instead it is reliant on the URLRequestContext providing an
+// HTTP cache if this is desired.
+//
+// -------------------------
+// Cancellation of requests
+// -------------------------
+//
+//  * Network requests started by the CertNetFetcher can be cancelled using
+//    CancelRequest(). Cancellation means the callback will not be invoked.
+//
+//  * If the CertNetFetcher is deleted then any outstanding
+//    requests are automatically cancelled.
+//
+//  * Cancelling a request more than once is an error (and may result in a
+//    use-after-free).
+//
+//  * Cancelling a request within the execution of a callback is allowed.
+//
+//  * Deleting the CertNetFetcher from within the execution of a callback is
+//    allowed.
+//
+// -------------------------
+// Threading
+// -------------------------
+//
+// The CertNetFetcher is expected to be operated from a single thread, which has
+// an IO message loop. The URLRequestContext will be accessed from this same
+// thread, and callbacks will be posted to this message loop.
+//
+// For more details see the early design document:
+//   https://docs.google.com/a/chromium.org/document/d/1CdS9YOnPdAyVZBJqHY7ZJ6tUlU71OCvX8kHnaVhf144/edit
+class NET_EXPORT CertNetFetcher
+    : NON_EXPORTED_BASE(public base::NonThreadSafe) {

[Ryan Sleevi, 2015/02/11 08:41:54]

I'm not a fan of inheriting from base::NonThreadSafe, in that it's not a
pure-virtual base class

Have you considered using a base::ThreadChecker_ instead?

[eroman, 2015/02/11 23:18:08]

NonThreadSafe was based on a recommendation by Matt. I can try out ThreadChecker
too, will get back on this.

[eroman, 2015/02/13 22:11:38]

Done -- switched to ThreadChecker.

+ private:
+  struct Request;

[Ryan Sleevi, 2015/02/11 08:41:53]

Is it truly necessary for this to appear first? Though line 98 exists, I thought
that you could use the naming scope rules of classes to allow out-of-order
declaration (i.e. moving this to line 122 "should" still work)

[eroman, 2015/02/11 23:18:08]

I get a compile error when moving it down.

+
+ public:
+  typedef base::Callback<void(int, const std::vector<uint8_t>&)> FetchCallback;
+
+  enum RequestType {
+    REQUEST_TYPE_CA_ISSUERS,
+    REQUEST_TYPE_OCSP,
+    REQUEST_TYPE_CRL,
+  };
+
+  enum HttpMethod {
+    HTTP_METHOD_GET,
+    HTTP_METHOD_POST,
+  };
+
+  struct RequestParams {

[Ryan Sleevi, 2015/02/11 08:41:54]

I just dinged another reviewer for using a struct to smuggle params and
potentially create overloaded methods and default arguments.

A couple issues with this design approach:
1) By having this struct public, it's part of your API for others to use as
well. To me, that does not logically fit with how it is used (as a set of
arguments for Fetch)
2) You've now made Fetch() have four possible overloads (at least)
- (url, http_method) specified, (max_response, timeout) as default values
- (url, http_method, max_response) specified, timeout as default values
- (url, http_method, timeout) specified, max_response as default values
- (url, http_method, max_response, timeout) specified, no default values

When reading code, I can no longer reason about which of these four methods is
'effectively' invoked. If refactoring Fetch(), I can no longer reason about
which arguments are required and which are optional.

3) By having operator<() be a member, rather than a comparator helper, you're
encouraging others further to store these values. Indeed, strictly speaking from
how your API is used, there's no need to expose the operator<() at all


Suggestion: If you wish to allow response_size and timeout to be optional, but
still have semi-sane defaults, then perhaps declaring public constants for these
(CertNetFetcher::kDefaultMaxResponseSize) and making Fetch() explicitly require
all four parameters be present can resolve this ambiguity.

[eroman, 2015/02/11 20:15:16]

I will switch this over to using explicit methods, and keep the RequestParams
internal.

For the record, some properties of the RequestParams approach that I like:
  - Adding POST parameters can be expressed as a method on RequestParams
  - Fewer copying of parameters
  - Ease of testing by being able to override timeouts and max sizes without
needing to introduce test-only mechanisms, or require callers to always specify
them.

[eroman, 2015/02/11 23:18:08]

Done. In the latest patchset there are separate methods for fetching, and
RequestParams is an internal detail.

I agree this is a cleaner design.

+    RequestParams(const GURL& url, RequestType type);
+
+    bool operator<(const RequestParams& other) const;
+
+    GURL url;
+    HttpMethod http_method;
+    size_t max_response_size_in_bytes;
+
+    // If set to a value <= 0 then means "no timeout".
+    base::TimeDelta timeout;
+
+    // IMPORTANT: When adding fields to this structure, update operator<().
+  };
+
+  typedef Request* RequestId;
+
+  // Initilizes CertNetFetcher using the specified URLRequestContext for issuing
+  // requests. |context| must remain valid for the entire lifetime of the
+  // CertNetFetcher.
+  explicit CertNetFetcher(URLRequestContext* context);
+
+  ~CertNetFetcher();
+
+  // Starts an asynchronous request to fetch the given URL. On completion
+  // |callback| will be invoked.
+  //
+  // Completion of the request will never occur synchronously. In other words it
+  // is guaranteed that |callback| will only be invoked once the Fetch*() method
+  // has returned.
+  RequestId Fetch(scoped_ptr<RequestParams> request_params,

[Ryan Sleevi, 2015/02/11 08:41:54]

See comments re: implicit overloads being created here.

[eroman, 2015/02/11 23:18:08]

Acknowledged.

+                  FetchCallback callback);

[Ryan Sleevi, 2015/02/11 08:41:53]

Pass by const-ref

[eroman, 2015/02/11 23:18:08]

Done.

+
+  // Cancels the indicated request. It is an error to call this function on a
+  // request which has already completed (including one that was already
+  // cancelled).
+  void CancelRequest(RequestId request);
+
+ private:
+  class Job;
+  struct JobToRequestParamsComparator;
+
+  struct JobComparator {
+    bool operator()(const scoped_refptr<Job>& job1,
+                    const scoped_refptr<Job>& job2) const;
+  };
+
+  typedef std::set<scoped_refptr<Job>, JobComparator> JobSet;
+
+  // Finds a job with a matching RequestPararms or returns NULL if there was no
+  // match.
+  Job* FindJob(const RequestParams& params);

[Ryan Sleevi, 2015/02/11 08:41:54]

DANGER: This API pattern generally results in "doom", in my experience, since
the result may be invalidated through some nested call.

Recommendation: return a scoped_refptr<Job>, to ensure that the returned Job
remains valid for the execution of the FullStatement.

[eroman, 2015/02/11 23:18:08]

No longer relevant as Job is not refcounted.

+
+  // Removes |job| from the in progress jobs. This may cause the final reference
+  // to |job| to be released.
+  void RemoveJob(Job* job);
+
+  // The in-progress jobs.
+  JobSet jobs_;
+
+  // Not owned. CertNetFetcher must outlive the URLRequestContext.
+  URLRequestContext* context_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertNetFetcher);
+};
+
+}  // namespace net
+
+#endif  // NET_CERT_CERT_NET_FETCHER_H_