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

net/url_request/url_request.cc

Index: net/url_request/url_request.cc
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index eac30aeb4157f9f54ea97f57c3ce1c6d2a91fbb6..627b38d61ed735d61099ded093fcc27418c49af6 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -159,6 +159,25 @@ void URLRequest::Delegate::OnSSLCertificateError(URLRequest* request,
   request->Cancel();
 }
 
+void URLRequest::Delegate::NotifyOnResponseStarted(URLRequest* request,
+                                                   int net_error) {
+  DCHECK(request);
+
+  implemented_ = true;
+
+  OnResponseStarted(request, net_error);
+  if (!implemented_)

[mmenke, 2016/08/26 19:36:48]

The issue with the failures is that the OnResponseStarted call may result in the
URLRequest being deleted, so this will access freed memory.

The way to do this is to get rid of this method, and remove implemented_.

Instead, make the default OnResponseStarted(URLRequest* request, int net_error)
implementation call OnResponseStarted(URLRequest* request), and make the default
OnResponseStarted(URLRequest* request) just contain a "NOTREACHED", to prevent
people from relying on both default implementations to do weird things.

[maksims (do not use this acc), 2016/08/29 12:30:35]

Oh, thanks! Good to know this trick!

+    OnResponseStarted(request);
+}
+
+void URLRequest::Delegate::OnResponseStarted(URLRequest* request,
+                                             int net_error) {
+  NOTIMPLEMENTED();
+  implemented_ = false;
+}
+
+void URLRequest::Delegate::OnResponseStarted(URLRequest* request) {}
+
 ///////////////////////////////////////////////////////////////////////////////
 // URLRequest
 
@@ -496,6 +515,9 @@ void URLRequest::set_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"));
@@ -555,6 +577,7 @@ URLRequest::URLRequest(const GURL& url,
       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),
@@ -672,12 +695,12 @@ void URLRequest::RestartWithJob(URLRequestJob *job) {
   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) {
@@ -689,7 +712,7 @@ void URLRequest::CancelWithSSLError(int error, const SSLInfo& ssl_info) {
   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_) {
@@ -722,8 +745,46 @@ void URLRequest::DoCancel(int error, const SSLInfo& ssl_info) {
   // 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();
+}
+
+int URLRequest::Read(IOBuffer* dest, int dest_size) {
+  DCHECK(job_.get());
+
+  // If this is the first read, end the delegate call that may have started in
+  // OnResponseStarted.
+  OnCallToDelegateComplete();
+
+  // Once the request fails or is cancelled, read will just return 0 bytes
+  // to indicate end of stream.
+  if (!status_.is_success()) {

[mmenke, 2016/08/26 19:36:48]

Can this happen, if job_->is_done() is false?

(Also, if so, should remove the braces here)

[maksims (do not use this acc), 2016/08/29 12:30:35]

Yes, if URLRequestJob::NotifyStartError() is called.

+    return status_.error();
+  }
+
+  // This handles a cancel that happens while paused.
+  // TODO(ahendrickson): DCHECK() that it is not done after
+  // http://crbug.com/115705 is fixed.
+  if (job_->is_done())
+    return status_.error();
+
+  if (dest_size == 0) {
+    // Caller is not too bright.  I guess we've done what they asked.
+    return OK;
+  }
+
+  int rv = job_->Read(dest, dest_size);
+  if (rv == ERR_IO_PENDING)
+    set_status(URLRequestStatus::FromError(ERR_IO_PENDING));
+
+  // If rv is not 0 or actual bytes read, the status cannot be success.
+  DCHECK(!(rv < 0) || status_.status() != URLRequestStatus::SUCCESS);

[mmenke, 2016/08/26 19:36:48]

Think rv >= 0 is easier to read, for the first check.

[maksims (do not use this acc), 2016/08/29 12:30:35]

Done.

+
+  if (rv == 0 && status_.is_success())
+    NotifyRequestCompleted();
+  return rv;
 }
 
+// Deprecated.
 bool URLRequest::Read(IOBuffer* dest, int dest_size, int* bytes_read) {
   DCHECK(job_.get());
   DCHECK(bytes_read);

[mmenke, 2016/08/26 19:36:48]

Isn't everything below this the same as:

int result = Read(dest, dest_size);
if (result == ERR_IO_PENDING) {
  // set *bytes_read to 0, return 0.
}
if (result < ERR_IO_PENDING) {
  // set *bytes_read to 0, return -1.
}

etc.

Then you can get rid of the old URLRequestJob::Read, and don't need duplicated
code here.  That also gets us more testing of the new URLRequest[Job]::Read
methods.

[maksims (do not use this acc), 2016/08/29 12:30:35]

Right! Thanks!

@@ -826,7 +887,7 @@ void URLRequest::NotifyResponseStarted(const URLRequestStatus& status) {
     // 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.
@@ -834,7 +895,7 @@ void URLRequest::NotifyResponseStarted(const URLRequestStatus& status) {
       NotifyRequestCompleted();
 
     OnCallToDelegate();
-    delegate_->OnResponseStarted(this);
+    delegate_->NotifyOnResponseStarted(this, net_error);
     // Nothing may appear below this line as OnResponseStarted may delete
     // |this|.
   }
@@ -1140,6 +1201,12 @@ void URLRequest::NotifyReadCompleted(int bytes_read) {
   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.
+  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),
@@ -1184,7 +1251,8 @@ void URLRequest::NotifyRequestCompleted() {
   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() {