Merge CrossSiteResourceHandler and NavigationResourceThrottle

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..aeb141842e723955dc7de285cc64c5ee051c76e1 100644
--- a/content/browser/loader/navigation_resource_throttle.cc
+++ b/content/browser/loader/navigation_resource_throttle.cc
@@ -14,6 +14,8 @@
 #include "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/loader/navigation_resource_handler.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/loader/resource_loader.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_data.h"
@@ -32,6 +34,12 @@
 namespace content {
 
 namespace {
+
+// Used in unit tests to make UI checks succeed even if there is no
+// NavigationHandle and to transfer all navigations.
+bool g_ui_checks_always_succeed = false;
+bool g_force_transfer = false;
+
 typedef base::Callback<void(NavigationThrottle::ThrottleCheckResult)>
     UIChecksPerformedCallback;
 
@@ -54,19 +62,31 @@ void CheckWillStartRequestOnUIThread(
     bool has_user_gesture,
     ui::PageTransition transition,
     bool is_external_protocol,
-    RequestContextType request_context_type) {
+    RequestContextType request_context_type,
+    bool proceed_if_handle_not_found) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RenderFrameHostImpl* render_frame_host =
       RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
-  if (!render_frame_host) {
+  if (g_ui_checks_always_succeed) {
     SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
     return;
   }
 
+  if (!render_frame_host) {
+    if (proceed_if_handle_not_found)
+      SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+    else
+      SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL);
+    return;
+  }
+
   NavigationHandleImpl* navigation_handle =
       render_frame_host->navigation_handle();
   if (!navigation_handle) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+    if (proceed_if_handle_not_found)
+      SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+    else
+      SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL);
     return;
   }
 
@@ -88,15 +108,20 @@ void CheckWillRedirectRequestOnUIThread(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RenderFrameHostImpl* render_frame_host =
       RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
-  if (!render_frame_host) {
+  if (g_ui_checks_always_succeed) {
     SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
     return;
   }
 
+  if (!render_frame_host) {
+    SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL);
+    return;
+  }
+
   NavigationHandleImpl* navigation_handle =
       render_frame_host->navigation_handle();
   if (!navigation_handle) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+    SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL);
     return;
   }
 
@@ -114,19 +139,40 @@ void WillProcessResponseOnUIThread(
     int render_frame_host_id,
     scoped_refptr<net::HttpResponseHeaders> headers,
     const SSLStatus& ssl_status,
-    std::unique_ptr<NavigationData> navigation_data) {
+    const GlobalRequestID& request_id,
+    bool should_replace_current_entry,
+    bool is_download,
+    bool is_stream,
+    const base::Closure& transfer_callback,
+    std::unique_ptr<NavigationData> navigation_data,
+    bool proceed_if_handle_not_found) {

[Charlie Reis, 2016/09/21 16:47:08]

This needs some documentation somewhere.  I don't understand how this is a
problem specific to data URLs-- why do they not have a NavigationHandle?  It
seems like it might be possible for other cases that we don't currently have
test coverage for.  Can you clarify if it's actually limited to data URLs?

(For example, there's also
RendererPreferences::browser_handles_all_top_level_requests, which sounds like
it could have a similar effect to Android sending all data URLs?)

[clamy, 2016/09/22 16:27:23]

Actually this is limited to interstitials: it's because with interstitials the
NavigatorImpl is replaced by the interstitial page itself, so the code in
NavigatorImpl::OnDidStartProvisionalLoad that creates the handle is not
triggered. Now that I think about it some more, it's probably better to just
ensure that interstitials are always loaded as a data url (patchset 9) rather
than this fix. Wdyt?

[nasko, 2016/09/22 22:44:55]

I am not a fan of this approach either. Putting all data URLs in the same bucket
as interstitials is a recipe for problems.

[Charlie Reis, 2016/09/23 05:46:55]

Let's avoid using the parameters that are meant for LoadBaseURLForDataURL, which
is an Android Webview feature with unfortunate behavior.

I'm happy to understand the root of the problem now, though: interstitials that
don't have NavigatorImpl, and thus don't have NavigationHandles.  Any chance we
could make InterstitialPageNavigatorImpl create NavigationHandles to avoid this
bug entirely?

If not, I could be convinced that proceed_if_handle_not_found is ok, though I'd
like to keep the fix for interstitials closer to interstitial code if possible.

[clamy, 2016/09/26 12:20:32]

The fix for interstitials has been moved to
https://codereview.chromium.org/2364943002/.

   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RenderFrameHostImpl* render_frame_host =
       RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
-  if (!render_frame_host) {
+  if (g_force_transfer) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, transfer_callback);
+  }

[mmenke, 2016/09/21 17:10:24]

nit:  Remove braces/

[clamy, 2016/09/26 12:20:32]

This code was moved in https://codereview.chromium.org/2364943002/.

+
+  if (g_ui_checks_always_succeed) {
     SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
     return;
   }
 
+  if (!render_frame_host) {

[nasko, 2016/09/22 22:44:55]

I don't quite understand this. In the case of interstitials, why would we not
find the RFH that is rendering it? We navigate it from the browser process and
presumably because we don't have base/history URL associated with it, we send it
to the browser process to load on Android. Doesn't that resource request get
tagged with the right process id/frame id?
If we manage to get a RenderFrameHost (which I think we should), we can use
InterstitialPage::FromRenderFrameHost to check if it is rendering an
interstitial and let it proceed directly.

In general it also seems busted to allow Android code to know about interstitial
page loads. Can we give it a base URL such that we know it is interstitial vs
any other data URL?

[clamy, 2016/09/26 12:20:32]

The fix for interstitials has been moved to
https://codereview.chromium.org/2364943002/.

+    if (proceed_if_handle_not_found)
+      SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+    else
+      SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL);
+    return;
+  }
+
   NavigationHandleImpl* navigation_handle =
       render_frame_host->navigation_handle();
   if (!navigation_handle) {
-    SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+    if (proceed_if_handle_not_found)
+      SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED);
+    else
+      SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL);
     return;
   }
 
@@ -134,7 +180,8 @@ void WillProcessResponseOnUIThread(
     navigation_handle->set_navigation_data(std::move(navigation_data));
 
   navigation_handle->WillProcessResponse(
-      render_frame_host, headers, ssl_status,
+      render_frame_host, headers, ssl_status, request_id,
+      should_replace_current_entry, is_download, is_stream, transfer_callback,
       base::Bind(&SendCheckResultToIOThread, callback));
 }
 
@@ -147,6 +194,8 @@ NavigationResourceThrottle::NavigationResourceThrottle(
     : request_(request),
       resource_dispatcher_host_delegate_(resource_dispatcher_host_delegate),
       request_context_type_(request_context_type),
+      in_cross_site_transition_(false),
+      on_transfer_done_result_(NavigationThrottle::DEFER),
       weak_ptr_factory_(this) {}
 
 NavigationResourceThrottle::~NavigationResourceThrottle() {}
@@ -177,7 +226,8 @@ void NavigationResourceThrottle::WillStartRequest(bool* defer) {
                      request_->url(), Referrer(GURL(request_->referrer()),
                                                info->GetReferrerPolicy())),
                  info->HasUserGesture(), info->GetPageTransition(),
-                 is_external_protocol, request_context_type_));
+                 is_external_protocol, request_context_type_,
+                 request_->url().SchemeIs(url::kDataScheme)));
   *defer = true;
 }
 
@@ -225,7 +275,8 @@ void NavigationResourceThrottle::WillRedirectRequest(
 
 void NavigationResourceThrottle::WillProcessResponse(bool* defer) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request_);
+  const ResourceRequestInfoImpl* info =
+      ResourceRequestInfoImpl::ForRequest(request_);
   if (!info)
     return;
 
@@ -255,6 +306,9 @@ void NavigationResourceThrottle::WillProcessResponse(bool* defer) {
   UIChecksPerformedCallback callback =
       base::Bind(&NavigationResourceThrottle::OnUIChecksPerformed,
                  weak_ptr_factory_.GetWeakPtr());
+  base::Closure transfer_callback =
+      base::Bind(&NavigationResourceThrottle::InitiateTransfer,
+                 weak_ptr_factory_.GetWeakPtr());
 
   SSLStatus ssl_status;
   if (request_->ssl_info().cert.get()) {
@@ -266,7 +320,11 @@ void NavigationResourceThrottle::WillProcessResponse(bool* defer) {
       BrowserThread::UI, FROM_HERE,
       base::Bind(&WillProcessResponseOnUIThread, callback, render_process_id,
                  render_frame_id, response_headers, ssl_status,
-                 base::Passed(&cloned_data)));
+                 info->GetGlobalRequestID(),
+                 info->should_replace_current_entry(), info->IsDownload(),
+                 info->is_stream(), transfer_callback,
+                 base::Passed(&cloned_data),
+                 request_->url().SchemeIs(url::kDataScheme)));
   *defer = true;
 }
 
@@ -274,9 +332,25 @@ const char* NavigationResourceThrottle::GetNameForLogging() const {
   return "NavigationResourceThrottle";
 }
 
+void NavigationResourceThrottle::SetUIChecksAlwaysSucceedForTesting(
+    bool ui_checks_always_succeed) {
+  g_ui_checks_always_succeed = ui_checks_always_succeed;
+}
+
+void NavigationResourceThrottle::SetForceTransferForTesting(
+    bool force_transfer) {
+  g_force_transfer = force_transfer;
+}
+
 void NavigationResourceThrottle::OnUIChecksPerformed(
     NavigationThrottle::ThrottleCheckResult result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_NE(NavigationThrottle::DEFER, result);
+  if (in_cross_site_transition_) {
+    on_transfer_done_result_ = result;
+    return;
+  }
+
   if (result == NavigationThrottle::CANCEL_AND_IGNORE) {
     controller()->CancelAndIgnore();
   } else if (result == NavigationThrottle::CANCEL) {
@@ -288,4 +362,28 @@ void NavigationResourceThrottle::OnUIChecksPerformed(
   }
 }
 
+void NavigationResourceThrottle::InitiateTransfer() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  in_cross_site_transition_ = true;
+  ResourceRequestInfoImpl* info =
+      ResourceRequestInfoImpl::ForRequest(request_);
+  ResourceDispatcherHostImpl::Get()->MarkAsTransferredNavigation(
+      info->GetGlobalRequestID(),
+      base::Bind(&NavigationResourceThrottle::OnTransferComplete,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void NavigationResourceThrottle::OnTransferComplete() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(in_cross_site_transition_);
+  in_cross_site_transition_ = false;
+
+  // If the results of the checks on the UI thread are known, unblock the
+  // navigation. Otherwise, wait until the callback has executed.
+  if (on_transfer_done_result_ != NavigationThrottle::DEFER) {
+    OnUIChecksPerformed(on_transfer_done_result_);
+    on_transfer_done_result_ = NavigationThrottle::DEFER;
+  }
+}
+
 }  // namespace content