Initial implementation for CertNetFetcher.

net/cert/cert_net_fetcher.cc

+// 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.
+
+#include "net/cert/cert_net_fetcher.h"
+
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/timer/timer.h"
+#include "net/base/load_flags.h"
+#include "net/url_request/redirect_info.h"
+#include "net/url_request/url_request_context.h"
+
+// TODO(eroman): Add support for POST parameters.
+// TODO(eroman): Add controls for bypassing the cache.
+// TODO(eroman): Add a maximum number of in-flight jobs/requests.
+
+namespace net {
+
+namespace {
+
+// The error returned when the response body exceeded the size limit.
+const int kNetErrorResponseTooLarge = ERR_FILE_TOO_BIG;
+
+// The error returned when the URL could not be fetched because it was not an
+// allowed scheme (http).
+const int kNetErrorNotHttpUrl = ERR_DISALLOWED_URL_SCHEME;
+
+// The error returned when the URL fetch did not complete in time.
+const int kNetErrorTimedOut = ERR_TIMED_OUT;
+
+// The error returned when the response was HTTP however it did not have a
+// status of 200/OK.
+// TODO(eroman): Use a more specific error code.
+const int kNetErrorNot200HttpResponse = ERR_FAILED;
+
+// The size of the buffer used for reading the response body of the URLRequest.
+const int kReadBufferSizeInBytes = 4096;
+
+// The maximum size in bytes for the response body when fetching a CRL.
+const int kMaxResponseSizeInBytesForCrl = 5 * 1024 * 1024;
+
+// The maximum size in bytes for the response body when fetching an AIA URL
+// (caIssuers/OCSP).
+const int kMaxResponseSizeInBytesForAia = 64 * 1024;
+
+// The default timeout in seconds for fetch requests.
+const int kTimeoutSeconds = 15;
+
+// Policy for which URLs are allowed to be fetched. This is called both for the
+// initial URL and for each redirect. Returns OK on success or a net error
+// code on failure.
+int CanFetchUrl(const GURL& url) {
+  if (!url.SchemeIs("http"))
+    return kNetErrorNotHttpUrl;
+  return OK;
+}
+
+}  // namespace
+
+// CertNetFetcher::Request tracks an outstanding call to Fetch().
+struct CertNetFetcher::Request {
+  Request(FetchCallback callback, Job* job) : callback(callback), job(job) {}
+
+  // The callback to invoke when the request has completed.
+  FetchCallback callback;
+
+  // A non-owned pointer to the job that is executing the request (and in effect
+  // owns |this|).
+  Job* job;
+
+  DISALLOW_COPY_AND_ASSIGN(Request);
+};
+
+CertNetFetcher::RequestParams::RequestParams(const GURL& url, RequestType type)
+    : url(url),
+      http_method(HTTP_METHOD_GET),
+      timeout(base::TimeDelta::FromSeconds(kTimeoutSeconds)) {
+  // Use the request type to determine the maximum allowed response size.
+  switch (type) {
+    case REQUEST_TYPE_CRL:
+      max_response_size_in_bytes = kMaxResponseSizeInBytesForCrl;
+      break;
+    case REQUEST_TYPE_CA_ISSUERS:
+    case REQUEST_TYPE_OCSP:
+      max_response_size_in_bytes = kMaxResponseSizeInBytesForAia;
+      break;
+  }
+}
+
+bool CertNetFetcher::RequestParams::operator<(
+    const RequestParams& other) const {
+  if (url != other.url)
+    return url < other.url;
+  if (http_method != other.http_method)
+    return http_method < other.http_method;
+  if (max_response_size_in_bytes != other.max_response_size_in_bytes)
+    return max_response_size_in_bytes < other.max_response_size_in_bytes;
+  if (timeout != other.timeout)
+    return timeout < other.timeout;
+  return false;
+}
+
+// CertNetFetcher::Job tracks an outstanding URLRequest as well as all of the
+// pending requests for it.
+class CertNetFetcher::Job : public base::RefCounted<CertNetFetcher::Job>,
+                            public URLRequest::Delegate {
+ public:
+  Job(scoped_ptr<RequestParams> request_params, CertNetFetcher* parent);
+
+  // Cancels the job and all requests attached to it. No callbacks will be
+  // invoked following cancellation.
+  void Cancel();
+
+  const RequestParams& request_params() const { return *request_params_.get(); }

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

No need for .get(), just *request_params_ will do. (operator* overload)

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

Done.

+
+  // Attaches a request to the job. When the job completes it will invoke
+  // |callback|.
+  RequestId AddRequest(FetchCallback callback);

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

Pass by const-ref

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

Done.

+
+  // Removes |request| from the job and deletes it.
+  void CancelRequest(RequestId request);
+
+  // Creates and starts a URLRequest for the job. After the request has
+  // completed, OnJobCompleted() will be invoked and all the registered requests
+  // notified of completion.
+  void StartURLRequest(URLRequestContext* context);
+
+ private:
+  // The pointers in RequestList are owned by the Job.
+  typedef std::vector<Request*> RequestList;
+  friend class base::RefCounted<Job>;
+
+  ~Job() override;
+
+  // Implementation of URLRequest::Delegate
+  void OnReceivedRedirect(URLRequest* request,
+                          const RedirectInfo& redirect_info,
+                          bool* defer_redirect) override;
+  void OnResponseStarted(URLRequest* request) override;
+  void OnReadCompleted(URLRequest* request, int bytes_read) override;
+
+  // Clears the URLRequest and timer. Helper for doing work common to
+  // cancellation and job completion.
+  void Stop();
+
+  // Reads as much data as available from the |request|.
+  void ReadBody(URLRequest* request);
+
+  // Helper to copy the partial bytes read from the read IOBuffer to an
+  // aggregated buffer.
+  bool ConsumeBytesRead(URLRequest* request, int num_bytes);
+
+  // Called once the job has exceeded its deadline.
+  void OnTimeout();
+
+  // Called when the URLRequest has completed (either success or failure).
+  void OnUrlRequestCompleted(URLRequest* request);
+
+  // Called when the Job has completed. The job may finish in response to a
+  // timeout, an invalid URL, or the URLRequest completing. By the time this
+  // method is called, the response variables have been assigned
+  // (result_net_error_code_ et al).
+  void OnJobCompleted();
+
+  // The requests attached to this job.
+  RequestList requests_;
+
+  // The URLRequest input parameters.
+  scoped_ptr<RequestParams> request_params_;
+
+  // The URLRequest response information.
+  std::vector<uint8_t> response_body_;

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

Why not use a GrowableIOBuffer? Saves you on line 177 as well.

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

GrowableIOBuffer is more complicated to use. You need to increase capacity
manually, track its size, and then you end up with a pointer and length to pass
to consumers.

No thanks, I will stick with std::vector<>.

+  int result_net_error_;
+
+  scoped_ptr<URLRequest> url_request_;
+  scoped_refptr<IOBuffer> read_buffer_;
+
+  base::OneShotTimer<Job> timer_;
+
+  // Non-owned pointer to the CertNetFetcher that created this job.
+  CertNetFetcher* parent_;

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

DISALLOW_COPY_AND_ASSIGN

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

Done.

+};
+
+CertNetFetcher::Job::Job(scoped_ptr<RequestParams> request_params,
+                         CertNetFetcher* parent)
+    : request_params_(request_params.Pass()),
+      result_net_error_(ERR_IO_PENDING),
+      parent_(parent) {
+}
+
+void CertNetFetcher::Job::Cancel() {
+  parent_ = NULL;
+  STLDeleteElements(&requests_);
+  Stop();
+}
+
+CertNetFetcher::RequestId CertNetFetcher::Job::AddRequest(
+    FetchCallback callback) {
+  requests_.push_back(new Request(callback, this));
+  return requests_.back();
+}
+
+void CertNetFetcher::Job::CancelRequest(RequestId request) {
+  // This scope ensures that |it| is destroyed before RemoveJob() is called,
+  // since that may delete |this|.

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

Why is this necessary? |it| is already invalidated by line 211 (as are all other
iterators in |requests_|

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

I have re-worked this so deletion is guaranteed to happen afterwards.

+  {
+    RequestList::iterator it =
+        std::find(requests_.begin(), requests_.end(), request);
+    DCHECK(it != requests_.end());
+    requests_.erase(it);
+    delete request;
+  }
+
+  // If there are no longer any requests attached to the job, cancel and delete
+  // the job.
+  if (requests_.empty() && parent_) {

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

When would |parent_| be NULL? Doesn't that indicate programmer error?

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

This is a left-over from when I removing jobs from the in-progress set before
invoking callbacks.

That said, I have now restored that behavior so it is relevant again.

+    CertNetFetcher* parent = parent_;
+    Cancel();
+    parent->RemoveJob(this);
+  }
+}
+
+void CertNetFetcher::Job::StartURLRequest(URLRequestContext* context) {
+  int error = CanFetchUrl(request_params_->url);
+  if (error != OK) {
+    result_net_error_ = error;
+    // The CertNetFetcher's API contract is that requests always complete
+    // asynchronously. This situation is the only one which requires an
+    // explicit PostTask().
+    base::MessageLoop::current()->PostTask(

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

base::ThreadTaskRunnerHandle::Get()->PostTask

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

No longer relevant, using a OneShotTimer instead.

+        FROM_HERE, base::Bind(&Job::OnJobCompleted, this));
+    return;
+  }
+
+  // Start the URLRequest.
+  read_buffer_ = new IOBuffer(kReadBufferSizeInBytes);
+  url_request_ = context->CreateRequest(request_params_->url, DEFAULT_PRIORITY,
+                                        this, NULL);
+  if (request_params_->http_method == HTTP_METHOD_POST)
+    url_request_->set_method("POST");
+  url_request_->SetLoadFlags(LOAD_DO_NOT_SAVE_COOKIES |
+                             LOAD_DO_NOT_SEND_COOKIES);
+  url_request_->Start();
+
+  // Start a timer to limit how long the job runs for.
+  if (request_params_->timeout > base::TimeDelta())
+    timer_.Start(FROM_HERE, request_params_->timeout, this, &Job::OnTimeout);
+}
+
+CertNetFetcher::Job::~Job() {
+  DCHECK(requests_.empty());
+}
+
+void CertNetFetcher::Job::OnReceivedRedirect(URLRequest* request,
+                                             const RedirectInfo& redirect_info,
+                                             bool* defer_redirect) {
+  DCHECK_EQ(url_request_.get(), request);
+
+  // Ensure that the new URL matches the policy.
+  int error = CanFetchUrl(redirect_info.new_url);
+  if (error != OK) {
+    request->CancelWithError(error);
+    OnUrlRequestCompleted(request);
+    return;
+  }
+}
+
+void CertNetFetcher::Job::OnResponseStarted(URLRequest* request) {
+  DCHECK_EQ(url_request_.get(), request);

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

Hopefully we can fix this with https://codereview.chromium.org/837873002/ , in
which case, the .get() would not be necessary.

But if wishes were horses...

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

Acknowledged.

+
+  if (!request->status().is_success()) {
+    OnUrlRequestCompleted(request);
+    return;
+  }
+
+  // In practice all URLs fetched are HTTP, but check anyway as defensive
+  // measure in case the policy is ever changed.
+  DCHECK(request->url().SchemeIsHTTPOrHTTPS());

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

Why this, instead of re-using your helper method?

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

Removed.

+  int http_response_code = request->GetResponseCode();
+  if (http_response_code != 200 && request->url().SchemeIsHTTPOrHTTPS()) {

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

BUG? What's up with this &&?

Does that mean if I somehow slip LDAP in here, and it fails with != 200, then
we'll call ReadBody?

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

Removed in favor of unconditionally checking response_code.

+    request->CancelWithError(kNetErrorNot200HttpResponse);
+    OnUrlRequestCompleted(request);
+    return;
+  }
+
+  ReadBody(request);
+}
+
+void CertNetFetcher::Job::OnReadCompleted(URLRequest* request, int bytes_read) {
+  DCHECK_EQ(url_request_.get(), request);
+
+  // Keep reading the response body.
+  if (ConsumeBytesRead(request, bytes_read))
+    ReadBody(request);
+}
+
+void CertNetFetcher::Job::Stop() {
+  timer_.Stop();
+  url_request_.reset();
+}
+
+void CertNetFetcher::Job::ReadBody(URLRequest* request) {
+  // Read as many bytes as are available synchronously.
+  int num_bytes;
+  while (
+      request->Read(read_buffer_.get(), kReadBufferSizeInBytes, &num_bytes)) {
+    if (!ConsumeBytesRead(request, num_bytes))
+      return;
+  }
+
+  // Check whether the read failed synchronously.
+  if (!request->status().is_io_pending())
+    OnUrlRequestCompleted(request);
+  return;
+}
+
+bool CertNetFetcher::Job::ConsumeBytesRead(URLRequest* request, int num_bytes) {
+  if (num_bytes <= 0) {
+    // Error while reading, or EOF.
+    OnUrlRequestCompleted(request);
+    return false;
+  }
+
+  // Enforce maximum size bound.
+  if (num_bytes + response_body_.size() >
+      request_params_->max_response_size_in_bytes) {

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

something something overflow

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

Overflow was possible if the consumer of CertNetFetcher were to set the size
limit for the request to a value larger than:

 std::numeric_limits<size_t>::max() - 4096

Which practically speaking is likely to OOM or fail before ever overflowing.

That said I appreciate you pointing out an integer overflow!

In the new patchset I believe overflow is no longer possible as the input is
being given as a signed "int". However I have added some safety checks as well
so there isn't a reliance on relative sizes of size_t <--> int.

+    request->CancelWithError(kNetErrorResponseTooLarge);
+    OnUrlRequestCompleted(request);
+    return false;
+  }
+
+  // Append the data to |response_body_|.
+  response_body_.insert(response_body_.end(), read_buffer_->data(),
+                        read_buffer_->data() + num_bytes);
+  return true;
+}
+
+void CertNetFetcher::Job::OnTimeout() {
+  result_net_error_ = kNetErrorTimedOut;
+  url_request_->CancelWithError(result_net_error_);
+  OnJobCompleted();
+}
+
+void CertNetFetcher::Job::OnUrlRequestCompleted(URLRequest* request) {
+  DCHECK_EQ(request, url_request_.get());
+
+  if (request->status().is_success())
+    result_net_error_ = OK;
+  else
+    result_net_error_ = request->status().error();
+
+  OnJobCompleted();
+}
+
+void CertNetFetcher::Job::OnJobCompleted() {
+  // Stop the timer and clear the URLRequest.
+  Stop();
+
+  // Check if the job was cancelled. This can happen when running the posted
+  // task for OnJobCompleted() as it is not aborted on cancellation.
+  if (!parent_)
+    return;
+
+  // Invoking the callbacks is subtle as it may cause re-entrancy:
+  //   * The parent CertNetFetcher may be deleted (in which case this job is
+  //     kept alive by |keep_alive|.
+  //   * Requests in this job may be cancelled (which is why |requests_| is not
+  //     iterated though.
+  //   * New requests may be attached to this job (which is why |requests_| is
+  //     not iterated through.

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

Am I reading this right that if new jobs are attached following a failing job,
they'll be immediately failed?

Does this create an API usability concern where, as a response to a job failing,
a new request is started, except it's just attached to this existing request and
failed?

I believe we handle this elsewhere (CertVerifier, HostResolver) by having it
indirect first through the parent (to remove the job from the pending list, so
that any new requests create/are joined to a new job) and then end up here in
the Job.

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

Yes your reading is correct.

I had done it this way to easily support the "delete CertNetFetcher withing a
callback case".

However I agree with you that on error it would be more useful to re-issue the
request, and that pattern is more useful for building re-try logic.

I will change it to match your expectations. Will probably add some concept of a
"job currently being completed" to the CertNetFetcher so the deletion case is
still supported.

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

This is now addressed in the new patchset. I also simplified Job to not be
refcounted.

+
+  scoped_refptr<Job> keep_alive(this);
+
+  while (!requests_.empty()) {
+    scoped_ptr<Request> request(requests_.front());
+    requests_.erase(requests_.begin());
+    request->callback.Run(result_net_error_, response_body_);
+  }
+
+  if (parent_)
+    parent_->RemoveJob(this);
+}
+
+bool CertNetFetcher::JobComparator::operator()(
+    const scoped_refptr<Job>& job1,
+    const scoped_refptr<Job>& job2) const {
+  return job1->request_params() < job2->request_params();
+}
+
+CertNetFetcher::CertNetFetcher(URLRequestContext* context) : context_(context) {
+}
+
+CertNetFetcher::~CertNetFetcher() {
+  // Each job is explicitly cancelled since destroying |jobs_| might not
+  // release the final reference to an in-progress job.
+  for (JobSet::iterator it = jobs_.begin(); it != jobs_.end(); ++it) {

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

for (auto* job : jobs) {
  job->Cancel();
}

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

I have deleted this code in favor of calling STLDeleteElements(), as Job is no
longer refcounted.

+    (*it)->Cancel();
+  }
+}
+
+CertNetFetcher::RequestId CertNetFetcher::Fetch(
+    scoped_ptr<RequestParams> request_params,
+    FetchCallback callback) {
+  DCHECK(CalledOnValidThread());
+
+  // If there is an in-progress job that matches the request parameters use it.
+  // Otherwise start a new job.
+  scoped_refptr<Job> job = FindJob(*request_params.get());
+
+  if (!job) {
+    job = new Job(request_params.Pass(), this);
+    jobs_.insert(job);
+    job->StartURLRequest(context_);
+  }
+
+  return job->AddRequest(callback);
+}
+
+void CertNetFetcher::CancelRequest(RequestId request) {
+  DCHECK(CalledOnValidThread());
+
+  request->job->CancelRequest(request);
+}
+
+struct CertNetFetcher::JobToRequestParamsComparator {
+  bool operator()(const scoped_refptr<Job>& job,
+                  const CertNetFetcher::RequestParams& value) const {
+    return job->request_params() < value;
+  }
+};
+
+CertNetFetcher::Job* CertNetFetcher::FindJob(const RequestParams& params) {
+  DCHECK(CalledOnValidThread());
+
+  // The JobSet is kept in sorted order so items can be find using binary
+  // search.
+  JobSet::iterator it = std::lower_bound(jobs_.begin(), jobs_.end(), params,
+                                         JobToRequestParamsComparator());
+  if (it != jobs_.end() && !(params < (*it)->request_params()))
+    return it->get();
+  return NULL;
+}
+
+void CertNetFetcher::RemoveJob(Job* job) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(jobs_.find(job) != jobs_.end());
+  jobs_.erase(job);

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

performance nit: Even though the .find() call is optimized out in a non-DCHECK
build, the .erase(job) method will still effectively invoke a find.

Because of this, you can optimize and avoid a double .find() for DCHECK builds,
without trading any of the performance for release builds, by:

auto it = jobs_.find(job);
DCHECK(it != jobs_.end());
jobs_.erase(it);

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

I changed to using the return value from erase() and testing it it was 1.

+}
+
+}  // namespace net