Certificate Transparency: Fetching consistency proofs.

components/certificate_transparency/log_proof_fetcher.cc

Index: components/certificate_transparency/log_proof_fetcher.cc
diff --git a/components/certificate_transparency/log_proof_fetcher.cc b/components/certificate_transparency/log_proof_fetcher.cc
index 7e51858708b0acb5d60dbd550cba646ff6ac5ab6..b886bb87bfe6b82368e4ead7e08c038215a41782 100644
--- a/components/certificate_transparency/log_proof_fetcher.cc
+++ b/components/certificate_transparency/log_proof_fetcher.cc
@@ -6,9 +6,13 @@
 
 #include <iterator>
 
+#include "base/callback_helpers.h"
+#include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "components/safe_json/safe_json_parser.h"
 #include "net/base/io_buffer.h"
@@ -18,6 +22,7 @@
 #include "net/cert/ct_log_response_parser.h"
 #include "net/cert/signed_tree_head.h"
 #include "net/http/http_status_code.h"
+#include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "url/gurl.h"
 
@@ -27,216 +32,438 @@ namespace {
 
 // Shamelessly copied from domain_reliability/util.cc
 int GetNetErrorFromURLRequestStatus(const net::URLRequestStatus& status) {
-  switch (status.status()) {
-    case net::URLRequestStatus::SUCCESS:
-      return net::OK;
-    case net::URLRequestStatus::CANCELED:
-      return net::ERR_ABORTED;
-    case net::URLRequestStatus::FAILED:
-      return status.error();
-    default:
-      NOTREACHED();
-      return net::ERR_FAILED;
-  }
+  if (status.is_success())
+    return net::OK;
+
+  return status.error();
 }
 
 }  // namespace
 
-struct LogProofFetcher::FetchState {
-  FetchState(const std::string& log_id,
-             const SignedTreeHeadFetchedCallback& fetched_callback,
-             const FetchFailedCallback& failed_callback);
-  ~FetchState();
-
-  std::string log_id;
-  SignedTreeHeadFetchedCallback fetched_callback;
-  FetchFailedCallback failed_callback;
-  scoped_refptr<net::IOBufferWithSize> response_buffer;
-  std::string assembled_response;
+// Class for issuing a particular request from a CT log and assembling the
+// response.
+// Creates the URLRequest instance for fetching the URL from the log
+// (supplied as |request_url| in the c'tor) and implements the
+// URLRequest::Delegate interface for assembling the response.
+class LogProofFetcher::LogFetcher : public net::URLRequest::Delegate {
+ public:
+  using FailureCallback = base::Callback<void(int, int)>;
+
+  LogFetcher(net::URLRequestContext* request_context,
+             const GURL& request_url,
+             const base::Closure& success_callback,
+             const FailureCallback& failure_callback);
+  ~LogFetcher() override {}
+
+  // net::URLRequest::Delegate
+  void OnResponseStarted(net::URLRequest* request) override;
+  void OnReadCompleted(net::URLRequest* request, int bytes_read) override;
+
+  const std::string& assembled_response() const { return assembled_response_; }
+
+ private:
+  // Handles the final result of a URLRequest::Read call on the request.
+  // Returns true if another read should be started, false if the read
+  // failed completely or we have to wait for OnResponseStarted to
+  // be called.
+  bool HandleReadResult(int bytes_read);
+
+  // Calls URLRequest::Read on |request| repeatedly, until HandleReadResult
+  // indicates it should no longer be called. Usually this would be when there
+  // is pending IO that requires waiting for OnResponseStarted to be called.
+  void StartNextReadLoop();
+
+  // Invokes the success callback. After this method is called, the LogFetcher
+  // is deleted and no longer safe to call.
+  void RequestComplete();
+
+  // Invokes the failure callback with the supplied error information.
+  // After this method the LogFetcher is deleted and no longer safe to call.
+  void InvokeFailureCallback(int net_error, int http_response_code);
+
+  scoped_ptr<net::URLRequest> url_request_;
+  const GURL request_url_;
+  base::Closure success_callback_;
+  FailureCallback failure_callback_;
+  scoped_refptr<net::IOBufferWithSize> response_buffer_;
+  std::string assembled_response_;
+
+  DISALLOW_COPY_AND_ASSIGN(LogFetcher);
 };
 
-LogProofFetcher::FetchState::FetchState(
-    const std::string& log_id,
-    const SignedTreeHeadFetchedCallback& fetched_callback,
-    const FetchFailedCallback& failed_callback)
-    : log_id(log_id),
-      fetched_callback(fetched_callback),
-      failed_callback(failed_callback),
-      response_buffer(new net::IOBufferWithSize(kMaxLogResponseSizeInBytes)) {}
-
-LogProofFetcher::FetchState::~FetchState() {}
-
-LogProofFetcher::LogProofFetcher(net::URLRequestContext* request_context)
-    : request_context_(request_context), weak_factory_(this) {
-  DCHECK(request_context);
+LogProofFetcher::LogFetcher::LogFetcher(net::URLRequestContext* request_context,
+                                        const GURL& request_url,
+                                        const base::Closure& success_callback,
+                                        const FailureCallback& failure_callback)
+    : request_url_(request_url),
+      success_callback_(success_callback),
+      failure_callback_(failure_callback) {
+  DCHECK(request_url_.SchemeIsHTTPOrHTTPS());
+  url_request_ =
+      request_context->CreateRequest(request_url_, net::DEFAULT_PRIORITY, this);
+  // This request should not send any cookies or otherwise identifying data,
+  // as CT logs are expected to be publicly-accessible and connections to them
+  // stateless.
+  url_request_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                             net::LOAD_DO_NOT_SAVE_COOKIES |
+                             net::LOAD_DO_NOT_SEND_AUTH_DATA);
+
+  url_request_->Start();
 }
 
-LogProofFetcher::~LogProofFetcher() {
-  STLDeleteContainerPairPointers(inflight_requests_.begin(),
-                                 inflight_requests_.end());
-}
+void LogProofFetcher::LogFetcher::OnResponseStarted(net::URLRequest* request) {
+  DCHECK_EQ(url_request_.get(), request);
+  int net_error = GetNetErrorFromURLRequestStatus(request->status());

[Ryan Sleevi, 2016/01/23 00:34:23]

if status.is_success() is false, will status.error() ever be net::OK?

If status.error() is set, is request->GetResponseCode() guaranteed to be
anything meaningful?

I feel like this helper is actually hiding logic/side-behaviours, and I wonder
whether it might make more sense to write out explicitly

if (!request->status.is_success()) {
  InvokeFailureCallback(request->status.error(), request->GetResponseCode() /*
is this ever filled if request->status.is_success() is false? */);
  return;
}

if (request->GetResponseCode() != net::HTTP_OK) {
  InvokeFailureCallback(net::OK /*is this really what you want? */,
request->GetResponseCode());
  return;
}

Yes, it's more duplication, but it also makes it slightly clearer the exit
cases.

[Eran Messeri, 2016/01/25 17:07:38]

Done - implemented two separate checks as you suggested.

 
-void LogProofFetcher::FetchSignedTreeHead(
-    const GURL& base_log_url,
-    const std::string& log_id,
-    const SignedTreeHeadFetchedCallback& fetched_callback,
-    const FetchFailedCallback& failed_callback) {
-  DCHECK(base_log_url.SchemeIsHTTPOrHTTPS());
-  GURL fetch_url(base_log_url.Resolve("ct/v1/get-sth"));
-  scoped_ptr<net::URLRequest> request =
-      request_context_->CreateRequest(fetch_url, net::DEFAULT_PRIORITY, this);
-  request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                        net::LOAD_DO_NOT_SAVE_COOKIES |
-                        net::LOAD_DO_NOT_SEND_AUTH_DATA);
-
-  FetchState* fetch_state =
-      new FetchState(log_id, fetched_callback, failed_callback);
-  request->Start();
-  inflight_requests_.insert(std::make_pair(request.release(), fetch_state));
-}
-
-void LogProofFetcher::OnResponseStarted(net::URLRequest* request) {
-  net::URLRequestStatus status(request->status());
-  DCHECK(inflight_requests_.count(request));
-  FetchState* fetch_state = inflight_requests_.find(request)->second;
-
-  if (!status.is_success() || request->GetResponseCode() != net::HTTP_OK) {
-    int net_error = net::OK;
+  if (net_error != net::OK || request->GetResponseCode() != net::HTTP_OK) {
     int http_response_code = request->GetResponseCode();
 
-    DVLOG(1) << "Fetching STH from " << request->original_url()
-             << " failed. status:" << status.status()
-             << " error:" << status.error()
-             << " http response code: " << http_response_code;
-    if (!status.is_success())
-      net_error = GetNetErrorFromURLRequestStatus(status);
-
-    InvokeFailureCallback(request, net_error, http_response_code);
+    InvokeFailureCallback(net_error, http_response_code);
     return;
   }
 
-  StartNextRead(request, fetch_state);
+  // Lazily initialize |response_buffer_| to avoid consuming memory until an
+  // actual response has been received.
+  if (!response_buffer_)

[Ryan Sleevi, 2016/01/23 00:34:23]

braces

[Eran Messeri, 2016/01/25 17:07:38]

Done.

+    response_buffer_ =
+        new net::IOBufferWithSize(LogProofFetcher::kMaxLogResponseSizeInBytes);
+
+  StartNextReadLoop();
 }
 
-void LogProofFetcher::OnReadCompleted(net::URLRequest* request,
-                                      int bytes_read) {
-  DCHECK(inflight_requests_.count(request));
-  FetchState* fetch_state = inflight_requests_.find(request)->second;
+void LogProofFetcher::LogFetcher::OnReadCompleted(net::URLRequest* request,
+                                                  int bytes_read) {
+  DCHECK_EQ(url_request_.get(), request);
 
-  if (HandleReadResult(request, fetch_state, bytes_read))
-    StartNextRead(request, fetch_state);
+  if (HandleReadResult(bytes_read))
+    StartNextReadLoop();
 }
 
-bool LogProofFetcher::HandleReadResult(net::URLRequest* request,
-                                       FetchState* fetch_state,
-                                       int bytes_read) {
+bool LogProofFetcher::LogFetcher::HandleReadResult(int bytes_read) {
   // Start by checking for an error condition.
   // If there are errors, invoke the failure callback and clean up the
   // request.
-  if (bytes_read == -1 || !request->status().is_success()) {
-    net::URLRequestStatus status(request->status());
-    DVLOG(1) << "Read error: " << status.status() << " " << status.error();
-    InvokeFailureCallback(request, GetNetErrorFromURLRequestStatus(status),
-                          net::OK);
+  if (bytes_read < 0 || !url_request_->status().is_success()) {

[Ryan Sleevi, 2016/01/23 00:34:23]

This order of conditions, while likely irrelevant to the function, seems
non-obvious from a self-documenting sense. Shouldn't you check for errors
_before_ you then check to see if data was screwed up? It seems like the (latter
condition) implies the former, but not vice-versa.

[Eran Messeri, 2016/01/25 17:07:37]

Swapped ordering.

+    InvokeFailureCallback(
+        GetNetErrorFromURLRequestStatus(url_request_->status()), net::OK);

[Ryan Sleevi, 2016/01/23 00:34:23]

This is the other case where GetNetError() resulted in surprising behaviour,
IMO.

If bytes_read <0, and is_success() is true, this calls
InvokeFailureCallback(net::OK, net::OK), which... doesn't make sense. Unless it
does?

That's why I'm not sure whether it makes sense to first handle the !is_success()
case (where a real .error() is available), and then handle the bytes_read < 0
(if that's ever actually possible w/ is_success()), with a more meaningful
explicit error code.

This would also get rid of the helper entirely.

[Eran Messeri, 2016/01/25 17:07:38]

Fair point. I've checked (in this if clause) if read_bytes < 0 and if so changed
the error passed to InvokeFailureCallback to be net::URLRequestStatus::FAILED
(because a more detailed failure information is not available).

 
     return false;
   }
 
   // Not an error, but no data available, so wait for OnReadCompleted
   // callback.
-  if (request->status().is_io_pending())
+  if (url_request_->status().is_io_pending())
     return false;
 
   // Nothing more to read from the stream - finish handling the response.
   if (bytes_read == 0) {
-    RequestComplete(request);
+    RequestComplete();
     return false;
   }
 
   // We have data, collect it and indicate another read is needed.
-  DVLOG(1) << "Have " << bytes_read << " bytes to assemble.";
   DCHECK_GE(bytes_read, 0);
-  fetch_state->assembled_response.append(fetch_state->response_buffer->data(),
-                                         bytes_read);
-  if (fetch_state->assembled_response.size() > kMaxLogResponseSizeInBytes) {
+  // |bytes_read| is non-negative at this point, casting to size_t should be
+  // safe.
+  if (base::checked_cast<size_t>(bytes_read) >
+          LogProofFetcher::kMaxLogResponseSizeInBytes ||
+      LogProofFetcher::kMaxLogResponseSizeInBytes <
+          (assembled_response_.size() + bytes_read)) {
     // Log response is too big, invoke the failure callback.
-    InvokeFailureCallback(request, net::ERR_FILE_TOO_BIG, net::HTTP_OK);
+    InvokeFailureCallback(net::ERR_FILE_TOO_BIG, net::HTTP_OK);
     return false;
   }
 
+  assembled_response_.append(response_buffer_->data(), bytes_read);
   return true;
 }
 
-void LogProofFetcher::StartNextRead(net::URLRequest* request,
-                                    FetchState* fetch_state) {
+void LogProofFetcher::LogFetcher::StartNextReadLoop() {
   bool continue_reading = true;
   while (continue_reading) {
     int read_bytes = 0;
-    request->Read(fetch_state->response_buffer.get(),
-                  fetch_state->response_buffer->size(), &read_bytes);
-    continue_reading = HandleReadResult(request, fetch_state, read_bytes);
+    url_request_->Read(response_buffer_.get(), response_buffer_->size(),
+                       &read_bytes);
+    continue_reading = HandleReadResult(read_bytes);
   }
 }
 
-void LogProofFetcher::RequestComplete(net::URLRequest* request) {
-  DCHECK(inflight_requests_.count(request));
-
-  FetchState* fetch_state = inflight_requests_.find(request)->second;
-
+void LogProofFetcher::LogFetcher::RequestComplete() {
   // Get rid of the buffer as it really isn't necessary.
-  fetch_state->response_buffer = nullptr;
-  safe_json::SafeJsonParser::Parse(
-      fetch_state->assembled_response,
-      base::Bind(&LogProofFetcher::OnSTHJsonParseSuccess,
-                 weak_factory_.GetWeakPtr(), request),
-      base::Bind(&LogProofFetcher::OnSTHJsonParseError,
-                 weak_factory_.GetWeakPtr(), request));
+  response_buffer_ = nullptr;
+  base::ResetAndReturn(&success_callback_).Run();
+  // NOTE: |this| not valid after invoking the callback as the LogFetcher
+  // instance will be delected by the callback.

[Ryan Sleevi, 2016/01/23 00:34:23]

typo: deleted
grammar: |this| is not valid
grammar: s/callback as/callback, as/

[Eran Messeri, 2016/01/25 17:07:38]

Done.

 }
 
-void LogProofFetcher::CleanupRequest(net::URLRequest* request) {
-  DVLOG(1) << "Cleaning up request to " << request->original_url();
-  auto it = inflight_requests_.find(request);
-  DCHECK(it != inflight_requests_.end());
-  auto next_it = it;
-  std::advance(next_it, 1);
+void LogProofFetcher::LogFetcher::InvokeFailureCallback(
+    int net_error,
+    int http_response_code) {
+  base::ResetAndReturn(&failure_callback_).Run(net_error, http_response_code);
+  // NOTE: |this| not valid after this callback as the LogFetcher instance

[Ryan Sleevi, 2016/01/23 00:34:22]

grammar: |this| is not valid
grammar: s/callback as/callback, as/

[Eran Messeri, 2016/01/25 17:07:38]

Done.

+  // invoking the callback will be deleted by the callback.
+}
+
+// Interface for handling the response from a CT log for particular
+// requests.

[Ryan Sleevi, 2016/01/23 00:34:23]

grammar: Is the plurality agreement correct? Isn't this handling the response
from a CT log for _a_ particular request_.

?

[Eran Messeri, 2016/01/25 17:07:38]

Good point, fixed to be explicitly singular.

+// All log responses are JSON and should be parsed; however the response
+// to each request should be parsed and validated diffenetly.

[Ryan Sleevi, 2016/01/23 00:34:23]

typo: differently.

[Eran Messeri, 2016/01/25 17:07:37]

Done.

+//
+// LogProofFetcher instances are owned by the LogProofFetcher and are

[Ryan Sleevi, 2016/01/23 00:34:22]

This comment seems confusingly worded.

Is this a typo about LogResponseHandler's are owned by the LogProofFetcher? Or
that LogProofFetchers own themselves? (which doesn't seem like it belongs here)

The comment as a whole seems to be documenting the particular implementation of
LogProofFetcher, and doesn't seem relevant to the LogResponseHandler (especially
since the public dtor on line 226 means it's not really guaranteed that the
comment isn't a lie)

[Eran Messeri, 2016/01/25 17:07:37]

(1) Yes, there's a typo.
(2) I've changed the documentation to clarify that LogResponseHandler instances
are expected to be deleted by the DoneCallback.

+// deleted by the |done_callback| when it is invoked.
+class LogProofFetcher::LogResponseHandler {
+ public:
+  using DoneCallback = base::Callback<void(LogProofFetcher::LogResponseHandler*,
+                                           const base::Closure&)>;

[Ryan Sleevi, 2016/01/23 00:34:23]

DESIGN: While this is a .cc file, and so I'm not going to press hard on it, it
does seem an odd design to pass the |this| as the first member - you're
effectively binding the API contract to this specific implementation, which
seems a very tight coupling.

That is, an alternative example (based solely on reading this class) would seem
to be that DoneCallback is just a base::Closure() (that is, remove the typedef
in line 232), and the caller's responsible for lifetime management - such as
using the form

void Caller::MyMethod(LogResponseHandler* handler);

LogResponseHandler* handler = new LogResponseHandler(...);
handler->StartFetch(..., base::Bind(&Caller::MyMethod, caller_as_a_weak_ptr,
base::Owned(handler)));
handler = nullptr;

The difference in this API contract is that it separates out the lifetime
management to being the responsibility of the creator entirely, whereas here,
this API contract places some restrictions (which are equally hard to discern
with respect to failure_callback)

Now, if you didn't go the Owned approach (perfectly reasonable), you could and
should still focus on what *your* class needs rather than what *callers* need.
You can still leave the ownership up to the invoker, but rather than smuggling
the |this| pointer back in the callback, leave it up to the invoker to curry
this classes pointer themselves, since they're responsible for lifetime
management.

[Eran Messeri, 2016/01/25 17:07:38]

I've dropped the LogResponseHandler* from the DoneCallback.
I've still left  a typedef for it as it receives one parameter - the callback to
invoke after deleting the LogProofFetcher instance (that callback is prepared by
the LogResponseHandler, with the result of the fetch, so cannot be another bound
parameter to DoneCallback, thus preventing it from being a base::Closure).

+
+  // |log_id| will be passed to the callback to indicate which log
+  // this failure pretains to.

[Ryan Sleevi, 2016/01/23 00:34:23]

Which callback? failure_callback?

[Eran Messeri, 2016/01/25 17:07:37]

Done.

+  // All requests could fail so the |failure_callback| is shared to all
+  // request types.

[Ryan Sleevi, 2016/01/23 00:34:23]

I have trouble parsing this, such that I'm not sure how to suggest rewording.

[Eran Messeri, 2016/01/25 17:07:38]

This was meant to be a documentation of a design decision - that the
|failure_callback| is in the base class because all types of requests have to
handle failure.
I've simply removed this comment.

+  LogResponseHandler(
+      const std::string& log_id,
+      const LogProofFetcher::FetchFailedCallback& failure_callback);
+  virtual ~LogResponseHandler();
+
+  // Starts the actual fetching from the URL, storing |done_callback| for
+  // invocation when fetching and parsing of the request finished.

[Ryan Sleevi, 2016/01/23 00:34:23]

It sounds like it's safe to delete this object in the |done_callback|, which
seems like it should be documented as such.

[Eran Messeri, 2016/01/25 17:07:37]

Done.

+  void StartFetch(net::URLRequestContext* request_context,
+                  const GURL& request_url,
+                  const DoneCallback& done_callback);
+
+  // Handle successful fetch by the LogFetcher (by parsing the JSON and
+  // handing the parsed JSON to HandleParsedJson, which is request-specific).
+  void HandleFetchCompletion();
+
+  // Handle network failure to complete the request to the log, by invoking
+  // the |done_callback_|.
+  virtual void HandleNetFailure(int net_error, int http_response_code);
+
+ protected:
+  // Handle successful parsing of JSON by invoking HandleParsedJson, then
+  // invoking the |done_callback_| with the returned Closure.
+  void OnJsonParseSuccess(scoped_ptr<base::Value> parsed_json);
+  // Handle failure to parse the JSON by invoking HandleJsonParseFailure, then

[Ryan Sleevi, 2016/01/23 00:34:23]

newline between 245 and 246

[Eran Messeri, 2016/01/25 17:07:37]

Done.

+  // invoking the |done_callback_| with the returned Closure.
+  void OnJsonParseError(const std::string& error);
+
+  // Handle respones JSON that parsed successfully, usually by
+  // returning the success callback bound to parsed values as a Closure.
+  virtual base::Closure HandleParsedJson(const base::Value& parsed_json) = 0;
+
+  // Handle failure to parse response JSON, usually by returning the failure
+  // callback bound to a request-specific net error code.
+  virtual base::Closure HandleJsonParseFailure(
+      const std::string& json_error) = 0;
+
+  const std::string log_id_;
+  LogProofFetcher::FetchFailedCallback failure_callback_;
+  scoped_ptr<LogFetcher> fetcher_;
+  DoneCallback done_callback_;
+
+  base::WeakPtrFactory<LogProofFetcher::LogResponseHandler> weak_factory_;
+};
+
+LogProofFetcher::LogResponseHandler::LogResponseHandler(
+    const std::string& log_id,
+    const LogProofFetcher::FetchFailedCallback& failure_callback)
+    : log_id_(log_id),
+      failure_callback_(failure_callback),
+      fetcher_(nullptr),
+      weak_factory_(this) {
+  DCHECK(!failure_callback_.is_null());
+}
 
-  // Delete FetchState and URLRequest, then the entry from inflight_requests_.
-  STLDeleteContainerPairPointers(it, next_it);
-  inflight_requests_.erase(it);
+LogProofFetcher::LogResponseHandler::~LogResponseHandler() {}
+
+void LogProofFetcher::LogResponseHandler::StartFetch(
+    net::URLRequestContext* request_context,
+    const GURL& request_url,
+    const DoneCallback& done_callback) {
+  base::Closure success_callback =
+      base::Bind(&LogProofFetcher::LogResponseHandler::HandleFetchCompletion,
+                 weak_factory_.GetWeakPtr());
+  LogFetcher::FailureCallback failure_callback =

[Ryan Sleevi, 2016/01/23 00:34:22]

I'm not used to see temporaries here like this. That's not to say it's wrong,
I'm just... not used to it :) Normally we base::Bind() chain directly, AFAIK.

For example, you do this on lines 298-301, which is why the internal
inconsistency stood out to me.

[Eran Messeri, 2016/01/25 17:07:38]

Done.

+      base::Bind(&LogProofFetcher::LogResponseHandler::HandleNetFailure,
+                 weak_factory_.GetWeakPtr());
+
+  done_callback_ = done_callback;
+  fetcher_.reset(new LogFetcher(request_context, request_url, success_callback,
+                                failure_callback));
 }
 
-void LogProofFetcher::InvokeFailureCallback(net::URLRequest* request,
-                                            int net_error,
-                                            int http_response_code) {
-  DCHECK(inflight_requests_.count(request));
-  auto it = inflight_requests_.find(request);
-  FetchState* fetch_state = it->second;
+void LogProofFetcher::LogResponseHandler::HandleFetchCompletion() {
+  safe_json::SafeJsonParser::Parse(
+      fetcher_->assembled_response(),
+      base::Bind(&LogProofFetcher::LogResponseHandler::OnJsonParseSuccess,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&LogProofFetcher::LogResponseHandler::OnJsonParseError,
+                 weak_factory_.GetWeakPtr()));
+
+  // the assembled_response string is copied into the SafeJsonParser so it

[Ryan Sleevi, 2016/01/23 00:34:23]

grammar: s/the/The/

[Eran Messeri, 2016/01/25 17:07:38]

Done.

+  // is safe to get rid of the object that owns it.
+  fetcher_.reset();
+}
 
-  fetch_state->failed_callback.Run(fetch_state->log_id, net_error,
-                                   http_response_code);
-  CleanupRequest(request);
+void LogProofFetcher::LogResponseHandler::HandleNetFailure(
+    int net_error,
+    int http_response_code) {
+  fetcher_.reset();
+  LogProofFetcher::FetchFailedCallback failure_callback =
+      base::ResetAndReturn(&failure_callback_);
+
+  base::Closure bound_failure =
+      base::Bind(failure_callback, log_id_, net_error, http_response_code);

[Ryan Sleevi, 2016/01/23 00:34:23]

same remark re: temporaries as arguments (bound_failure)

[Eran Messeri, 2016/01/25 17:07:38]

Done.

+  base::ResetAndReturn(&done_callback_).Run(this, bound_failure);
+  // NOTE: |this| is not valid after the |done_callback_| was invoked.

[Ryan Sleevi, 2016/01/23 00:34:22]

grammar: tense agreement (is vs was)

|this| is no longer valid after |done_callback_| is invoked.

[Eran Messeri, 2016/01/25 17:07:38]

Done.

 }
 
-void LogProofFetcher::OnSTHJsonParseSuccess(
-    net::URLRequest* request,
+void LogProofFetcher::LogResponseHandler::OnJsonParseSuccess(
     scoped_ptr<base::Value> parsed_json) {
-  DCHECK(inflight_requests_.count(request));
-
-  FetchState* fetch_state = inflight_requests_.find(request)->second;
-  net::ct::SignedTreeHead signed_tree_head;
-  if (net::ct::FillSignedTreeHead(*parsed_json.get(), &signed_tree_head)) {
-    fetch_state->fetched_callback.Run(fetch_state->log_id, signed_tree_head);
-  } else {
-    fetch_state->failed_callback.Run(fetch_state->log_id,
-                                     net::ERR_CT_STH_INCOMPLETE, net::HTTP_OK);
+  base::Closure bound_success_callback = HandleParsedJson(*parsed_json);

[Ryan Sleevi, 2016/01/23 00:34:23]

same remark re: temporary

[Eran Messeri, 2016/01/25 17:07:37]

Done.

+  base::ResetAndReturn(&done_callback_).Run(this, bound_success_callback);
+  // NOTE: |this| is not valid after the |done_callback_| was invoked.

[Ryan Sleevi, 2016/01/23 00:34:23]

same nit

[Eran Messeri, 2016/01/25 17:07:37]

Done.

+}
+
+void LogProofFetcher::LogResponseHandler::OnJsonParseError(
+    const std::string& error) {
+  base::Closure bound_failure_callback = HandleJsonParseFailure(error);
+  base::ResetAndReturn(&done_callback_).Run(this, bound_failure_callback);
+  // NOTE: |this| is not valid after the |done_callback_| was invoked.

[Ryan Sleevi, 2016/01/23 00:34:23]

Same remarks to both

[Eran Messeri, 2016/01/25 17:07:38]

Done.

+}
+
+class LogProofFetcher::GetSTHLogResponseHandler
+    : public LogProofFetcher::LogResponseHandler {
+ public:
+  GetSTHLogResponseHandler(
+      const std::string& log_id,
+      const LogProofFetcher::SignedTreeHeadFetchedCallback& sth_fetch_callback,
+      const LogProofFetcher::FetchFailedCallback& failure_callback)
+      : LogResponseHandler(log_id, failure_callback),
+        sth_fetched_(sth_fetch_callback) {}
+
+  // Fill a net::ct::SignedTreeHead instance from the parsed JSON and, if
+  // successful, invoke the success callback with this STH.

[Ryan Sleevi, 2016/01/23 00:34:23]

I originally had a grammar nit about "this STH" vs "the STH" (the this implies
there are multiple instances, hence the need to specify, but there aren't)...

But more generally, this seems to go against the declarative vs imperative
comment guidance of
https://google.github.io/styleguide/cppguide.html#Function_Comments

// Parses the JSON into a net::ct::SignedTreeHead and, if successful, invokes
the
// success callback with it. Otherwise, invokes the failure callback indicating
// the error that occurred.

[Eran Messeri, 2016/01/25 17:07:38]

Done.

+  // Otherwise, invoke the failure callback.
+  base::Closure HandleParsedJson(const base::Value& parsed_json) override {
+    net::ct::SignedTreeHead signed_tree_head;
+    if (!net::ct::FillSignedTreeHead(parsed_json, &signed_tree_head)) {
+      LogProofFetcher::FetchFailedCallback failure_callback =
+          base::ResetAndReturn(&failure_callback_);

[Ryan Sleevi, 2016/01/23 00:34:22]

Unnecessary temporaries (similarly throughout)

[Eran Messeri, 2016/01/25 17:07:37]

Done throughout. 

+      return base::Bind(failure_callback, log_id_, net::ERR_CT_STH_INCOMPLETE,
+                        net::HTTP_OK);
+    }
+
+    LogProofFetcher::SignedTreeHeadFetchedCallback sth_fetched =
+        base::ResetAndReturn(&sth_fetched_);
+    return base::Bind(sth_fetched, log_id_, signed_tree_head);
   }
 
-  CleanupRequest(request);
+  // Invoke the error callback indicating that STH parsing failed.
+  base::Closure HandleJsonParseFailure(const std::string& json_error) override {
+    LogProofFetcher::FetchFailedCallback failure_callback =
+        base::ResetAndReturn(&failure_callback_);
+    return base::Bind(failure_callback, log_id_, net::ERR_CT_STH_PARSING_FAILED,
+                      net::HTTP_OK);
+  }
+
+ private:
+  LogProofFetcher::SignedTreeHeadFetchedCallback sth_fetched_;
+};
+
+class LogProofFetcher::GetConsistencyProofLogResponseHandler
+    : public LogProofFetcher::LogResponseHandler {
+ public:
+  GetConsistencyProofLogResponseHandler(
+      const std::string& log_id,
+      const LogProofFetcher::ConsistencyProofFetchedCallback&
+          proof_fetch_callback,
+      const LogProofFetcher::FetchFailedCallback& failure_callback)
+      : LogResponseHandler(log_id, failure_callback),
+        proof_fetched_(proof_fetch_callback) {}
+
+  // Fill a vector of strings with nodes from the received consistency proof
+  // and, if successful, invoke the success callback with this vector.
+  // Otherwise, invoke the failure callback indicating proof parsing has failed.

[Ryan Sleevi, 2016/01/23 00:34:23]

Similar comment-level remarks.

[Eran Messeri, 2016/01/25 17:07:38]

Done.

+  base::Closure HandleParsedJson(const base::Value& parsed_json) override {
+    std::vector<std::string> consistency_proof;
+    if (!net::ct::FillConsistencyProof(parsed_json, &consistency_proof)) {
+      LogProofFetcher::FetchFailedCallback failure_callback =
+          base::ResetAndReturn(&failure_callback_);
+      return base::Bind(failure_callback, log_id_,
+                        net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED,
+                        net::HTTP_OK);
+    }
+
+    LogProofFetcher::ConsistencyProofFetchedCallback fetched_callback =
+        base::ResetAndReturn(&proof_fetched_);
+    return base::Bind(fetched_callback, log_id_, consistency_proof);
+  }
+
+  // Invoke the error callback indicating proof fetching failed.
+  base::Closure HandleJsonParseFailure(const std::string& json_error) override {
+    LogProofFetcher::FetchFailedCallback failure_callback =
+        base::ResetAndReturn(&failure_callback_);
+    return base::Bind(failure_callback, log_id_,
+                      net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED,
+                      net::HTTP_OK);
+  }
+
+ private:
+  LogProofFetcher::ConsistencyProofFetchedCallback proof_fetched_;
+};
+
+LogProofFetcher::LogProofFetcher(net::URLRequestContext* request_context)
+    : request_context_(request_context), weak_factory_(this) {
+  DCHECK(request_context);
+}
+
+LogProofFetcher::~LogProofFetcher() {
+  STLDeleteContainerPointers(inflight_fetches_.begin(),
+                             inflight_fetches_.end());
 }
 
-void LogProofFetcher::OnSTHJsonParseError(net::URLRequest* request,
-                                          const std::string& error) {
-  InvokeFailureCallback(request, net::ERR_CT_STH_PARSING_FAILED, net::HTTP_OK);
+void LogProofFetcher::FetchSignedTreeHead(
+    const GURL& base_log_url,
+    const std::string& log_id,
+    const SignedTreeHeadFetchedCallback& fetched_callback,
+    const FetchFailedCallback& failed_callback) {
+  GURL request_url = base_log_url.Resolve("ct/v1/get-sth");
+  StartFetch(request_url, new GetSTHLogResponseHandler(log_id, fetched_callback,
+                                                       failed_callback));
+}
+
+void LogProofFetcher::FetchConsistencyProof(
+    const GURL& base_log_url,
+    const std::string& log_id,
+    uint64_t old_tree_size,
+    uint64_t new_tree_size,
+    const ConsistencyProofFetchedCallback& fetched_callback,
+    const FetchFailedCallback& failed_callback) {
+  GURL request_url = base_log_url.Resolve(base::StringPrintf(
+      "ct/v1/get-sth-consistency?first=%" PRIu64 "&second=%" PRIu64,
+      old_tree_size, new_tree_size));
+  StartFetch(request_url, new GetConsistencyProofLogResponseHandler(
+                              log_id, fetched_callback, failed_callback));
+}
+
+void LogProofFetcher::StartFetch(const GURL& request_url,
+                                 LogResponseHandler* log_request) {
+  LogResponseHandler::DoneCallback done_callback =
+      base::Bind(&LogProofFetcher::OnFetchDone, weak_factory_.GetWeakPtr());
+
+  log_request->StartFetch(request_context_, request_url, done_callback);
+  inflight_fetches_.insert(log_request);
+}
+
+void LogProofFetcher::OnFetchDone(LogResponseHandler* log_handler,
+                                  const base::Closure& requestor_callback) {
+  auto it = inflight_fetches_.find(log_handler);
+  DCHECK(it != inflight_fetches_.end());
+
+  delete *it;
+  inflight_fetches_.erase(it);
+  requestor_callback.Run();
 }
 
 }  // namespace certificate_transparency