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..c8d07399fc859769613d395ccbb5c2fb7e6bd50a 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"
 
@@ -42,201 +47,394 @@ int GetNetErrorFromURLRequestStatus(const net::URLRequestStatus& status) {
 
 }  // 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,

[Ryan Sleevi, 2016/01/12 22:15:01]

nit: newline between 57 & 58

[Eran Messeri, 2016/01/18 15:50:56]

Done.

+             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 StartNextRead();
+
+  // Invokes the success callback.
+  void RequestComplete();
+  // Invokes the failure callback with the supplied error information.

[Ryan Sleevi, 2016/01/12 22:15:01]

nit: newline between 83 and 84?

[Eran Messeri, 2016/01/18 15:50:56]

Done.

+  // After this method the LogFetcher instance may be deleted and |this|

[Ryan Sleevi, 2016/01/12 22:15:01]

After this method what?

(I believe you mean "After this method is called, the LogFetcher instance may be
deleted")

However, that's a sort of weak API contract - it implies there are cases where
it won't be deleted. And that makes it ambiguous on how to safely call this.

// After this method is called, the LogFetcher is deleted and no longer safe to
call

But if that's the case, then Line 62 should likely be moved into private and we
should have a stronger contract on what the API is.

(Hopefully it's clearer the ambiguity of lifetime; I realize this is all
localized to a .cc, just hoping we can provide strong guarantees one way or
another as to what the lifetime and validity of one of these objects is)

[Eran Messeri, 2016/01/18 15:50:56]

Done - the LogFetcher instance is deleted either by the success or failure
callbacks. I've updated the documentation and code to reflect that.
I did not, however, make the d'tor private as it the LogFetcher will be deleted
by the LogResponseHandler (which owns it).

+  // no longer valid.
+  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),
+      response_buffer_(new net::IOBufferWithSize(
+          LogProofFetcher::kMaxLogResponseSizeInBytes)) {

[mmenke, 2016/01/12 22:43:20]

Optional:  Could initialize this only once we receive the headers.  Reduce the
length of time we use the memory for.

[Eran Messeri, 2016/01/18 15:50:55]

Done - I've moved it to OnResponseStarted, please let me know if that's not a
sensible choice.

+  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

[Ryan Sleevi, 2016/01/12 22:15:01]

grammar: s/data/data, /

[Eran Messeri, 2016/01/18 15:50:56]

Done.

+  // 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::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) {
+void LogProofFetcher::LogFetcher::OnResponseStarted(net::URLRequest* request) {
+  DCHECK_EQ(url_request_.get(), request);
   net::URLRequestStatus status(request->status());

[Ryan Sleevi, 2016/01/12 22:15:01]

Do you actually need to clone status? It seems like you're really just doing

bool had_error = !request->status().is_success()

if (had_error || ...) {
  ..
  if (had_error)
    net_error = ...
}

StartNextRead();

[Eran Messeri, 2016/01/18 15:50:55]

I've simplified the code a bit to always calculate net_error and check net_error
against net::OK - which GetNetErrorFromURLRequestStatus will only return if
request->status().is_success() is true.

-  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;
     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);

[mmenke, 2016/01/12 22:43:20]

Get rid of this, and use GetNetErrorFromURLRequestStatus(...)?

[Eran Messeri, 2016/01/18 15:50:55]

Done.

 
-    InvokeFailureCallback(request, net_error, http_response_code);
+    InvokeFailureCallback(net_error, http_response_code);
     return;
   }
 
-  StartNextRead(request, fetch_state);
+  StartNextRead();
 }
 
-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))
+    StartNextRead();
 }
 
-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()) {
+    net::URLRequestStatus status(url_request_->status());

[Ryan Sleevi, 2016/01/12 22:15:01]

This seems unnecessarily verbose - do you really need the temporary?

[Eran Messeri, 2016/01/18 15:50:56]

Done - removed temporary.

+    InvokeFailureCallback(GetNetErrorFromURLRequestStatus(status), net::OK);
 
     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);

[Ryan Sleevi, 2016/01/12 22:15:01]

Why DCHECK here, given line 173?

[Eran Messeri, 2016/01/18 15:50:56]

The checks later are to make sure we haven't read too much, not just that
bytes_read is non-negative. This DCHECK enforces the contract for calling
HandleReadResult - assuming bytes_read >= 0 implying that there's a valid read
output to handle.
Does that make sense? If it seems superfluous, I could remove this DCHECK.

-  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::StartNextRead() {

[mmenke, 2016/01/12 22:43:20]

ReadLoop(), maybe?

[Eran Messeri, 2016/01/18 15:50:55]

Done.

   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();
 }
 
-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
+  // invoking the callback will be deleted by the callback.
+}
+
+// Interface for handling the response from a CT log for particular
+// requests.
+// All log responses are JSON and should be parsed; however the response
+// to each request should be parsed and validated diffenetly.
+class LogProofFetcher::LogResponseHandler {
+ public:
+  using DoneCallback =
+      base::Callback<void(LogProofFetcher::LogResponseHandler*)>;
+
+  // |log_id| will be passed to the callback to indicate which log
+  // this failure pretains to.
+  // All requests could fail so the |failure_callback| is shared to all
+  // request types.
+  LogResponseHandler(
+      const std::string& log_id,
+      const LogProofFetcher::FetchFailedCallback& failure_callback);
+  virtual ~LogResponseHandler();
+
+  void StartFetch(net::URLRequestContext* request_context,
+                  const GURL& request_url,
+                  const DoneCallback& done_callback);
+
+  // Handle successful fetch by the LogFetcher.
+  void HandleFetchCompletion();
+
+  // Handle network failure to complete the request to the log.
+  virtual void HandleNetFailure(int net_error, int http_response_code);
+
+ protected:
+  void OnJsonParseSuccess(scoped_ptr<base::Value> parsed_json);
+  void OnJsonParseError(const std::string& error);
+
+  // Handle respones JSON that parsed successfully, usually by
+  // invoking the success callback.
+  virtual void HandleParsedJson(const base::Value& parsed_json) = 0;
+
+  // Handle failure to parse response JSON, usually by invoking the failure
+  // callback with a request-specific net error code.
+  virtual void 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 =
+      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()));
+}
 
-  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) {
+  base::ResetAndReturn(&failure_callback_)
+      .Run(log_id_, net_error, http_response_code);
+  base::ResetAndReturn(&done_callback_).Run(this);

[Ryan Sleevi, 2016/01/12 22:15:01]

DESIGN: This strikes me weird that we would invoke both failure_callback_ and
done_callback_, and is usually a source of bugs (invoking multiple callbacks
simultaneously, where one callback may lead to the deletion of |this|)

I think it'd be easier to reason about if you only had one callback being
invoked out from the edge of the graph.

You could also stack-copy the callback (in this case, done_callback_), but I've
also seen errors come from that (double frees), so I'm loathe to encourage it.

[Eran Messeri, 2016/01/18 15:50:56]

Per your suggestion I now have one callback being invoked out from the edge of
the graph:
The LogResponseHandler prepares/passes on a base::Closure that is binding the
output to the success/failure callbacks.
This closure is then passed into the DoneCallback as a parameter. The
DoneCallback is implemented by the LogProofFetcher itself: It will delete the
LogResponseHandler it owns, only then invoke the base::Closure instance prepared
by the LogResponseHandler.

Does this design address your concerns?

 }
 
-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);
+  HandleParsedJson(*parsed_json);
+  base::ResetAndReturn(&done_callback_).Run(this);
+}
+
+void LogProofFetcher::LogResponseHandler::OnJsonParseError(
+    const std::string& error) {
+  HandleJsonParseFailure(error);
+  base::ResetAndReturn(&done_callback_).Run(this);

[Ryan Sleevi, 2016/01/12 22:15:01]

Both of these methods also seem to suffer from the same double-callback exit
(error & done)

It's further compounded that you're using ResetAndReturn in those methods,
except you should be *guaranteed* that's unnecessary because you access the
done_callback_ in these methods, so you know the |this| cannot be invalidated in
HandleJsonParseFailure nor HandleParsedJson, since neither of those callbacks is
permitted to affect |this| in any way (by virtue of reasoning about these
methods)

[Eran Messeri, 2016/01/18 15:50:55]

See design changes I've highlighted above.

+}
+
+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.
+  // Otherwise, invoke the failure callback.
+  void HandleParsedJson(const base::Value& parsed_json) override {
+    net::ct::SignedTreeHead signed_tree_head;
+    if (!net::ct::FillSignedTreeHead(parsed_json, &signed_tree_head)) {
+      base::ResetAndReturn(&failure_callback_)
+          .Run(log_id_, net::ERR_CT_STH_INCOMPLETE, net::HTTP_OK);
+      return;
+    }
+
+    base::ResetAndReturn(&sth_fetched_).Run(log_id_, signed_tree_head);
   }
 
-  CleanupRequest(request);
+  // Invoke the error callback indicating that STH parsing failed.
+  void HandleJsonParseFailure(const std::string& json_error) override {
+    base::ResetAndReturn(&failure_callback_)
+        .Run(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.
+  void HandleParsedJson(const base::Value& parsed_json) override {
+    std::vector<std::string> consistency_proof;
+    if (!net::ct::FillConsistencyProof(parsed_json, &consistency_proof)) {
+      base::ResetAndReturn(&failure_callback_)
+          .Run(log_id_, net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED,
+               net::HTTP_OK);
+      return;
+    }
+
+    base::ResetAndReturn(&proof_fetched_).Run(log_id_, consistency_proof);
+  }
+
+  // Invoke the error callback indicating proof fetching failed.
+  void HandleJsonParseFailure(const std::string& json_error) override {
+    base::ResetAndReturn(&failure_callback_)
+        .Run(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);
 }
 
-void LogProofFetcher::OnSTHJsonParseError(net::URLRequest* request,
-                                          const std::string& error) {
-  InvokeFailureCallback(request, net::ERR_CT_STH_PARSING_FAILED, net::HTTP_OK);
+LogProofFetcher::~LogProofFetcher() {
+  STLDeleteContainerPointers(inflight_fetches_.begin(),
+                             inflight_fetches_.end());
+}
+
+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) {
+  auto it = inflight_fetches_.find(log_handler);
+  DCHECK(it != inflight_fetches_.end());
+
+  auto next = it;
+  std::advance(next, 1);
+
+  STLDeleteContainerPointers(it, next);

[Ryan Sleevi, 2016/01/12 22:15:01]

This (line 433-436) just seems... needlessly complex?

What's it trying to defend against? Why not just

auto it = ..
DCHECK(it != )
delete *it;
inflight_fetches_.erase(it);

[Eran Messeri, 2016/01/18 15:50:55]

Done.

+  inflight_fetches_.erase(it);
 }
 
 }  // namespace certificate_transparency