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..2415efd966835e3f7d9d695145a5b1461ed88da5 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,76 @@ void PopulateResourceResponse(ResourceRequestInfoImpl* info,
 
 }  // 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)
@@ -143,7 +215,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 +229,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 +302,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,17 +338,22 @@ 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());
-    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,
@@ -344,14 +416,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 +432,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 +477,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 +486,44 @@ 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:
+      // 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;
   }
 }
@@ -558,16 +620,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 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());
@@ -598,7 +657,7 @@ void ResourceLoader::ReadMore(bool is_continuation) {
   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
@@ -636,18 +695,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 +707,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() {