Use modifed URLRequest API in net/cert_net

This cl splits the big CL with changes to net/ folder and
has changes only to net/cert_net according to the
changes to URLRequest API.

What the big cl does:
It modifies net/ clients that use URLRequest API as long as
URLRequest::Read returns int net errors and
URLRequest::Delegate and NetworkDelegate methods
(for example, OnResponseStarted or OnCompleted) have int
net_error in the arguments now.

The reason behind splitting the CL into small one is that
an android bot started to be unstable and unittests became
flaky. It was not possible to locate the problem and the
decision was to split the CL and upload small parts with a
6+ hours interval in order to make it possible to locate
the problematic code.

The big CL is located here -
https://codereview.chromium.org/2265873002/

BUG=423484

Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9
Review-Url: https://codereview.chromium.org/2330873002
Cr-Original-Commit-Position: refs/heads/master@{#418014}
Cr-Commit-Position: refs/heads/master@{#418852}

net/cert_net/cert_net_fetcher_impl.cc

@@ -178,7 +178,7 @@ class CertNetFetcherImpl::Job : public URLRequest::Delegate {
   void OnReceivedRedirect(URLRequest* request,
                           const RedirectInfo& redirect_info,
                           bool* defer_redirect) override;
-  void OnResponseStarted(URLRequest* request) override;
+  void OnResponseStarted(URLRequest* request, int net_error) override;
   void OnReadCompleted(URLRequest* request, int bytes_read) override;
 
   // Clears the URLRequest and timer. Helper for doing work common to
@@ -192,17 +192,17 @@ class CertNetFetcherImpl::Job : public URLRequest::Delegate {
   // 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);
+  void OnUrlRequestCompleted(int net_error);
 
   // 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_ and response_body_).
-  void OnJobCompleted();
+  // method is called, the |response_body_| variable have been assigned.
+  void OnJobCompleted(Error error);
+
+  // Cancels a request with a specified error code and calls
+  // OnUrlRequestCompleted().
+  void FailRequest(Error error);
 
   // The requests attached to this job.
   RequestList requests_;
@@ -212,7 +212,6 @@ class CertNetFetcherImpl::Job : public URLRequest::Delegate {
 
   // The URLRequest response information.
   std::vector<uint8_t> response_body_;
-  Error result_net_error_;
 
   std::unique_ptr<URLRequest> url_request_;
   scoped_refptr<IOBuffer> read_buffer_;
@@ -235,7 +234,6 @@ CertNetFetcherImpl::RequestImpl::~RequestImpl() {
 CertNetFetcherImpl::Job::Job(std::unique_ptr<RequestParams> request_params,
                              CertNetFetcherImpl* parent)
     : request_params_(std::move(request_params)),
-      result_net_error_(ERR_IO_PENDING),
       parent_(parent) {}
 
 CertNetFetcherImpl::Job::~Job() {
@@ -280,10 +278,11 @@ void CertNetFetcherImpl::Job::DetachRequest(RequestImpl* request) {
 void CertNetFetcherImpl::Job::StartURLRequest(URLRequestContext* context) {
   Error error = CanFetchUrl(request_params_->url);
   if (error != OK) {
-    result_net_error_ = error;
     // The CertNetFetcher's API contract is that requests always complete
     // asynchronously. Use the timer class so the task is easily cancelled.
-    timer_.Start(FROM_HERE, base::TimeDelta(), this, &Job::OnJobCompleted);
+    timer_.Start(
+        FROM_HERE, base::TimeDelta(),
+        base::Bind(&Job::OnJobCompleted, base::Unretained(this), error));
     return;
   }
 
@@ -299,7 +298,9 @@ void CertNetFetcherImpl::Job::StartURLRequest(URLRequestContext* context) {
 
   // 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);
+    timer_.Start(
+        FROM_HERE, request_params_->timeout,
+        base::Bind(&Job::FailRequest, base::Unretained(this), ERR_TIMED_OUT));
 }
 
 void CertNetFetcherImpl::Job::OnReceivedRedirect(
@@ -311,24 +312,24 @@ void CertNetFetcherImpl::Job::OnReceivedRedirect(
   // Ensure that the new URL matches the policy.
   Error error = CanFetchUrl(redirect_info.new_url);
   if (error != OK) {
-    request->CancelWithError(error);
-    OnUrlRequestCompleted(request);
+    FailRequest(error);
     return;
   }
 }
 
-void CertNetFetcherImpl::Job::OnResponseStarted(URLRequest* request) {
+void CertNetFetcherImpl::Job::OnResponseStarted(URLRequest* request,
+                                                int net_error) {
   DCHECK_EQ(url_request_.get(), request);
+  DCHECK_NE(ERR_IO_PENDING, net_error);
 
-  if (!request->status().is_success()) {
-    OnUrlRequestCompleted(request);
+  if (net_error != OK) {
+    OnUrlRequestCompleted(net_error);
     return;
   }
 
   if (request->GetResponseCode() != 200) {
     // TODO(eroman): Use a more specific error code.
-    request->CancelWithError(ERR_FAILED);
-    OnUrlRequestCompleted(request);
+    FailRequest(ERR_FAILED);
     return;
   }
 
@@ -338,6 +339,7 @@ void CertNetFetcherImpl::Job::OnResponseStarted(URLRequest* request) {
 void CertNetFetcherImpl::Job::OnReadCompleted(URLRequest* request,
                                               int bytes_read) {
   DCHECK_EQ(url_request_.get(), request);
+  DCHECK_NE(ERR_IO_PENDING, bytes_read);
 
   // Keep reading the response body.
   if (ConsumeBytesRead(request, bytes_read))
@@ -351,31 +353,30 @@ void CertNetFetcherImpl::Job::Stop() {
 
 void CertNetFetcherImpl::Job::ReadBody(URLRequest* request) {
   // Read as many bytes as are available synchronously.
-  int num_bytes;
-  while (
-      request->Read(read_buffer_.get(), kReadBufferSizeInBytes, &num_bytes)) {
+  int num_bytes = 0;
+  while (num_bytes >= 0) {
+    num_bytes = request->Read(read_buffer_.get(), kReadBufferSizeInBytes);
+    if (num_bytes == ERR_IO_PENDING)
+      return;
     if (!ConsumeBytesRead(request, num_bytes))
       return;
   }
 
-  // Check whether the read failed synchronously.
-  if (!request->status().is_io_pending())
-    OnUrlRequestCompleted(request);
-  return;
+  OnUrlRequestCompleted(num_bytes);
 }
 
 bool CertNetFetcherImpl::Job::ConsumeBytesRead(URLRequest* request,
                                                int num_bytes) {
+  DCHECK_NE(ERR_IO_PENDING, num_bytes);
   if (num_bytes <= 0) {
     // Error while reading, or EOF.
-    OnUrlRequestCompleted(request);
+    OnUrlRequestCompleted(num_bytes);
     return false;
   }
 
   // Enforce maximum size bound.
   if (num_bytes + response_body_.size() > request_params_->max_response_bytes) {
-    request->CancelWithError(ERR_FILE_TOO_BIG);
-    OnUrlRequestCompleted(request);
+    FailRequest(ERR_FILE_TOO_BIG);
     return false;
   }
 
@@ -386,24 +387,14 @@ bool CertNetFetcherImpl::Job::ConsumeBytesRead(URLRequest* request,
   return true;
 }
 
-void CertNetFetcherImpl::Job::OnTimeout() {
-  result_net_error_ = ERR_TIMED_OUT;
-  url_request_->CancelWithError(result_net_error_);
-  OnJobCompleted();
+void CertNetFetcherImpl::Job::OnUrlRequestCompleted(int net_error) {
+  DCHECK_NE(ERR_IO_PENDING, net_error);
+  Error result = static_cast<Error>(net_error);
+  OnJobCompleted(result);
 }
 
-void CertNetFetcherImpl::Job::OnUrlRequestCompleted(URLRequest* request) {
-  DCHECK_EQ(request, url_request_.get());
-
-  if (request->status().is_success())
-    result_net_error_ = OK;
-  else
-    result_net_error_ = static_cast<Error>(request->status().error());
-
-  OnJobCompleted();
-}
-
-void CertNetFetcherImpl::Job::OnJobCompleted() {
+void CertNetFetcherImpl::Job::OnJobCompleted(Error error) {
+  DCHECK_NE(ERR_IO_PENDING, error);
   // Stop the timer and clear the URLRequest.
   Stop();
 
@@ -419,13 +410,19 @@ void CertNetFetcherImpl::Job::OnJobCompleted() {
   while (!requests_.empty()) {
     base::LinkNode<RequestImpl>* request = requests_.head();
     request->RemoveFromList();
-    request->value()->OnJobCompleted(this, result_net_error_, response_body_);
+    request->value()->OnJobCompleted(this, error, response_body_);
   }
 
   if (parent_)
     parent_->ClearCurrentlyCompletingJob(this);
 }
 
+void CertNetFetcherImpl::Job::FailRequest(Error error) {
+  DCHECK_NE(ERR_IO_PENDING, error);
+  int result = url_request_->CancelWithError(error);
+  OnUrlRequestCompleted(result);
+}
+
 CertNetFetcherImpl::CertNetFetcherImpl(URLRequestContext* context)
     : currently_completing_job_(nullptr), context_(context) {
 }

net/cert_net/nss_ocsp.cc

@@ -299,16 +299,17 @@ class OCSPRequestSession
     }
   }
 
-  void OnResponseStarted(URLRequest* request) override {
+  void OnResponseStarted(URLRequest* request, int net_error) override {
     DCHECK_EQ(request_.get(), request);
     DCHECK_EQ(base::MessageLoopForIO::current(), io_loop_);
+    DCHECK_NE(ERR_IO_PENDING, net_error);
 
     int bytes_read = 0;
-    if (request->status().is_success()) {
+    if (net_error == OK) {
       response_code_ = request_->GetResponseCode();
       response_headers_ = request_->response_headers();
       response_headers_->GetMimeType(&response_content_type_);
-      request_->Read(buffer_.get(), kRecvBufferSize, &bytes_read);
+      bytes_read = request_->Read(buffer_.get(), kRecvBufferSize);
     }
     OnReadCompleted(request_.get(), bytes_read);
   }
@@ -317,13 +318,12 @@ class OCSPRequestSession
     DCHECK_EQ(request_.get(), request);
     DCHECK_EQ(base::MessageLoopForIO::current(), io_loop_);
 
-    do {
-      if (!request_->status().is_success() || bytes_read <= 0)
-        break;
+    while (bytes_read > 0) {
       data_.append(buffer_->data(), bytes_read);
-    } while (request_->Read(buffer_.get(), kRecvBufferSize, &bytes_read));
+      bytes_read = request_->Read(buffer_.get(), kRecvBufferSize);
+    }
 
-    if (!request_->status().is_io_pending()) {
+    if (bytes_read != ERR_IO_PENDING) {
       request_.reset();
       g_ocsp_io_loop.Get().RemoveRequest(this);
       {