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 {}

[Charlie Harrison, 2017/01/25 20:23:00]

Can we DCHECK(done_) here? I'm nervous about people forgetting to resume /
cancel.

[mmenke, 2017/01/25 22:07:59]

Unfortunately not, due to out-of-band cancels.  :(

+
+  // ResourceController implementation:
+  void Resume() override {
+    DCHECK(!done_);
+    done_ = true;
+    resource_loader_->Resume(true);
+  }
+
+  void Cancel() override {
+    DCHECK(!done_);
+    done_ = true;
+    resource_loader_->Cancel();
+  }
+
+  void CancelAndIgnore() override {
+    DCHECK(!done_);
+    done_ = true;
+    resource_loader_->CancelAndIgnore();
+  }
+
+  void CancelWithError(int error_code) override {
+    DCHECK(!done_);
+    done_ = true;
+    resource_loader_->CancelWithError(error_code);
+  }
+
+ private:
+  ResourceLoader* const resource_loader_;
+
+  // Set to true once one of the ResourceContoller methods has been invoked.
+  bool done_ = false;
+
+  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

[Charlie Harrison, 2017/01/25 20:23:00]

Can the ResourceLoader have deferred stage apart from NONE or SYNC? Of not,
let's DCHECK it.

[mmenke, 2017/01/25 22:07:59]

Good idea, done.  Wonder if it would be cleaner to switch this class to a do
loop and have it work like the others (At some point, not in this CL) - the
ScopedDeferral stuff is a bit unintuitive.  :(

+// 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_;
+    resource_loader_->deferred_stage_ = deferred_stage_;
+    if (old_deferred_stage == DEFERRED_NONE)
+      resource_loader_->Resume(false);
+  }
+
+ 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());
-    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();
+
+  // 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()) {

[Charlie Harrison, 2017/01/25 20:23:00]

optional: I don't think early-return is a more readable pattern here. I would
prefer:

if (is_deferred()) {
  *defer = true;
  deferred_redirect_url_ = redirect_info.new_url;
  deferred_stage_ = DEFERRED_REDIRECT;
} else {
  *defer = false;
  if (delegate->HandleExternalProtocol(...))
    CancelAndIgnore();
}

[mmenke, 2017/01/25 22:08:00]

Done.

+    *defer = false;
+    if (delegate_->HandleExternalProtocol(this, redirect_info.new_url))
+      CancelAndIgnore();
     return;
   }
+
+  *defer = true;
+  deferred_redirect_url_ = redirect_info.new_url;
+  deferred_stage_ = DEFERRED_REDIRECT;
 }
 
 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();
     return;
@@ skipping 42 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:
+      // Starting requests completes asynchronously, so starting the request now

[Charlie Harrison, 2017/01/25 20:23:00]

nit: "so start the request now"? The sentence confuses me.

[mmenke, 2017/01/25 22:08:00]

Done.

       StartRequestInternal();
       break;
     case DEFERRED_REDIRECT:
       FollowDeferredRedirectInternal();
       break;
     case DEFERRED_READ:
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(&ResourceLoader::ResumeReading,
-                                weak_ptr_factory_.GetWeakPtr()));
+      if (!called_from_resource_controller) {

[Charlie Harrison, 2017/01/25 20:23:00]

nit: I would prefer the conditional to be reversed, with the positive case
first, same for all these cases.

[mmenke, 2017/01/25 22:07:59]

Done.

+        ReadMore(called_from_resource_controller);
+      } else {
+        base::ThreadTaskRunnerHandle::Get()->PostTask(
+            FROM_HERE, base::Bind(&ResourceLoader::ResumeReading,
+                                  weak_ptr_factory_.GetWeakPtr()));
+      }
       break;
     case DEFERRED_RESPONSE_COMPLETE:
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(&ResourceLoader::ResponseCompleted,
-                                weak_ptr_factory_.GetWeakPtr()));
+      if (!called_from_resource_controller) {
+        ResponseCompleted();
+      } else {
+        base::ThreadTaskRunnerHandle::Get()->PostTask(
+            FROM_HERE, base::Bind(&ResourceLoader::ResponseCompleted,
+                                  weak_ptr_factory_.GetWeakPtr()));
+      }
       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) {
+        CallDidFinishLoading();
+      } else {
+        // 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()));
+      }
       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 called_from_resource_controller) {
   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 (!called_from_resource_controller || result <= 0) {
     OnReadCompleted(request_.get(), result);

[Charlie Harrison, 2017/01/25 20:23:00]

Isn't it breaking the contract to call OnReadCompleted if
called_from_resource_controller is true? If that contract does not apply to
ReadMore, it should be specified in the header.

[mmenke, 2017/01/25 22:07:59]

Great catch!  I was keeping old behavior here, but you're right.  I don't think
it should be fixed in this CL, though.  CL is too big already, and it needs
tests.

I think we could land this CL without fixing this first, because I don't think
this CL makes things any more likely to crash if we violate that part of the
contract, but it's probably best to land the first first, just in case there's
some implicit assumption I'm unaware of.

[mmenke, 2017/01/25 22:38:04]

One other thing I noticed - the old code mentions explicitly that it also wants 
to limit the number of reads we do to 1 before we do a posttask.  This code
doesn't do that.  I'll also look into fixing that.

[Charlie Harrison, 2017/01/26 00:25:50]

Acknowledged

[mmenke, 2017/01/27 19:14:29]

Ok, I looked into this:  The reason why I couldn't repro is that this wasn't an
issue before this patch, and with this patch, called_from_resource_controller is
always false, anyways, because of the PostTask in Resume().

I've removed the result <= 0 check, gotten rid of that PostTask, and now call
ReadMore with true in Resume, unconditionally, to match (I think?) old behavior,
which was designed to avoid getting stuck in a read loop.

We did have a test that caught this, once I made this an actual bug -
ResourceLoaderTest.AsyncResourceHandler, so I'm not going to add any tests.

I also modified the ResourceHandler API to allow re-entrant calls to OnWillRead
(Which that test also caught), and modified TestResourceHandler not to catch
them.

Sorry for the merge + fixes in the same CL.  Only the resource_loader.cc,
mock_resource_loader.cc, and resource_handler.h changes are relevant (Only merge
conflicts were with redirect_to_file_resource_handler).

   } 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));
   }
 }
 
@@ skipping 16 matching lines @@
   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