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

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 ba3a117e4417e3e3a67248fc1d29f30a8d6210f5..980b2e5029581c81e04d8b8b05f5c11be052c3d9 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -298,8 +298,11 @@ void NavigationRequest::BeginNavigation() {
 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,