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, 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) {
   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.
   if (bytes_read == -1 || !request_->status().is_success()) {
     ResponseCompleted();
     return;
   }
 
   CompleteRead(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;
 
   if (bytes_read > 0) {
     StartReading(true);  // Read the next chunk.
   } else {
     // TODO(darin): Remove ScopedTracker below once crbug.com/475761 is fixed.
     tracked_objects::ScopedTracker tracking_profile(
         FROM_HERE_WITH_EXPLICIT_FUNCTION("475761 ResponseCompleted()"));
 
@@ skipping 161 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.
+  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