Perform navigation policy check on UI thread for --site-per-process.

content/browser/loader/cross_site_resource_handler.cc

Index: content/browser/loader/cross_site_resource_handler.cc
diff --git a/content/browser/loader/cross_site_resource_handler.cc b/content/browser/loader/cross_site_resource_handler.cc
index 8e563972111dd87236eea15ad0814ff0c6d5ddb5..59ecaf0c778aefe5660986a185e6cc7c96b3ac20 100644
--- a/content/browser/loader/cross_site_resource_handler.cc
+++ b/content/browser/loader/cross_site_resource_handler.cc
@@ -84,6 +84,20 @@ void OnCrossSiteResponseHelper(const CrossSiteResponseParams& params) {
   }
 }
 
+bool CheckNavigationPolicyOnUI(GURL url, int process_id, int render_frame_id) {
+  RenderFrameHostImpl* rfh =
+      RenderFrameHostImpl::FromID(process_id, render_frame_id);

[Charlie Reis, 2014/04/24 20:37:50]

We should check for null here, in case the tab was closed during the hop from IO
to UI thread.

[nasko, 2014/04/24 22:31:12]

Do'h, I had a check here.

+
+  // TODO(nasko): This check is very simplistic and is used temporarily only
+  // for --site-per-process. It should be updated to match the check performed
+  // by RenderFrameHostManager::UpdateRendererStateForNavigate.
+  bool transfer = !SiteInstance::IsSameWebSite(

[Charlie Reis, 2014/04/24 20:37:50]

Why not just return from here?

[nasko, 2014/04/24 22:31:12]

Done.

+      rfh->GetSiteInstance()->GetBrowserContext(),
+      rfh->GetSiteInstance()->GetSiteURL(), url);
+
+  return transfer;
+}
+
 }  // namespace
 
 CrossSiteResourceHandler::CrossSiteResourceHandler(
@@ -93,7 +107,8 @@ CrossSiteResourceHandler::CrossSiteResourceHandler(
       has_started_response_(false),
       in_cross_site_transition_(false),
       completed_during_transition_(false),
-      did_defer_(false) {
+      did_defer_(false),
+      weak_ptr_factory_(this) {
 }
 
 CrossSiteResourceHandler::~CrossSiteResourceHandler() {
@@ -134,24 +149,35 @@ bool CrossSiteResourceHandler::OnResponseStarted(
           info->GetContext(), request()->original_url(), request()->url());
 
   // When the --site-per-process flag is passed, we transfer processes for
-  // cross-site subframe navigations.
-  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) {
+  // cross-site navigations. This is skipped if a transfer is already required
+  // or for WebUI processes for now, since pages like the NTP host cross-site
+  // WebUI iframes but don't have referrers.

[Charlie Reis, 2014/04/24 20:37:50]

We can drop the "but don't have referrers" part now.  Maybe say "host multiple
cross-site WebUI iframes" instead, since that's the part we can't support.

[nasko, 2014/04/24 22:31:12]

Done.

+  if (!should_transfer &&
+      CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess) &&
+      !ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
+          info->GetChildID())) {
     GURL referrer(request()->referrer());

[Charlie Reis, 2014/04/24 20:37:50]

referrer looks unused now.

[nasko, 2014/04/24 22:31:12]

Done.

-    // We skip this for WebUI processes for now, since pages like the NTP host
-    // cross-site WebUI iframes but don't have referrers.
-    bool is_webui_process = ChildProcessSecurityPolicyImpl::GetInstance()->
-        HasWebUIBindings(info->GetChildID());
-
-    // TODO(creis): This shouldn't rely on the referrer to determine the parent
-    // frame's URL.  This also doesn't work for hosted apps, due to passing NULL
-    // to IsSameWebSite.  It should be possible to always send the navigation to
-    // the UI thread to make a policy decision, which could let us eliminate the
-    // renderer-side check in RenderViewImpl::decidePolicyForNavigation as well.
-    if (info->GetResourceType() == ResourceType::SUB_FRAME &&
-        !is_webui_process &&
-        !SiteInstance::IsSameWebSite(NULL, request()->url(), referrer)) {
-      should_transfer = true;
-    }
+    // Store the response_ object internally, since it is needed when the flow

[Charlie Reis, 2014/04/24 20:37:50]

Can we move this whole block out to a helper function, like
DeferForNavigationPolicyCheck?  It should have a TODO to merge it into the same
UI thread hop as StartCrossSiteTransition, rather than having two hops.

[nasko, 2014/04/24 22:31:12]

Done.

+    // comes back to the IO thread.
+    response_ = response;

[Charlie Reis, 2014/04/24 20:37:50]

Hmm, this is subtle.  It means this value is set in the non-transfer case,
unlike before.  I think it's only right because StartCrossSiteTransition and
ResumeResponse are the only ones that look at it, and we need ResumeResponse in
the non-transfer case here thanks to the defer below.  Maybe it's worth
mentioning in the comment that this is needed because we defer the request
whether we choose to transfer it or not.

[nasko, 2014/04/24 22:31:12]

Done.

+
+    // Always send the navigation to the UI thread to make a policy decision. It

[Charlie Reis, 2014/04/24 20:37:50]

nit: send the navigation to the -> defer to the

[nasko, 2014/04/24 22:31:12]

Done.

+    // will send the result back to the IO thread to either resume or transfer
+    // it to a new renderer.
+    BrowserThread::PostTaskAndReplyWithResult(

[Charlie Reis, 2014/04/24 20:37:50]

This function and the use of WeakPtr here is still a bit magic to me.  Can you
have Albert take a look to make sure it's correct?

[nasko, 2014/04/24 22:31:12]

I'll add dcheng@ to review.

+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&CheckNavigationPolicyOnUI,
+                   request()->url(),
+                   info->GetChildID(),
+                   info->GetRenderFrameID()),
+        base::Bind(&CrossSiteResourceHandler::ResumeOrTransfer,
+                   weak_ptr_factory_.GetWeakPtr()));

[Charlie Reis, 2014/04/24 20:37:50]

I'm sad that we need to add a weak pointer factory here, but I agree that it's
possible for this to get deleted in the mean time.  We've gotten by without it
before because ResumeDeferredNavigation has a null ResourceLoader check in
ResourceDispatcherHostImpl.

Can this just be a temporary thing (with a TODO to remove the factory) until we
combine the two UI hops into one?

[nasko, 2014/04/24 22:31:12]

I could do away without the factory, but then we risk crashes. I'll put a TODO.

+    // Defer loading until it is known whether the navigation will transfer
+    // to a new process or continue in the existing one.
+    *defer = true;
+    OnDidDefer();
+    return true;
   }
 
   bool swap_needed = should_transfer ||
@@ -190,6 +216,15 @@ bool CrossSiteResourceHandler::OnResponseStarted(
   return true;
 }
 
+void CrossSiteResourceHandler::ResumeOrTransfer(bool is_transfer) {
+  if (is_transfer) {
+    ResourceRequestInfoImpl* info = GetRequestInfo();
+    StartCrossSiteTransition(info->GetRequestID(), response_, is_transfer);
+  } else {
+    ResumeResponse();
+  }
+}
+
 bool CrossSiteResourceHandler::OnReadCompleted(int request_id,
                                                int bytes_read,
                                                bool* defer) {
@@ -238,7 +273,6 @@ void CrossSiteResourceHandler::OnResponseCompleted(
 // WebContentsImpl to swap in the new renderer and destroy the old one.
 void CrossSiteResourceHandler::ResumeResponse() {
   DCHECK(request());
-  DCHECK(in_cross_site_transition_);
   in_cross_site_transition_ = false;
   ResourceRequestInfoImpl* info = GetRequestInfo();