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..05bb3a5efbf1d39f056ec962fd3a1f7f6232109a 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, the request will call back into |this| with a
+  // bogus read completed error.
+  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) {
@@ -377,11 +377,9 @@ void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
 
   // If the handler cancelled or deferred the request, do not continue
   // processing the read. If cancelled, the URLRequest has already been
-  // cancelled and will schedule an erroring OnReadCompleted later. If deferred,
-  // do nothing until resumed.
-  //
-  // Note: if bytes_read is 0 (EOF) and the handler defers, resumption will call
-  // ResponseCompleted().
+  // cancelled and either the request will call into |this| with a bogus

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

The "If cancelled" seems to me to refer to the URLRequest and so "the URLRequest
has already been canceled"  seems redundant.

[mmenke, 2016/12/02 21:26:52]

Done.

+  // read error, or a posted task will run OnResponseCompleted, if the
+  // request was completed. If deferred, nothing to do until resumed.

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

Suggest changing to ", or, if the request was completed, a task posted from
ResourceLoader::CancelRequestInternal will run OnResponseCompleted."

(Deciding on how far down the rabbit hole to go in comments is tricky, and if
you don't want that level of detail I'm good with that.  I'm just reading the
above and feeling like "a posted task" is missing some level of context.)

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

I think the last sentence is redundant with the first?  

[mmenke, 2016/12/02 21:26:52]

Done.

[mmenke, 2016/12/02 21:26:52]

Done.

   if (is_deferred() || !request_->status().is_success())
     return;
 
@@ -563,7 +561,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.
+  if (request_->status().is_io_pending() || !request_->status().is_success())
     return;
 
   if (!is_continuation || bytes_read <= 0) {