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::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
 
 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();
 }
 
+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!

   *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.
   // TODO(ahendrickson): DCHECK() that it is not done after
   // http://crbug.com/115705 is fixed.
   if (job_->is_done())
@@ skipping 79 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_->NotifyOnResponseStarted(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.
+  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