Add a NavigationThrottle to the public content/ interface

content/browser/frame_host/navigation_request.cc

Index: content/browser/frame_host/navigation_request.cc
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index a99074f2ce3aeb56440b44cc0e9530f0f0c1db39..c0e94fcf0bfaaa172b18cf7b02793aa71f8b413f 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -14,6 +14,7 @@
 #include "content/browser/site_instance_impl.h"
 #include "content/common/resource_request_body.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/stream_handle.h"
 #include "content/public/common/content_client.h"
 #include "net/base/load_flags.h"
@@ -49,6 +50,17 @@ int LoadFlagFromNavigationType(FrameMsg_Navigate_Type::Value navigation_type) {
   return load_flags;
 }
 
+// Filters the URL for the navigation based.
+GURL FilterURL(const GURL& url, FrameTreeNode* frame_tree_node) {
+  RenderFrameHostImpl* render_frame_host =
+      frame_tree_node->render_manager()->speculative_frame_host()
+          ? frame_tree_node->render_manager()->speculative_frame_host()
+          : frame_tree_node->render_manager()->current_frame_host();

[carlosk, 2015/08/28 16:40:24]

This next-RFH logic should live inside RFHM. We've been in the verge of adding
it there for some time already and this usage looks like the perfect use case.

[nasko, 2015/08/31 23:25:15]

This code seems a bit fragile. What happens if there is a speculative RFH and
the url passed in doesn't match the SiteInstance for that one? The current code
doesn't handle this case, does it?

[clamy, 2015/09/03 15:30:51]

This would happen in the case of redirects. In that case we may not even have a
renderer with the right site instance. I'm not sure what to do in that case:
fall back to the current RFH? Note that the ResourceThrottles try to filter the
url based on the RFH that initiated the navigation (with the redirect it may not
be appropriate).

[nasko, 2015/09/04 23:36:49]

I'm not sure we need to call FilterURL on redirects though. We don't do it today
and it is meant to ensure the renderer doesn't request something invalid. If the
server redirects us to some bogus place, so be it. Maybe add a TODO on what the
problem is and let's leave it to be resolved later on.

+  GURL filtered_url = url;
+  render_frame_host->GetProcess()->FilterURL(false, &filtered_url);
+  return filtered_url;
+}
+
 }  // namespace
 
 // static
@@ -179,6 +191,20 @@ bool NavigationRequest::BeginNavigation() {
   state_ = STARTED;
 
   if (ShouldMakeNetworkRequestForURL(common_params_.url)) {
+    // TODO(clamy): pass the real value for |is_external_protocol|.

[davidben, 2015/09/01 21:55:18]

Hrm. It seems to manage that, we'd need to either fire this until the network
request has hit the IO thread (and so it becomes UI -> IO -> UI -> IO before
starting the request), or we have a way to compute it on the UI thread, which
means the OnRequestRedirect logic changes.

I haven't looked closely, but this is only used by the Android Intents code,
right? It seems some codepaths actually compute it from the URL in Java with
UrlUtilities.isAcceptedScheme. Perhaps we should just switch it all to that and
remove it from this API?

(A little ugly to have the duplicated code like that, but maybe it's fine in
this context? That code is at least top-level. I don't know what context it's
used in.)

[clamy, 2015/09/03 15:30:51]

So it seems it is only used on Android to log to the DevTools console that a
navigation failed if it is not handled by an intent. This is some internal code
that was upstreamed recently, so I'm thinking maybe we could get rid of it
entirely. In any case, I don't want to do a UI -> IO -> UI -> IO hop unless
really forced to do so.

+    NavigationThrottle::ThrottleCheckResult result =
+        navigation_handle_->WillStartRequest(
+            begin_params_.method == "POST",
+            Referrer::SanitizeForRequest(common_params_.url,
+                                         common_params_.referrer),
+            begin_params_.has_user_gesture, common_params_.transition, false);
+
+    // Abort the request if needed. This will destroy the NavigationRequest.
+    if (result == NavigationThrottle::CANCEL_AND_IGNORE) {
+      frame_tree_node_->ResetNavigationRequest(false);
+      return false;
+    }
+
     loader_ = NavigationURLLoader::Create(
         frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
         frame_tree_node_->frame_tree_node_id(), info_.Pass(), this);
@@ -198,7 +224,8 @@ bool NavigationRequest::BeginNavigation() {
 
 void NavigationRequest::CreateNavigationHandle(NavigatorDelegate* delegate) {
   navigation_handle_ = NavigationHandleImpl::Create(
-      common_params_.url, frame_tree_node_->IsMainFrame(), delegate);
+      common_params_.url, FilterURL(common_params_.url, frame_tree_node_),
+      frame_tree_node_->IsMainFrame(), delegate);
 }
 
 void NavigationRequest::TransferNavigationHandleOwnership(
@@ -208,13 +235,25 @@ void NavigationRequest::TransferNavigationHandleOwnership(
 
 void NavigationRequest::OnRequestRedirected(
     const net::RedirectInfo& redirect_info,
-    const scoped_refptr<ResourceResponse>& response) {
-  // TODO(davidben): Track other changes from redirects. These are important
-  // for, e.g., reloads.
+    const scoped_refptr<ResourceResponse>& response,
+    bool is_external_protocol) {
   common_params_.url = redirect_info.new_url;
+  begin_params_.method = redirect_info.new_method;
+  common_params_.referrer.url = GURL(redirect_info.new_referrer);
+
+  // TODO(clamy): Have CSP + security upgrade checks here.
+  NavigationThrottle::ThrottleCheckResult result =
+      navigation_handle_->WillRedirectRequest(
+          common_params_.url, FilterURL(common_params_.url, frame_tree_node_),

[nasko, 2015/08/31 23:25:15]

FilterURL should be a called separately and possibly kill the renderer if it
fails. Even if we don't do the kill now, the code will be easier to refactor
later on.

[clamy, 2015/09/03 15:30:51]

Done. I have added a TODO for the kill (note that the current code does not kill
the renderer if it fails).

+          begin_params_.method == "POST", common_params_.referrer.url,
+          is_external_protocol);
+
+  // Abort the request if needed. This will destroy the NavigationRequest.
+  if (result == NavigationThrottle::CANCEL_AND_IGNORE) {
+    frame_tree_node_->ResetNavigationRequest(false);
+    return;
+  }
 
-  // TODO(davidben): This where prerender and navigation_interceptor should be
-  // integrated. For now, just always follow all redirects.
   loader_->FollowRedirect();
 
   navigation_handle_->DidRedirectNavigation(redirect_info.new_url);