Fix a pair of ResourceLoader cancellation/error bugs.

content/browser/loader/resource_loader.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 "content/browser/loader/resource_loader.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
@@ skipping 333 matching lines @@
 
   DVLOG(1) << "OnResponseStarted: " << request_->url().spec();
 
   if (!request_->status().is_success()) {
     ResponseCompleted();
     return;
   }
 
   CompleteResponseStarted();
 
-  if (is_deferred())
+  // If the handler deferred the request, it will resume the request later. If
+  // the request was cancelled, it will call back into |this| to notify it of
+  // the cancellation.

[Randy Smith (Not in Mondays), 2016/12/01 20:12:52]

I'm confused by this comment.  We're dealing here with ResourceHandler
cancellations, correct?  They way I follow the code is that if the RH
synchronously cancels the request, CompleteResponseStarted() will note that and
cancel the request, and ResourceLoader::CancelRequestInternal will post a task
to ResponseCompleted().  So I agree with avoiding ResponseCompleted() here, but
I'm not sure the comment is accurate or precise, and I'd like to have it so to
reduce the chances of these kind of errors creeping back in (though your
refactor will help).  

If I'm missing something, let me know :-}.  If I'm not, what about: 

// If the handler deferred the request, it will resume the request later.  If
the request was 
// cancelled, CompleteResponseStarted() above will have posted a task to call
ResponseCompleted().

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

CompleteResponseStarted() will have cancelled the request, and the request will
call back into us (With a read error...Which, yes, is weird).  I've updated the
wording.

[Randy Smith (Not in Mondays), 2016/12/02 20:38:55]

So just to confirm I understand what's going on:
* If the request *wasn't* pending (which isn't true at this point in the code),
RL::CancelRequestInternal, called from CompleteResponseStarted in the RH
synchronous cancel case, will post a task to run RL::ResponseCompleted, which
will execute *after* this code finishes. 

* The URLRequest::CancelWithError call within CompleteResponseStarted will
(through DoCancel) call URLRequestJob::Kill, which will (NotifyCanceled ->
NotifyDone) result in a PostTask of URLRJ::CompleteNotifyDone, which in the
has_handled_response_ case will call request_->NotifyReadCompleted() which will
end up back in RL, also after this code completes.

* has_handled_response_ != was_pending so these two cases are not necessarily
exclusive-but-one-will-happen.

? 

+  if (is_deferred() || !request_->status().is_success())
     return;
 
-  if (request_->status().is_success())
-    StartReading(false);  // Read the first chunk.
-  else
-    ResponseCompleted();
+  StartReading(false);  // Read the first chunk.
 }
 
 void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("loading"),
                "ResourceLoader::OnReadCompleted");
   DCHECK_EQ(request_.get(), unused);
   DVLOG(1) << "OnReadCompleted: \"" << request_->url().spec() << "\""
            << " bytes_read = " << bytes_read;
 
   // bytes_read == -1 always implies an error.
@@ skipping 185 matching lines @@
     read_deferral_start_time_ = base::TimeTicks::Now();
     deferred_stage_ = DEFERRED_READ;  // Read first chunk when resumed.
   }
 }
 
 void ResourceLoader::StartReading(bool is_continuation) {
   int bytes_read = 0;
   ReadMore(&bytes_read);
 
   // If IO is pending, wait for the URLRequest to call OnReadCompleted.
-  if (request_->status().is_io_pending())
+  // On error or cancellation, wait for notification of failure.

[Randy Smith (Not in Mondays), 2016/12/01 20:12:52]

I'm sure I'm missing a pathway, but ReadMore includes a call into
URLRequest::Read().  If that fails synchronously, how do we find out?

[Randy Smith (Not in Mondays), 2016/12/01 20:12:52]

How about "... wait for arrival of ResponseCompleted() task posted by Cancel()"?
 I'd like to at least gesture at the pathway by which the notification will
occur.

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

Note that Cancel only posts that task if the request is not "pending".  i.e., if
the last read that completed was not EOF.  Otherwise, the URLRequest will call
back into us to tell us about the cancellation.  (In some cases, both happens,
but that's why one of the tests is disabled)

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

On synchronous failure, URLRequest::Read calls back into us on error, in
addition of synchronously telling us about the failure.  The followup CL (Which
will re-enable the DISABLED test) will actually change that, and change how this
code works.  And hopefully not break anything....  I've audited all non-test
URLRequest::Delegates.

[Randy Smith (Not in Mondays), 2016/12/02 20:38:55]

Wow.  I keep feeling like I've gotten to the bottom of the rabbit hole, and I
keep being wrong.  

Will we have a reasonably easy-to-specify interface contract for URLRequest
error signalling  after this CL and the next one?  If not, I'm probably going to
want to request some documentation enhancement around what that contract
actually is in one of those CLs.

+  if (request_->status().is_io_pending() || !request_->status().is_success())
     return;
 
   if (!is_continuation || bytes_read <= 0) {
     OnReadCompleted(request_.get(), bytes_read);
   } else {
     // Else, trigger OnReadCompleted asynchronously to avoid starving the IO
     // thread in case the URLRequest can provide data synchronously.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(&ResourceLoader::OnReadCompleted,
@@ skipping 144 matching lines @@
     }
 
     UMA_HISTOGRAM_ENUMERATION("Net.Prefetch.Pattern", status, STATUS_MAX);
   } else if (request_->response_info().unused_since_prefetch) {
     TimeDelta total_time = base::TimeTicks::Now() - request_->creation_time();
     UMA_HISTOGRAM_TIMES("Net.Prefetch.TimeSpentOnPrefetchHit", total_time);
   }
 }
 
 }  // namespace content