Create NavigationHandles for interstitials if needed

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 a0701876314316a010f00dc4d130aab58a4515b9..e4ae064254eed116c48bb4279bf97494e3e458f9 100644
--- a/content/browser/loader/navigation_resource_throttle.cc
+++ b/content/browser/loader/navigation_resource_throttle.cc
@@ -12,6 +12,7 @@
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "content/browser/frame_host/navigation_handle_impl.h"
+#include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/loader/navigation_resource_handler.h"
 #include "content/browser/loader/resource_request_info_impl.h"
@@ -32,6 +33,11 @@
 namespace content {
 
 namespace {
+
+// Used in unit tests to make UI checks succeed even if there is no

[nasko, 2016/09/23 17:41:35]

nit: UI thread

[clamy, 2016/09/26 11:37:27]

Done.

+// NavigationHandle.
+bool g_ui_checks_always_succeed = false;
+
 typedef base::Callback<void(NavigationThrottle::ThrottleCheckResult)>
     UIChecksPerformedCallback;
 
@@ -43,6 +49,43 @@ void SendCheckResultToIOThread(UIChecksPerformedCallback callback,
                           base::Bind(callback, result));
 }
 
+// Returns the NavigationHandle to use for a navigation in the frame specified
+// by

[nasko, 2016/09/23 17:41:35]

nit: Combine with next line.

[clamy, 2016/09/26 11:37:27]

Done.

+// |render_process_id| and |render_frame_host_id|. If not found, |callback| will
+// be invoked to cancel the request.
+// Note: in unit test |callback| may be invoked with a value of proceed if no
+// handle is found. This happens when
+// NavigationResourceThrottle::SetUIChecksAlwaysSucceedForTesting is called with
+// a vlue of true.

[nasko, 2016/09/23 17:41:35]

nit: value

[clamy, 2016/09/26 11:37:27]

Done.

+NavigationHandleImpl* FindNavigationHandle(
+    int render_process_id,
+    int render_frame_host_id,
+    const UIChecksPerformedCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (g_ui_checks_always_succeed) {
+    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+    return nullptr;
+  }
+
+  RenderFrameHostImpl* render_frame_host =
+      RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
+  if (!render_frame_host) {

[nasko, 2016/09/23 17:43:14]

In what case are we going to reach this? DCHECK maybe?

[clamy, 2016/09/26 11:37:27]

I think it could be possible to reach it in race conditions where we delete a
RFH that has an ongoing navigation. One case I'm thinking about is that it's
possible to delete the WebContents directly on Android without going through the
proper close sequence. This would also delete RFHs before they have time to
cancel their ongoing navigations.

+    SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL);
+    return nullptr;
+  }
+
+  NavigationHandleImpl* navigation_handle =
+      render_frame_host->frame_tree_node()
+          ->navigator()
+          ->GetNavigationHandleForFrameHost(render_frame_host);
+  if (!navigation_handle) {
+    SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL);
+    return nullptr;
+  }
+  return navigation_handle;
+}
+
 void CheckWillStartRequestOnUIThread(
     UIChecksPerformedCallback callback,
     int render_process_id,
@@ -56,19 +99,10 @@ void CheckWillStartRequestOnUIThread(
     bool is_external_protocol,
     RequestContextType request_context_type) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  RenderFrameHostImpl* render_frame_host =
-      RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
-  if (!render_frame_host) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
-    return;
-  }
-
   NavigationHandleImpl* navigation_handle =
-      render_frame_host->navigation_handle();
-  if (!navigation_handle) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+      FindNavigationHandle(render_process_id, render_frame_host_id, callback);
+  if (!navigation_handle)
     return;
-  }
 
   navigation_handle->WillStartRequest(
       method, resource_request_body, sanitized_referrer, has_user_gesture,
@@ -86,19 +120,10 @@ void CheckWillRedirectRequestOnUIThread(
     bool new_is_external_protocol,
     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) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
-    return;
-  }
-
   NavigationHandleImpl* navigation_handle =
-      render_frame_host->navigation_handle();
-  if (!navigation_handle) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+      FindNavigationHandle(render_process_id, render_frame_host_id, callback);
+  if (!navigation_handle)
     return;
-  }
 
   GURL new_validated_url(new_url);
   RenderProcessHost::FromID(render_process_id)
@@ -116,23 +141,17 @@ void WillProcessResponseOnUIThread(
     const SSLStatus& ssl_status,
     std::unique_ptr<NavigationData> navigation_data) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  RenderFrameHostImpl* render_frame_host =
-      RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
-  if (!render_frame_host) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
-    return;
-  }
-
   NavigationHandleImpl* navigation_handle =
-      render_frame_host->navigation_handle();
-  if (!navigation_handle) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+      FindNavigationHandle(render_process_id, render_frame_host_id, callback);
+  if (!navigation_handle)
     return;
-  }
 
   if (navigation_data)
     navigation_handle->set_navigation_data(std::move(navigation_data));
 
+  RenderFrameHostImpl* render_frame_host =
+      RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
+  DCHECK(render_frame_host);
   navigation_handle->WillProcessResponse(
       render_frame_host, headers, ssl_status,
       base::Bind(&SendCheckResultToIOThread, callback));
@@ -274,6 +293,11 @@ const char* NavigationResourceThrottle::GetNameForLogging() const {
   return "NavigationResourceThrottle";
 }
 
+void NavigationResourceThrottle::SetUIChecksAlwaysSucceedForTesting(

[nasko, 2016/09/23 17:41:35]

nit: Why the CamelCase when this is a simple setter?

[clamy, 2016/09/26 11:37:27]

Done.

+    bool ui_checks_always_succeed) {
+  g_ui_checks_always_succeed = ui_checks_always_succeed;
+}
+
 void NavigationResourceThrottle::OnUIChecksPerformed(
     NavigationThrottle::ThrottleCheckResult result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);