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

content/browser/loader/cross_site_resource_handler.cc

 // Copyright (c) 2012 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/loader/cross_site_resource_handler.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 66 matching lines @@
         params.global_request_id, cross_site_transferring_request.Pass(),
         params.transfer_url_chain, params.referrer,
         params.page_transition, params.should_replace_current_entry);
   } else if (leak_requests_for_testing_ && cross_site_transferring_request) {
     // Some unit tests expect requests to be leaked in this case, so they can
     // pass them along manually.
     cross_site_transferring_request->ReleaseRequest();
   }
 }
 
+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(
     scoped_ptr<ResourceHandler> next_handler,
     net::URLRequest* request)
     : LayeredResourceHandler(request, next_handler.Pass()),
       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() {
   // Cleanup back-pointer stored on the request info.
   GetRequestInfo()->set_cross_site_handler(NULL);
 }
 
 bool CrossSiteResourceHandler::OnRequestRedirected(
     int request_id,
     const GURL& new_url,
@@ skipping 20 matching lines @@
 
   // We will need to swap processes if either (1) a redirect that requires a
   // transfer occurred before we got here, or (2) a pending cross-site request
   // was already in progress.  Note that a swap may no longer be needed if we
   // transferred back into the original process due to a redirect.
   bool should_transfer =
       GetContentClient()->browser()->ShouldSwapProcessesForRedirect(
           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());
+    // 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.

 
-    // 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;
-    }
+    // 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 ||
       CrossSiteRequestManager::GetInstance()->
           HasPendingCrossSiteRequest(info->GetChildID(), info->GetRouteID());
 
   // If this is a download, just pass the response through without doing a
   // cross-site check.  The renderer will see it is a download and abort the
   // request.
   //
@@ skipping 18 matching lines @@
   // pause to let the UI thread run the unload handler of the previous page
   // and set up a transfer if needed.
   StartCrossSiteTransition(request_id, response, should_transfer);
 
   // Defer loading until after the onunload event handler has run.
   *defer = true;
   OnDidDefer();
   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) {
   CHECK(!in_cross_site_transition_);
   return next_handler_->OnReadCompleted(request_id, bytes_read, defer);
 }
 
 void CrossSiteResourceHandler::OnResponseCompleted(
     int request_id,
     const net::URLRequestStatus& status,
@@ skipping 28 matching lines @@
   // Defer to tell RDH not to notify the world or clean up the pending request.
   // We will do so in ResumeResponse.
   *defer = true;
   OnDidDefer();
 }
 
 // We can now send the response to the new renderer, which will cause
 // 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();
 
   if (has_started_response_) {
     // Send OnResponseStarted to the new renderer.
     DCHECK(response_);
     bool defer = false;
     if (!next_handler_->OnResponseStarted(info->GetRequestID(), response_.get(),
                                           &defer)) {
       controller()->Cancel();
@@ skipping 81 matching lines @@
     controller()->Resume();
   }
 }
 
 void CrossSiteResourceHandler::OnDidDefer() {
   did_defer_ = true;
   request()->LogBlockedBy("CrossSiteResourceHandler");
 }
 
 }  // namespace content