Implement redirect handling on MojoAsyncResourceHandler

content/browser/loader/mojo_async_resource_handler.cc

Index: content/browser/loader/mojo_async_resource_handler.cc
diff --git a/content/browser/loader/mojo_async_resource_handler.cc b/content/browser/loader/mojo_async_resource_handler.cc
index 6d5a9d7c4a08f396856ebee72852a861a6a3eb21..21961837a45cc8e98486d9ff154e429381ab8937 100644
--- a/content/browser/loader/mojo_async_resource_handler.cc
+++ b/content/browser/loader/mojo_async_resource_handler.cc
@@ -127,8 +127,23 @@ bool MojoAsyncResourceHandler::OnRequestRedirected(
     const net::RedirectInfo& redirect_info,
     ResourceResponse* response,
     bool* defer) {
-  // Not implemented.
-  return false;
+  // Unlike OnResponseReceived, OnRequestRedirected will NOT be preceded by
+  // OnWillRead.
+  DCHECK(!shared_writer_);
+
+  *defer = true;
+  OnDefer();
+
+  NetLogObserver::PopulateResponseInfo(request(), response);
+  response->head.encoded_data_length = request()->GetTotalReceivedBytes();
+  response->head.request_start = request()->creation_time();
+  response->head.response_start = base::TimeTicks::Now();
+  // TODO(davidben): Is it necessary to pass the new first party URL for
+  // cookies? The only case where it can change is top-level navigation requests
+  // and hopefully those will eventually all be owned by the browser. It's
+  // possible this is still needed while renderer-owned ones exist.
+  url_loader_client_->OnReceiveRedirect(redirect_info, response->head);
+  return true;
 }
 
 bool MojoAsyncResourceHandler::OnResponseStarted(ResourceResponse* response,
@@ -245,11 +260,19 @@ void MojoAsyncResourceHandler::OnDataDownloaded(int bytes_downloaded) {
 }
 
 void MojoAsyncResourceHandler::FollowRedirect() {
-  NOTIMPLEMENTED();
+  if (!request()->status().is_success()) {
+    DVLOG(1) << "OnFollowRedirect for invalid request";
+    return;
+  }
+
+  DCHECK(did_defer_);

[dcheng, 2016/11/10 07:09:27]

Is it safe to allow this to be called multiple times? This is called from the
renderer, and an exploited renderer could just invoke FollowRedirect() multiple
times.

(Basically, I'm wondering if this should be a DCHECK or if it's something we
need to check for and handle)

[mmenke, 2016/11/10 21:42:40]

Good question...It looks like it's actually safe (We tear down the redirect and
try again), but that's just luck.  We should probably just kill the renderer if
this happens.

[yhirano, 2016/11/11 09:47:03]

Done. It's now cancelling the request, I don't know what is the preferred manner
for the malformed request with mojo.

+  did_defer_ = false;
+  request()->LogUnblocked();
+  controller()->Resume();
 }
 
-void MojoAsyncResourceHandler::ResumeForTesting() {
-  Resume();
+void MojoAsyncResourceHandler::OnWritableForTesting() {
+  OnWritable(MOJO_RESULT_OK);
 }
 
 void MojoAsyncResourceHandler::SetAllocationSizeForTesting(size_t size) {
@@ -351,7 +374,24 @@ bool MojoAsyncResourceHandler::AllocateWriterIOBuffer(
   return true;
 }
 
-void MojoAsyncResourceHandler::Resume() {
+void MojoAsyncResourceHandler::OnDefer() {
+  request()->LogBlockedBy("MojoAsyncResourceHandler");
+  did_defer_ = true;
+}
+
+bool MojoAsyncResourceHandler::CheckForSufficientResource() {
+  if (has_checked_for_sufficient_resources_)
+    return true;
+  has_checked_for_sufficient_resources_ = true;
+
+  if (rdh_->HasSufficientResourcesForRequest(request()))
+    return true;
+
+  controller()->CancelWithError(net::ERR_INSUFFICIENT_RESOURCES);
+  return false;
+}
+
+void MojoAsyncResourceHandler::OnWritable(MojoResult unused) {

[mmenke, 2016/11/10 21:42:40]

nit:  I think the style is to not name unused variables?  Also should echo the
name in the header.  "MojoResult /*result*/" or "/*=result*/", or whatnot.

[yhirano, 2016/11/11 09:47:03]

Done.

   if (!did_defer_)
     return;
   did_defer_ = false;
@@ -380,27 +420,6 @@ void MojoAsyncResourceHandler::Resume() {
   controller()->Resume();
 }
 
-void MojoAsyncResourceHandler::OnDefer() {
-  request()->LogBlockedBy("MojoAsyncResourceHandler");
-  did_defer_ = true;
-}
-
-bool MojoAsyncResourceHandler::CheckForSufficientResource() {
-  if (has_checked_for_sufficient_resources_)
-    return true;
-  has_checked_for_sufficient_resources_ = true;
-
-  if (rdh_->HasSufficientResourcesForRequest(request()))
-    return true;
-
-  controller()->CancelWithError(net::ERR_INSUFFICIENT_RESOURCES);
-  return false;
-}
-
-void MojoAsyncResourceHandler::OnWritable(MojoResult unused) {
-  Resume();
-}
-
 void MojoAsyncResourceHandler::Cancel() {
   const ResourceRequestInfoImpl* info = GetRequestInfo();
   ResourceDispatcherHostImpl::Get()->CancelRequestFromRenderer(