Make URLRequest::Read to return net errors or bytes read instead of a bool

net/url_request/url_request.cc

 // Copyright (c) 2012 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/url_request/url_request.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 141 matching lines @@
     SSLCertRequestInfo* cert_request_info) {
   request->CancelWithError(ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
 }
 
 void URLRequest::Delegate::OnSSLCertificateError(URLRequest* request,
                                                  const SSLInfo& ssl_info,
                                                  bool is_hsts_ok) {
   request->Cancel();
 }
 
+void URLRequest::Delegate::OnResponseStarted(URLRequest* request,
+                                             int net_error) {
+  OnResponseStarted(request);
+}
+
+void URLRequest::Delegate::OnResponseStarted(URLRequest* request) {
+  NOTREACHED();
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // URLRequest
 
 URLRequest::~URLRequest() {
   Cancel();
 
   if (network_delegate_) {
     network_delegate_->NotifyURLRequestDestroyed(this);
     if (job_.get())
       job_->NotifyURLRequestDestroyed();
@@ skipping 317 matching lines @@
 
 void URLRequest::set_delegate(Delegate* delegate) {
   DCHECK(!delegate_);
   DCHECK(delegate);
   delegate_ = delegate;
 }
 
 void URLRequest::Start() {
   DCHECK(delegate_);
 
+  if (!status_.is_success())
+    return;
+
   // TODO(pkasting): Remove ScopedTracker below once crbug.com/456327 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("456327 URLRequest::Start"));
 
   // Some values can be NULL, but the job factory must not be.
   DCHECK(context_->job_factory());
 
   // Anything that sets |blocked_by_| before start should have cleaned up after
   // itself.
   DCHECK(blocked_by_.empty());
@@ skipping 39 matching lines @@
       network_delegate_(network_delegate ? network_delegate
                                          : context->network_delegate()),
       net_log_(
           BoundNetLog::Make(context->net_log(), NetLog::SOURCE_URL_REQUEST)),
       url_chain_(1, url),
       method_("GET"),
       referrer_policy_(CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE),
       first_party_url_policy_(NEVER_CHANGE_FIRST_PARTY_URL),
       load_flags_(LOAD_NORMAL),
       delegate_(delegate),
+      status_(URLRequestStatus::FromError(OK)),
       is_pending_(false),
       is_redirecting_(false),
       redirect_limit_(kMaxRedirects),
       priority_(priority),
       identifier_(GenerateURLRequestIdentifier()),
       calling_delegate_(false),
       use_blocked_by_as_load_param_(false),
       before_request_callback_(base::Bind(&URLRequest::BeforeRequestComplete,
                                           base::Unretained(this))),
       has_notified_completion_(false),
@@ skipping 97 matching lines @@
   RestartWithJob(
       URLRequestJobManager::GetInstance()->CreateJob(this, network_delegate_));
 }
 
 void URLRequest::RestartWithJob(URLRequestJob *job) {
   DCHECK(job->request() == this);
   PrepareToRestart();
   StartJob(job);
 }
 
-void URLRequest::Cancel() {
-  DoCancel(ERR_ABORTED, SSLInfo());
+int URLRequest::Cancel() {
+  return DoCancel(ERR_ABORTED, SSLInfo());
 }
 
-void URLRequest::CancelWithError(int error) {
-  DoCancel(error, SSLInfo());
+int URLRequest::CancelWithError(int error) {
+  return DoCancel(error, SSLInfo());
 }
 
 void URLRequest::CancelWithSSLError(int error, const SSLInfo& ssl_info) {
   // This should only be called on a started request.
   if (!is_pending_ || !job_.get() || job_->has_response_started()) {
     NOTREACHED();
     return;
   }
   DoCancel(error, ssl_info);
 }
 
-void URLRequest::DoCancel(int error, const SSLInfo& ssl_info) {
+int URLRequest::DoCancel(int error, const SSLInfo& ssl_info) {
   DCHECK(error < 0);
   // If cancelled while calling a delegate, clear delegate info.
   if (calling_delegate_) {
     LogUnblocked();
     OnCallToDelegateComplete();
   }
 
   // If the URL request already has an error status, then canceling is a no-op.
   // Plus, we don't want to change the error status once it has been set.
   if (status_.is_success()) {
@@ skipping 12 matching lines @@
     job_->Kill();
 
   // We need to notify about the end of this job here synchronously. The
   // Job sends an asynchronous notification but by the time this is processed,
   // our |context_| is NULL.
   NotifyRequestCompleted();
 
   // The Job will call our NotifyDone method asynchronously.  This is done so
   // that the Delegate implementation can call Cancel without having to worry
   // about being called recursively.
+
+  return status_.error();
 }
 
-bool URLRequest::Read(IOBuffer* dest, int dest_size, int* bytes_read) {
+int URLRequest::Read(IOBuffer* dest, int dest_size) {
   DCHECK(job_.get());
-  DCHECK(bytes_read);
-  *bytes_read = 0;
 
   // If this is the first read, end the delegate call that may have started in
   // OnResponseStarted.
   OnCallToDelegateComplete();
 
-  // This handles a cancel that happens while paused.
+  // If the request has failed, Read() will return actual network error code.
+  if (!status_.is_success())
+    return status_.error();
+
+  // This handles reads after the request already completed successfully.
   // TODO(ahendrickson): DCHECK() that it is not done after
   // http://crbug.com/115705 is fixed.
   if (job_->is_done())
-    return false;
+    return status_.error();
 
   if (dest_size == 0) {
     // Caller is not too bright.  I guess we've done what they asked.
-    return true;
+    return OK;
   }
 
-  // Once the request fails or is cancelled, read will just return 0 bytes
-  // to indicate end of stream.
-  if (!status_.is_success()) {
-    return true;
-  }
+  int rv = job_->Read(dest, dest_size);
+  if (rv == ERR_IO_PENDING)
+    set_status(URLRequestStatus::FromError(ERR_IO_PENDING));
 
-  bool rv = job_->Read(dest, dest_size, bytes_read);
-  if (*bytes_read < 0) {
-    if (*bytes_read == ERR_IO_PENDING) {
-      set_status(URLRequestStatus::FromError(ERR_IO_PENDING));
-      // Check the status was set correctly.
-      DCHECK_EQ(*bytes_read, status_.error());
-      // Adjust to the previous behavior. If the error is ERR_IO_PENDING,
-      // |*bytes_read| should be 0.
-      *bytes_read = 0;
-    } else {
-      // Check the status was set correctly.
-      DCHECK_EQ(*bytes_read, status_.error());
-      // Adjust to the previous behavior. If the error is other than
-      // ERR_IO_PENDING, |*bytes_read| should be -1.
-      *bytes_read = -1;
-    }
-  }
+  // If rv is not 0 or actual bytes read, the status cannot be success.
+  DCHECK(rv >= 0 || status_.status() != URLRequestStatus::SUCCESS);
 
-  // If rv is false, the status cannot be success.
-  DCHECK(rv || status_.status() != URLRequestStatus::SUCCESS);
-
-  if (rv && *bytes_read <= 0 && status_.is_success())
+  if (rv == 0 && status_.is_success())
     NotifyRequestCompleted();
   return rv;
 }
 
+// Deprecated.
+bool URLRequest::Read(IOBuffer* dest, int dest_size, int* bytes_read) {
+  int result = Read(dest, dest_size);
+  if (result >= 0) {
+    *bytes_read = result;
+    return true;
+  }
+
+  if (result == ERR_IO_PENDING) {
+    *bytes_read = 0;
+  } else {
+    *bytes_read = -1;
+  }
+
+  return false;
+}
+
 void URLRequest::StopCaching() {
   DCHECK(job_.get());
   job_->StopCaching();
 }
 
 void URLRequest::NotifyReceivedRedirect(const RedirectInfo& redirect_info,
                                         bool* defer_redirect) {
   is_redirecting_ = true;
 
   // TODO(davidben): Pass the full RedirectInfo down to MaybeInterceptRedirect?
@@ skipping 30 matching lines @@
   URLRequestJob* job =
       URLRequestJobManager::GetInstance()->MaybeInterceptResponse(
           this, network_delegate_);
   if (job) {
     RestartWithJob(job);
   } else {
     // In some cases (e.g. an event was canceled), we might have sent the
     // completion event and receive a NotifyResponseStarted() later.
     if (!has_notified_completion_ && status_.is_success()) {
       if (network_delegate_)
-        network_delegate_->NotifyResponseStarted(this);
+        network_delegate_->NotifyResponseStarted(this, net_error);
     }
 
     // Notify in case the entire URL Request has been finished.
     if (!has_notified_completion_ && !status_.is_success())
       NotifyRequestCompleted();
 
     OnCallToDelegate();
-    delegate_->OnResponseStarted(this);
+    delegate_->OnResponseStarted(this, net_error);
     // Nothing may appear below this line as OnResponseStarted may delete
     // |this|.
   }
 }
 
 void URLRequest::FollowDeferredRedirect() {
   DCHECK(job_.get());
   DCHECK(status_.is_success());
 
   status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
@@ skipping 285 matching lines @@
 }
 
 
 void URLRequest::NotifyReadCompleted(int bytes_read) {
   if (bytes_read > 0)
     set_status(URLRequestStatus());
   // Notify in case the entire URL Request has been finished.
   if (bytes_read <= 0)
     NotifyRequestCompleted();
 
+  // When URLRequestJob notices there was an error in URLRequest's |status_|,
+  // it calls this method with |bytes_read| set to -1. Set it to a real error
+  // here.
+  // TODO(maksims): NotifyReadCompleted take the error code as an argument on
+  // failure, rather than -1.
+  if (bytes_read == -1)
+    bytes_read = status_.error();
+
   // Notify NetworkChangeNotifier that we just received network data.
   // This is to identify cases where the NetworkChangeNotifier thinks we
   // are off-line but we are still receiving network data (crbug.com/124069),
   // and to get rough network connection measurements.
   if (bytes_read > 0 && !was_cached())
     NetworkChangeNotifier::NotifyDataReceived(*this, bytes_read);
 
   delegate_->OnReadCompleted(this, bytes_read);
 
   // Nothing below this line as OnReadCompleted may delete |this|.
@@ skipping 24 matching lines @@
 void URLRequest::NotifyRequestCompleted() {
   // TODO(battre): Get rid of this check, according to willchan it should
   // not be needed.
   if (has_notified_completion_)
     return;
 
   is_pending_ = false;
   is_redirecting_ = false;
   has_notified_completion_ = true;
   if (network_delegate_)
-    network_delegate_->NotifyCompleted(this, job_.get() != NULL);
+    network_delegate_->NotifyCompleted(this, job_.get() != NULL,
+                                       status_.error());
 }
 
 void URLRequest::OnCallToDelegate() {
   DCHECK(!calling_delegate_);
   DCHECK(blocked_by_.empty());
   calling_delegate_ = true;
   net_log_.BeginEvent(NetLog::TYPE_URL_REQUEST_DELEGATE);
 }
 
 void URLRequest::OnCallToDelegateComplete() {
@@ skipping 12 matching lines @@
     out->clear();
 }
 
 void URLRequest::set_status(URLRequestStatus status) {
   DCHECK(status_.is_io_pending() || status_.is_success() ||
          (!status.is_success() && !status.is_io_pending()));
   status_ = status;
 }
 
 }  // namespace net