Move blocking of top-level navigations to nested URLs with extension origins from non-extension pro…

content/browser/frame_host/navigation_request.cc

 // Copyright 2014 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_request.h"
 
 #include <utility>
 
 #include "content/browser/devtools/render_frame_devtools_agent_host.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ skipping 280 matching lines @@
 
   // Inform the NavigationHandle that the navigation will commit.
   navigation_handle_->ReadyToCommitNavigation(render_frame_host);
 
   CommitNavigation();
 }
 
 void NavigationRequest::CreateNavigationHandle(int pending_nav_entry_id) {
   // TODO(nasko): Update the NavigationHandle creation to ensure that the
   // proper values are specified for is_synchronous and is_srcdoc.
+  RenderFrameHostImpl* render_frame_host =
+      frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);

[nasko, 2016/10/12 17:00:35]

This is not going to give you the correct RFH. It will pick the RFH in which we
must commit this the navigation, not the one that started it.

Alex and I are looking through code to see what is the correct thing to use here
and will get back to you, as there isn't a convenient way right now that will do
the right thing.

[alexmos, 2016/10/12 17:18:16]

To double-check what happens (since I was confused by some of the PlzNav logic),
I patched in the CL and ran the
ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked test with
PlzNav, and I saw failures that indicated that the bad navigations were allowed.
 So yes, this picks the wrong SiteInstance.

By the way, to get around the test crash due to DCHECK(!frame_widget_) (which
you'd get if the bad navigations are blocked),  you can try patching in my draft
fix at https://codereview.chromium.org/2410153005/.  Actually, the Blocked test
passes without any further modifications with just that fix.

[jam, 2016/10/12 22:10:45]

thanks, I added a test based on your description and got the site instance from
the frame tree node's current frame host. It's not clear to me if this is
sufficient, or if sometimes I need to use the pending/speculative frame host if
the former doesn't exist because it's a new frame?

[alexmos, 2016/10/12 23:13:28]

Thanks, the test looks nice and makes it very clear what this returns.  We like
the new approach, it should just work for all the cases we're concerned about. 
New frames should also be fine, as they always start out in the SiteInstance of
their creator, so the current_frame_host() should give the right SiteInstance in
that case.

   navigation_handle_ = NavigationHandleImpl::Create(
-      common_params_.url, frame_tree_node_, !browser_initiated_,
+      common_params_.url, render_frame_host->GetSiteInstance()->GetSiteURL(),
+      frame_tree_node_, !browser_initiated_,
       false,  // is_synchronous
       false,  // is_srcdoc
       common_params_.navigation_start, pending_nav_entry_id,
       false);  // started_in_context_menu
 }
 
 void NavigationRequest::TransferNavigationHandleOwnership(
     RenderFrameHostImpl* render_frame_host) {
   render_frame_host->SetNavigationHandle(std::move(navigation_handle_));
 }
@@ skipping 246 matching lines @@
   DCHECK_EQ(request_params_.has_user_gesture, begin_params_.has_user_gesture);
 
   render_frame_host->CommitNavigation(response_.get(), std::move(body_),
                                       common_params_, request_params_,
                                       is_view_source_);
 
   frame_tree_node_->ResetNavigationRequest(true);
 }
 
 }  // namespace content