Refactor ResourceHandler API.

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 241727bad07cb3e69264224a4116031ba0ebb952..1afa016c959841eafc480a2fd049e40f55a6980c 100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -9,6 +9,7 @@
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
@@ -17,6 +18,7 @@
 #include "content/browser/appcache/appcache_interceptor.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/loader/detachable_resource_handler.h"
+#include "content/browser/loader/resource_controller.h"
 #include "content/browser/loader/resource_handler.h"
 #include "content/browser/loader/resource_loader_delegate.h"
 #include "content/browser/loader/resource_request_info_impl.h"
@@ -130,6 +132,88 @@ void PopulateResourceResponse(ResourceRequestInfoImpl* info,
 
 }  // namespace
 
+class ResourceLoader::Controller : public ResourceController {
+ public:
+  explicit Controller(ResourceLoader* resource_loader)
+      : resource_loader_(resource_loader){};
+
+  ~Controller() override {}
+
+  // ResourceController implementation:
+  void Resume() override {
+    MarkAsUsed();
+    resource_loader_->Resume(true /* called_from_resource_controller */);
+  }
+
+  void Cancel() override {
+    MarkAsUsed();
+    resource_loader_->Cancel();
+  }
+
+  void CancelAndIgnore() override {
+    MarkAsUsed();
+    resource_loader_->CancelAndIgnore();
+  }
+
+  void CancelWithError(int error_code) override {
+    MarkAsUsed();
+    resource_loader_->CancelWithError(error_code);
+  }
+
+ private:
+  void MarkAsUsed() {
+#if DCHECK_IS_ON()
+    DCHECK(!used_);
+    used_ = true;
+#endif
+  }
+
+  ResourceLoader* const resource_loader_;
+
+#if DCHECK_IS_ON()
+  // Set to true once one of the ResourceContoller methods has been invoked.
+  bool used_ = false;
+#endif
+
+  DISALLOW_COPY_AND_ASSIGN(Controller);
+};
+
+// Helper class.  Sets the stage of a ResourceLoader to DEFERRED_SYNC on
+// construction, and on destruction does one of the following:
+// 1) If the ResourceLoader has a deferred stage of DEFERRED_NONE, sets the
+// ResourceLoader's stage to the stage specified on construction and resumes it.
+// 2) If the ResourceLoader still has a deferred stage of DEFERRED_SYNC, sets
+// the ResourceLoader's stage to the stage specified on construction.  The
+// ResourceLoader will be resumed at some point in the future.
+class ResourceLoader::ScopedDeferral {
+ public:
+  ScopedDeferral(ResourceLoader* resource_loader,
+                 ResourceLoader::DeferredStage deferred_stage)
+      : resource_loader_(resource_loader), deferred_stage_(deferred_stage) {
+    resource_loader_->deferred_stage_ = DEFERRED_SYNC;
+  }
+
+  ~ScopedDeferral() {
+    DeferredStage old_deferred_stage = resource_loader_->deferred_stage_;
+    // On destruction, either the stage is still DEFERRED_SYNC, or Resume() was
+    // called once, and it advanced to DEFERRED_NONE.
+    DCHECK(old_deferred_stage == DEFERRED_NONE ||
+           old_deferred_stage == DEFERRED_SYNC)
+        << old_deferred_stage;
+    resource_loader_->deferred_stage_ = deferred_stage_;
+    // If Resume() was called, it just advanced the state without doing
+    // anything. Go ahead and resume the request now.
+    if (old_deferred_stage == DEFERRED_NONE)
+      resource_loader_->Resume(false /* called_from_resource_controller */);
+  }
+
+ private:
+  ResourceLoader* const resource_loader_;
+  const DeferredStage deferred_stage_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedDeferral);
+};
+
 ResourceLoader::ResourceLoader(std::unique_ptr<net::URLRequest> request,
                                std::unique_ptr<ResourceHandler> handler,
                                ResourceLoaderDelegate* delegate)
@@ -143,7 +227,7 @@ ResourceLoader::ResourceLoader(std::unique_ptr<net::URLRequest> request,
       times_cancelled_after_request_start_(0),
       weak_ptr_factory_(this) {
   request_->set_delegate(this);
-  handler_->SetController(this);
+  handler_->SetDelegate(this);
 }
 
 ResourceLoader::~ResourceLoader() {
@@ -157,20 +241,11 @@ ResourceLoader::~ResourceLoader() {
 }
 
 void ResourceLoader::StartRequest() {
-  // Give the handler a chance to delay the URLRequest from being started.
-  bool defer_start = false;
-  if (!handler_->OnWillStart(request_->url(), &defer_start)) {
-    Cancel();
-    return;
-  }
-
   TRACE_EVENT_WITH_FLOW0("loading", "ResourceLoader::StartRequest", this,
                          TRACE_EVENT_FLAG_FLOW_OUT);
-  if (defer_start) {
-    deferred_stage_ = DEFERRED_START;
-  } else {
-    StartRequestInternal();
-  }
+
+  ScopedDeferral scoped_deferral(this, DEFERRED_START);
+  handler_->OnWillStart(request_->url(), base::MakeUnique<Controller>(this));
 }
 
 void ResourceLoader::CancelRequest(bool from_renderer) {
@@ -239,6 +314,10 @@ void ResourceLoader::ClearLoginDelegate() {
   login_delegate_ = NULL;
 }
 
+void ResourceLoader::OutOfBandCancel(int error_code, bool tell_renderer) {
+  CancelRequestInternal(error_code, !tell_renderer);
+}
+
 void ResourceLoader::OnReceivedRedirect(net::URLRequest* unused,
                                         const net::RedirectInfo& redirect_info,
                                         bool* defer) {
@@ -271,16 +350,20 @@ void ResourceLoader::OnReceivedRedirect(net::URLRequest* unused,
   scoped_refptr<ResourceResponse> response = new ResourceResponse();
   PopulateResourceResponse(info, request_.get(), response.get());
   delegate_->DidReceiveRedirect(this, redirect_info.new_url, response.get());
-  if (!handler_->OnRequestRedirected(redirect_info, response.get(), defer)) {
-    Cancel();
-  } else if (*defer) {
-    deferred_stage_ = DEFERRED_REDIRECT;  // Follow redirect when resumed.
-    DCHECK(deferred_redirect_url_.is_empty());
+
+  // Can't used ScopedDeferral here, because on sync completion, need to set
+  // |defer| to false instead of calling back into the URLRequest.
+  deferred_stage_ = DEFERRED_SYNC;
+  handler_->OnRequestRedirected(redirect_info, response.get(),
+                                base::MakeUnique<Controller>(this));
+  if (is_deferred()) {
+    *defer = true;
     deferred_redirect_url_ = redirect_info.new_url;
-  } else if (delegate_->HandleExternalProtocol(this, redirect_info.new_url)) {
-    // The request is complete so we can remove it.
-    CancelAndIgnore();
-    return;
+    deferred_stage_ = DEFERRED_REDIRECT;
+  } else {
+    *defer = false;
+    if (delegate_->HandleExternalProtocol(this, redirect_info.new_url))
+      CancelAndIgnore();
   }
 }
 
@@ -344,14 +427,6 @@ void ResourceLoader::OnResponseStarted(net::URLRequest* unused) {
   }
 
   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;
-
-  ReadMore(false);  // Read the first chunk.
 }
 
 void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
@@ -368,25 +443,6 @@ void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
   }
 
   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) {
-    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();
-  }
 }
 
 void ResourceLoader::CancelSSLRequest(int error,
@@ -432,7 +488,7 @@ void ResourceLoader::CancelCertificateSelection() {
   request_->CancelWithError(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
 }
 
-void ResourceLoader::Resume() {
+void ResourceLoader::Resume(bool called_from_resource_controller) {
   DCHECK(!is_transferring_);
 
   DeferredStage stage = deferred_stage_;
@@ -441,27 +497,54 @@ void ResourceLoader::Resume() {
     case DEFERRED_NONE:
       NOTREACHED();
       break;
+    case DEFERRED_SYNC:
+      DCHECK(called_from_resource_controller);
+      // Request will be resumed when the stack unwinds.
+      break;
     case DEFERRED_START:
+      // URLRequest::Start completes asynchronously, so starting the request now
+      // won't result in synchronously calling into a ResourceHandler, if this
+      // was called from Resume().
       StartRequestInternal();
       break;
     case DEFERRED_REDIRECT:
+      // URLRequest::Start completes asynchronously, so starting the request now
+      // won't result in synchronously calling into a ResourceHandler, if this
+      // was called from Resume().
       FollowDeferredRedirectInternal();
       break;
     case DEFERRED_READ:
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(&ResourceLoader::ResumeReading,
-                                weak_ptr_factory_.GetWeakPtr()));
+      if (called_from_resource_controller) {
+        // TODO(mmenke):  Call ReadMore instead?  Strange that this is the only
+        // path which calls different methods, depending on the path.
+        base::ThreadTaskRunnerHandle::Get()->PostTask(
+            FROM_HERE, base::Bind(&ResourceLoader::ResumeReading,
+                                  weak_ptr_factory_.GetWeakPtr()));
+      } else {
+        // If this was called as a result of a handler succeeding synchronously,
+        // force the result of the next read to be handled asynchronously, to
+        // avoid blocking the IO thread.
+        ReadMore(true /* handle_result_asynchronously */);
+      }
       break;
     case DEFERRED_RESPONSE_COMPLETE:
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(&ResourceLoader::ResponseCompleted,
-                                weak_ptr_factory_.GetWeakPtr()));
+      if (called_from_resource_controller) {
+        base::ThreadTaskRunnerHandle::Get()->PostTask(
+            FROM_HERE, base::Bind(&ResourceLoader::ResponseCompleted,
+                                  weak_ptr_factory_.GetWeakPtr()));
+      } else {
+        ResponseCompleted();
+      }
       break;
     case DEFERRED_FINISH:
-      // Delay self-destruction since we don't know how we were reached.
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(&ResourceLoader::CallDidFinishLoading,
-                                weak_ptr_factory_.GetWeakPtr()));
+      if (called_from_resource_controller) {
+        // Delay self-destruction since we don't know how we were reached.
+        base::ThreadTaskRunnerHandle::Get()->PostTask(
+            FROM_HERE, base::Bind(&ResourceLoader::CallDidFinishLoading,
+                                  weak_ptr_factory_.GetWeakPtr()));
+      } else {
+        CallDidFinishLoading();
+      }
       break;
   }
 }
@@ -558,16 +641,13 @@ void ResourceLoader::CompleteResponseStarted() {
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("475761 OnResponseStarted()"));
 
-  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.
-  }
+  read_deferral_start_time_ = base::TimeTicks::Now();
+  ScopedDeferral scoped_deferral(this, DEFERRED_READ);
+  handler_->OnResponseStarted(response.get(),
+                              base::MakeUnique<Controller>(this));
 }
 
-void ResourceLoader::ReadMore(bool is_continuation) {
+void ResourceLoader::ReadMore(bool handle_result_asyncronously) {

[Charlie Harrison, 2017/01/27 23:15:27]

nit: s/asyncronously/asynchronously and ditto below. Took me way too long to see
why that word looked weird to me.

[mmenke, 2017/01/28 04:49:35]

Think I'll go with async.  :)

   TRACE_EVENT_WITH_FLOW0("loading", "ResourceLoader::ReadMore", this,
                          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
   DCHECK(!is_deferred());
@@ -598,7 +678,7 @@ void ResourceLoader::ReadMore(bool is_continuation) {
   if (result == net::ERR_IO_PENDING)
     return;
 
-  if (!is_continuation || result <= 0) {
+  if (!handle_result_asyncronously || result <= 0) {
     OnReadCompleted(request_.get(), result);
   } else {
     // Else, trigger OnReadCompleted asynchronously to avoid starving the IO
@@ -619,7 +699,7 @@ void ResourceLoader::ResumeReading() {
     read_deferral_start_time_ = base::TimeTicks();
   }
   if (request_->status().is_success()) {
-    ReadMore(false);  // Read the next chunk (OK to complete synchronously).
+    ReadMore(false /* handle_result_asynchronously */);
   } else {
     ResponseCompleted();
   }
@@ -636,18 +716,9 @@ void ResourceLoader::CompleteRead(int bytes_read) {
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("475761 OnReadCompleted()"));
 
-  bool defer = false;
-  if (!handler_->OnReadCompleted(bytes_read, &defer)) {
-    Cancel();
-  } else if (defer) {
-    deferred_stage_ =
-        bytes_read > 0 ? DEFERRED_READ : DEFERRED_RESPONSE_COMPLETE;
-  }
-
-  // Note: the request may still have been cancelled while OnReadCompleted
-  // returns true if OnReadCompleted caused request to get cancelled
-  // out-of-band. (In AwResourceDispatcherHostDelegate::DownloadStarting, for
-  // instance.)
+  ScopedDeferral scoped_deferral(
+      this, bytes_read > 0 ? DEFERRED_READ : DEFERRED_RESPONSE_COMPLETE);
+  handler_->OnReadCompleted(bytes_read, base::MakeUnique<Controller>(this));
 }
 
 void ResourceLoader::ResponseCompleted() {
@@ -657,22 +728,13 @@ void ResourceLoader::ResponseCompleted() {
   DVLOG(1) << "ResponseCompleted: " << request_->url().spec();
   RecordHistograms();
 
-  bool defer = false;
-  {
-    // TODO(darin): Remove ScopedTracker below once crbug.com/475761 is fixed.
-    tracked_objects::ScopedTracker tracking_profile(
-        FROM_HERE_WITH_EXPLICIT_FUNCTION("475761 OnResponseCompleted()"));
+  // TODO(darin): Remove ScopedTracker below once crbug.com/475761 is fixed.
+  tracked_objects::ScopedTracker tracking_profile(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION("475761 OnResponseCompleted()"));
 
-    handler_->OnResponseCompleted(request_->status(), &defer);
-  }
-  if (defer) {
-    // The handler is not ready to die yet.  We will call DidFinishLoading when
-    // we resume.
-    deferred_stage_ = DEFERRED_FINISH;
-  } else {
-    // This will result in our destruction.
-    CallDidFinishLoading();
-  }
+  ScopedDeferral scoped_deferral(this, DEFERRED_FINISH);
+  handler_->OnResponseCompleted(request_->status(),
+                                base::MakeUnique<Controller>(this));
 }
 
 void ResourceLoader::CallDidFinishLoading() {