PlzNavigate: Move navigation-level mixed content checks to the browser.

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigation_handle_impl.h"
 
 #include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
 #include "content/browser/appcache/appcache_service_impl.h"
 #include "content/browser/browsing_data/clear_site_data_throttle.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/devtools/render_frame_devtools_agent_host.h"
 #include "content/browser/frame_host/ancestor_throttle.h"
 #include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/mixed_content_navigation_throttle.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/navigator_delegate.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_navigation_handle.h"
 #include "content/common/frame_messages.h"
 #include "content/common/resource_request_body_impl.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/navigation_ui_data.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/url_constants.h"
 #include "net/base/net_errors.h"
 #include "net/url_request/redirect_info.h"
-#include "third_party/WebKit/public/platform/WebMixedContentContextType.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
 namespace content {
 
 namespace {
 
 void UpdateThrottleCheckResult(
     NavigationThrottle::ThrottleCheckResult* to_update,
     NavigationThrottle::ThrottleCheckResult result) {
@@ skipping 737 matching lines @@
 
   // No code after running the callback, as it might have resulted in our
   // destruction.
 }
 
 void NavigationHandleImpl::RegisterNavigationThrottles() {
   // Register the navigation throttles. The ScopedVector returned by
   // GetNavigationThrottles is not assigned to throttles_ directly because it
   // would overwrite any throttle previously added with
   // RegisterThrottleForTesting.
+  if (IsBrowserSideNavigationEnabled()) {
+    throttles_.insert(throttles_.end(),
+                      new MixedContentNavigationThrottle(this));

[nasko, 2017/01/12 18:32:37]

Does the ordering of when we add this matter? Would DevTools need to know and be
able to log the navigation before it is potentially blocked by the mixed content
checker?

[carlosk, 2017/01/21 02:54:59]

I don't know the answer for this.

As this is a security check it seemed appropriate to move it up the chain but
that's just me guessing. I created a new never-to-be-committed CL [1] where I
moved this registration to be the last one and no trybots ever complained about
it.

I also noticed the established pattern here of concealing the creation decision
from each throttle within their classes by having a CreateThrottleForNavigation
method called from here. I updated my code to do the same.

Another thing I'm usure about is if I should register directly to |throttles_|
or through |throttles_to_register| as both cases exist. I chose the latter
because 2 out of 3 cases do that...? :O


[1] https://codereview.chromium.org/2630043002

[nasko, 2017/01/23 22:32:37]

Let's keep it first then, until we hear something breaking.
Cool, thanks! That's definitely nice.
 
Yeah, what you did is good. We should follow up and make them all consistent.
 

[carlosk, 2017/02/08 02:59:02]

OK, I'm keeping as is but just reaffirming: trybots didn't complain when I moved
it to being the last one.

I also added a TODO to make this code a bit cleaner.

[nasko, 2017/02/10 01:08:20]

Acknowledged.

+  }
   ScopedVector<NavigationThrottle> throttles_to_register =
       GetDelegate()->CreateThrottlesForNavigation(this);
   std::unique_ptr<NavigationThrottle> devtools_throttle =
       RenderFrameDevToolsAgentHost::CreateThrottleForNavigation(this);
   if (devtools_throttle)
     throttles_to_register.push_back(std::move(devtools_throttle));
 
   std::unique_ptr<NavigationThrottle> clear_site_data_throttle =
       ClearSiteDataThrottle::CreateThrottleForNavigation(this);
   if (clear_site_data_throttle)
     throttles_to_register.push_back(std::move(clear_site_data_throttle));
 
   std::unique_ptr<content::NavigationThrottle> ancestor_throttle =
       content::AncestorThrottle::MaybeCreateThrottleFor(this);
   if (ancestor_throttle)
     throttles_.push_back(std::move(ancestor_throttle));
 
   if (throttles_to_register.size() > 0) {
     throttles_.insert(throttles_.begin(), throttles_to_register.begin(),
                       throttles_to_register.end());
     throttles_to_register.weak_clear();
   }
 }
 
 }  // namespace content