Fix a pair of ResourceLoader cancellation/error bugs.

content/browser/loader/resource_loader.cc

Index: content/browser/loader/resource_loader.cc
diff --git a/content/browser/loader/resource_loader.cc b/content/browser/loader/resource_loader.cc
index f2f670a566d55ed03d811df7269c241b66a3e1bf..6c3574e2a9ff3ef743300be78f21434918a502d3 100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -351,13 +351,13 @@ void ResourceLoader::OnResponseStarted(net::URLRequest* unused) {
 
   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) {
@@ -563,7 +563,8 @@ void ResourceLoader::StartReading(bool is_continuation) {
   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) {