Teach navigation throttles how to cancel requests in WillProcessResponse.

content/browser/loader/navigation_resource_throttle.cc

Index: content/browser/loader/navigation_resource_throttle.cc
diff --git a/content/browser/loader/navigation_resource_throttle.cc b/content/browser/loader/navigation_resource_throttle.cc
index 44fd0c131ff413fd0d90d0d4c07adae7dedc338b..5b79a98c2ee33f2f0303ffff17a4db493c6d71da 100644
--- a/content/browser/loader/navigation_resource_throttle.cc
+++ b/content/browser/loader/navigation_resource_throttle.cc
@@ -31,6 +31,26 @@ void SendCheckResultToIOThread(UIChecksPerformedCallback callback,
                           base::Bind(callback, result));
 }
 
+void MaybeCommitNavigationAndSendCheckResultToIOThread(
+    UIChecksPerformedCallback callback,
+    int render_process_id,
+    int render_frame_host_id,
+    scoped_refptr<net::HttpResponseHeaders> headers,
+    NavigationThrottle::ThrottleCheckResult result) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);

[mmenke, 2016/01/25 17:57:33]

Should include browser_thread header.

[Mike West, 2016/01/26 09:41:42]

Done.

+  CHECK(result != NavigationThrottle::DEFER);

[mmenke, 2016/01/25 17:57:33]

CHECK_NE is generally preferred.  This file should also include base/logging.h

[Mike West, 2016/01/26 09:41:42]

Done.

+  if (result == NavigationThrottle::PROCEED) {
+    RenderFrameHostImpl* render_frame_host =
+        RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
+    if (render_frame_host && render_frame_host->navigation_handle()) {
+      render_frame_host->navigation_handle()->ReadyToCommitNavigation(

[clamy, 2016/01/25 16:36:21]

Could we make this call part of NavigationHandle: when it's done with
WillProcessResponse and it's not canceled, it can call ReadyToCommitNavigation
internally?

[Mike West, 2016/01/26 09:41:41]

It made more sense to me for NavigationResourceThrottle to remain in control of
the flow, and WillProcessResponse calling ReadyToCommitNavigation felt a little
too magical. *shrug* I don't feel strongly about it; if you do, I'll move it. :)

+          render_frame_host, headers);
+    }
+  }
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(callback, result));
+}
+
 void CheckWillStartRequestOnUIThread(UIChecksPerformedCallback callback,
                                      int render_process_id,
                                      int render_frame_host_id,
@@ -93,21 +113,29 @@ void CheckWillRedirectRequestOnUIThread(
 }
 
 void WillProcessResponseOnUIThread(

[mmenke, 2016/01/25 17:57:33]

This is an old method...but was never being used before?

[Mike West, 2016/01/26 09:41:42]

It was being used, see the PostTask in
NavigationResourceThrottle::WillProcessResponse. We're just adding a callback
method to its signature.

+    UIChecksPerformedCallback callback,
     int render_process_id,
     int render_frame_host_id,
     scoped_refptr<net::HttpResponseHeaders> headers) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RenderFrameHostImpl* render_frame_host =
       RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
-  if (!render_frame_host)
+  if (!render_frame_host) {
+    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);

[mmenke, 2016/01/25 17:57:33]

If there's no RFH, should we really just proceed?

[Mike West, 2016/01/26 09:41:42]

This matches the existing behavior in the other methods; if we don't post
something, we'll just hang while the IO thread waits for the UI thread to finish
its checks. It might make sense to return CANCEL instead if we're not able to do
checks, but I'm honestly not familiar enough with the lifetime of the
RenderFrameHost/NavigationHandle to give a solid answer.

If it's possible to DCHECK instead, I'd be happy doing that. clamy@?

[clamy, 2016/01/26 14:08:54]

I think it's possible for downloads started from the browser. They won't be
associated with a RFH, but we should let them proceed anyway.

     return;
+  }
 
   NavigationHandleImpl* navigation_handle =
       render_frame_host->navigation_handle();
-  if (!navigation_handle)
+  if (!navigation_handle) {
+    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);

[mmenke, 2016/01/25 17:57:33]

Again, when does this happen?

[clamy, 2016/01/26 14:08:54]

Actually this should never happen. We should probably replace it with a
DCHECK(navigation_handle), and in other instances as well. This is a mistake in
the original code.

     return;
+  }
 
-  navigation_handle->ReadyToCommitNavigation(render_frame_host, headers);
+  navigation_handle->WillProcessResponse(
+      headers,
+      base::Bind(&MaybeCommitNavigationAndSendCheckResultToIOThread, callback,
+                 render_process_id, render_frame_host_id, headers));
 }
 
 }  // namespace
@@ -204,10 +232,15 @@ void NavigationResourceThrottle::WillProcessResponse(bool* defer) {
         request_->response_headers()->raw_headers());
   }
 
+  UIChecksPerformedCallback callback =
+      base::Bind(&NavigationResourceThrottle::OnUIChecksPerformed,
+                 weak_ptr_factory_.GetWeakPtr());
+
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&WillProcessResponseOnUIThread, render_process_id,
+      base::Bind(&WillProcessResponseOnUIThread, callback, render_process_id,
                  render_frame_id, response_headers));
+  *defer = true;
 }
 
 const char* NavigationResourceThrottle::GetNameForLogging() const {