ResourceLoader: Fix a bunch of double-cancellation/double-error notification cases.

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 338 matching lines @@
   }
 
   CompleteResponseStarted();
 
   // 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;
 
-  StartReading(false);  // Read the first chunk.
+  ReadMore(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 canceled, either the request will call into |this|
   // with a bogus read error, or, if the request was completed, a task posted
   // from ResourceLoader::CancelREquestInternal will run OnResponseCompleted.
   if (is_deferred() || !request_->status().is_success())
     return;
 
   if (bytes_read > 0) {
-    StartReading(true);  // Read the next chunk.
+    ReadMore(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()"));
 
     // URLRequest reported an EOF. Call ResponseCompleted.
     DCHECK_EQ(0, bytes_read);
     ResponseCompleted();
   }
 }
@@ skipping 151 matching lines @@
 
   bool defer = false;
   if (!handler_->OnResponseStarted(response.get(), &defer)) {
     Cancel();
   } else if (defer) {
     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.
-  // 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,
-                   weak_ptr_factory_.GetWeakPtr(), request_.get(), bytes_read));
-  }
-}
-
-void ResourceLoader::ResumeReading() {
-  DCHECK(!is_deferred());
-
-  if (!read_deferral_start_time_.is_null()) {
-    UMA_HISTOGRAM_TIMES("Net.ResourceLoader.ReadDeferral",
-                        base::TimeTicks::Now() - read_deferral_start_time_);
-    read_deferral_start_time_ = base::TimeTicks();
-  }
-  if (request_->status().is_success()) {
-    StartReading(false);  // Read the next chunk (OK to complete synchronously).
-  } else {
-    ResponseCompleted();
-  }
-}
-
-void ResourceLoader::ReadMore(int* bytes_read) {
+void ResourceLoader::ReadMore(bool is_continuation) {

[Randy Smith (Not in Mondays), 2016/12/06 23:44:43]

This file looks like pure refactoring rather than adapting to the new
URLRequest::Read contract around not dispatching an asynchronous call to
OnReadCompleted on synchronous error returns.  What am I missing?

[mmenke, 2016/12/07 16:17:15]

In the cancel path from the old ReadMore(), we want to exit immediately and let
ResourceLoader::Cancel() handle it, but in the sync read error path, we need to
pass the error on.  So either we'd have to have ReadMore return a value to
distinguish those two paths, or we need to just merge the methods.  Since there
was only one call site, went with the latter option.

I should have mentioned when I sent this CL out - the reason for this change is
completely non-obvious.

   TRACE_EVENT_WITH_FLOW0("loading", "ResourceLoader::ReadMore", this,
                          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
   DCHECK(!is_deferred());
 
   // Make sure we track the buffer in at least one place.  This ensures it gets
   // deleted even in the case the request has already finished its job and
   // doesn't use the buffer.
   scoped_refptr<net::IOBuffer> buf;
   int buf_size;
   {
     // TODO(darin): Remove ScopedTracker below once crbug.com/475761 is fixed.
     tracked_objects::ScopedTracker tracking_profile2(
         FROM_HERE_WITH_EXPLICIT_FUNCTION("475761 OnWillRead()"));
 
     if (!handler_->OnWillRead(&buf, &buf_size, -1)) {
+      // Cancel the request, which will then call back into |this| to it of

[Randy Smith (Not in Mondays), 2016/12/06 23:44:43]

nit: "to it"?

[mmenke, 2016/12/07 16:17:15]

Done.

+      // a "read error".
       Cancel();
       return;
     }
   }
 
   DCHECK(buf.get());
   DCHECK(buf_size > 0);
 
-  request_->Read(buf.get(), buf_size, bytes_read);
+  int result = request_->Read(buf.get(), buf_size);
 
-  // No need to check the return value here as we'll detect errors by
-  // inspecting the URLRequest's status.
+  if (result == net::ERR_IO_PENDING)
+    return;
+
+  if (!is_continuation || result <= 0) {
+    OnReadCompleted(request_.get(), result);
+  } 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,
+                   weak_ptr_factory_.GetWeakPtr(), request_.get(), result));
+  }
+}
+
+void ResourceLoader::ResumeReading() {
+  DCHECK(!is_deferred());
+
+  if (!read_deferral_start_time_.is_null()) {
+    UMA_HISTOGRAM_TIMES("Net.ResourceLoader.ReadDeferral",
+                        base::TimeTicks::Now() - read_deferral_start_time_);
+    read_deferral_start_time_ = base::TimeTicks();
+  }
+  if (request_->status().is_success()) {
+    ReadMore(false);  // Read the next chunk (OK to complete synchronously).
+  } else {
+    ResponseCompleted();
+  }
 }
 
 void ResourceLoader::CompleteRead(int bytes_read) {
   TRACE_EVENT_WITH_FLOW0("loading", "ResourceLoader::CompleteRead", this,
                          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
 
   DCHECK(bytes_read >= 0);
   DCHECK(request_->status().is_success());
 
   // TODO(darin): Remove ScopedTracker below once crbug.com/475761 is fixed.
@@ skipping 104 matching lines @@
       UMA_HISTOGRAM_ENUMERATION("Net.Prefetch.Pattern", prefetch_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