Refactor ResourceHandler API.

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"
 #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"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #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"
 #include "content/browser/service_worker/service_worker_request_handler.h"
 #include "content/browser/service_worker/service_worker_response_info.h"
 #include "content/browser/ssl/ssl_client_auth_handler.h"
 #include "content/browser/ssl/ssl_manager.h"
 #include "content/public/browser/resource_dispatcher_host_login_delegate.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_client.h"
@@ skipping 93 matching lines @@
     // We should not have any SSL state.
     DCHECK(!request->ssl_info().cert_status);
     DCHECK_EQ(request->ssl_info().security_bits, -1);
     DCHECK_EQ(request->ssl_info().key_exchange_group, 0);
     DCHECK(!request->ssl_info().connection_status);
   }
 }
 
 }  // 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);
+    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)
     : deferred_stage_(DEFERRED_NONE),
       request_(std::move(request)),
       handler_(std::move(handler)),
       delegate_(delegate),
       is_transferring_(false),
       times_cancelled_before_request_start_(0),
       started_request_(false),
       times_cancelled_after_request_start_(0),
       weak_ptr_factory_(this) {
   request_->set_delegate(this);
-  handler_->SetController(this);
+  handler_->SetDelegate(this);
 }
 
 ResourceLoader::~ResourceLoader() {
   if (login_delegate_.get())
     login_delegate_->OnRequestCancelled();
   ssl_client_auth_handler_.reset();
 
   // Run ResourceHandler destructor before we tear-down the rest of our state
   // as the ResourceHandler may want to inspect the URLRequest and other state.
   handler_.reset();
 }
 
 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) {
   TRACE_EVENT_WITH_FLOW0("loading", "ResourceLoader::CancelRequest", this,
                          TRACE_EVENT_FLAG_FLOW_IN);
   CancelRequestInternal(net::ERR_ABORTED, from_renderer);
 }
 
 void ResourceLoader::CancelAndIgnore() {
   ResourceRequestInfoImpl* info = GetRequestInfo();
@@ skipping 48 matching lines @@
 }
 
 ResourceRequestInfoImpl* ResourceLoader::GetRequestInfo() {
   return ResourceRequestInfoImpl::ForRequest(request_.get());
 }
 
 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) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("loading"),
                "ResourceLoader::OnReceivedRedirect");
   DCHECK_EQ(request_.get(), unused);
 
   DVLOG(1) << "OnReceivedRedirect: " << request_->url().spec();
   DCHECK(request_->status().is_success());
 
@@ skipping 12 matching lines @@
 
       // Tell the renderer that this request was disallowed.
       Cancel();
       return;
     }
   }
 
   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();
   }
 }
 
 void ResourceLoader::OnAuthRequired(net::URLRequest* unused,
                                     net::AuthChallengeInfo* auth_info) {
   DCHECK_EQ(request_.get(), unused);
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
   if (info->do_not_prompt_for_login()) {
     request_->CancelAuth();
@@ skipping 43 matching lines @@
   DCHECK_EQ(request_.get(), unused);
 
   DVLOG(1) << "OnResponseStarted: " << request_->url().spec();
 
   if (!request_->status().is_success()) {
     ResponseCompleted();
     return;
   }
 
   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) {
   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) {
-    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,
                                       const net::SSLInfo* ssl_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // The request can be NULL if it was cancelled by the renderer (as the
   // request of the user navigating to a new page from the location bar).
   if (!request_->is_pending())
     return;
@@ skipping 25 matching lines @@
       net::FetchClientCertPrivateKey(cert);
   request_->ContinueWithCertificate(cert, private_key.get());
 }
 
 void ResourceLoader::CancelCertificateSelection() {
   DCHECK(ssl_client_auth_handler_);
   ssl_client_auth_handler_.reset();
   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_;
   deferred_stage_ = DEFERRED_NONE;
   switch (stage) {
     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
+        // aboid blocking the IO thread.

[Charlie Harrison, 2017/01/27 22:48:05]

avoid

[mmenke, 2017/01/27 23:02:15]

Done.

+        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;
   }
 }
 
 void ResourceLoader::Cancel() {
   CancelRequest(false);
 }
 
 void ResourceLoader::StartRequestInternal() {
   DCHECK(!request_->is_pending());
@@ skipping 76 matching lines @@
   ResourceRequestInfoImpl* info = GetRequestInfo();
   scoped_refptr<ResourceResponse> response = new ResourceResponse();
   PopulateResourceResponse(info, request_.get(), response.get());
 
   delegate_->DidReceiveResponse(this);
 
   // TODO(darin): Remove ScopedTracker below once crbug.com/475761 is fixed.
   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) {
   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;
   {
@@ skipping 10 matching lines @@
   }
 
   DCHECK(buf.get());
   DCHECK(buf_size > 0);
 
   int result = request_->Read(buf.get(), buf_size);
 
   if (result == net::ERR_IO_PENDING)
     return;
 
-  if (!is_continuation || result <= 0) {
+  if (handle_result_asyncronously || result <= 0) {

[Charlie Harrison, 2017/01/27 22:48:05]

Do you mean !handle_result_asynchronously? This looks wrong.

[mmenke, 2017/01/27 23:02:15]

Yes I do...

Worth noting this wasn't caught by tests (Because of how I changed things). 
Should probably write a new test for that.

[mmenke, 2017/01/27 23:05:10]

Actually, this can't be tested - it's just to avoid blocking the message loop
while reading a file.  So I'd have to test how many times we re-entered the
message loop...ok, maybe testable, but such a test seems destined to be flaky
and weird.  WDYT?

     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).

[Charlie Harrison, 2017/01/27 22:48:05]

Remove "(Ok to complete synchronously)" and add /* handle_result_asyncronously
*/ inline

[mmenke, 2017/01/27 23:02:15]

Done.

   } 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.
   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() {
   TRACE_EVENT_WITH_FLOW0("loading", "ResourceLoader::ResponseCompleted", this,
                          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
 
   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() {
   TRACE_EVENT_WITH_FLOW0("loading", "ResourceLoader::CallDidFinishLoading",
                          this, TRACE_EVENT_FLAG_FLOW_IN);
   delegate_->DidFinishLoading(this);
 }
 
 void ResourceLoader::RecordHistograms() {
   ResourceRequestInfoImpl* info = GetRequestInfo();
@@ skipping 54 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