PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects

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 "base/memory/ptr_util.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
@@ skipping 535 matching lines @@
     source_site_instance()->GetProcess()->FilterURL(false, &url);
     // FilterURL sets the URL to about:blank if the CSP checks prevent the
     // renderer from accessing it.
     if ((url == url::kAboutBlankURL) && (url != common_params_.url)) {
       navigation_handle_->set_net_error_code(net::ERR_ABORTED);
       frame_tree_node_->ResetNavigationRequest(false, true);
       return;
     }
   }
 
+  // Compute the SiteInstance to use for the redirect and pass its
+  // RenderProcessHost ID if it has a process.

[Charlie Reis, 2017/06/27 04:24:53]

nit: ID vs RenderProcessHost

[clamy, 2017/06/27 15:23:05]

Done.

+  scoped_refptr<SiteInstance> site_instance =
+      frame_tree_node_->render_manager()->GetSiteInstanceForNavigationRequest(
+          *this);

[Charlie Reis, 2017/06/27 04:24:53]

I'm a small bit concerned that it's possible for this call to have side effects
(e.g., creating a new BrowsingInstance for things like redirects to the CWS) on
redirects that might not be the final one.  For example, if A redirects to CWS
which redirects to accounts.google.com, then no new BrowsingInstance is needed,
but we'd create one along the way.

Is this ok?  Or should we think about using the idempotent
SiteInstanceDescriptor methods instead?  (I'm not sure if it's worth it-- maybe
we can get by with the extra BrowsingInstance or other side effects.)

[clamy, 2017/06/27 15:23:05]

It seems the tests are fine with the change. For the BrowsingInstance case, I'm
not sure it's an issue - there doesn't seem to be anything inside the
constructor that would be an issue if we were to create one and discard it
immediately later.

[Charlie Reis, 2017/06/27 18:46:34]

I remembered the more specific problem we had in the past, and why we needed the
idempotent SiteInstanceDescriptors.  I think it might apply here as well.

If site A redirects to the CWS, that requires a new BrowsingInstance.  We'll
create a new SiteInstance in a new BrowsingInstance here during the redirect,
and we'll store it in the NavigationRequest if it has a process (e.g., if we're
over the process limit).

Then when the commit happens, it looks like RFHM won't realize we've already
created a new SiteInstance and BrowsingInstance, and it will create a *second*
one unnecessarily.

Is there a way we can use the speculative SiteInstance from the redirect at
commit time if it matches?  Maybe that would fit in
GetSiteInstanceForNavigationRequest?

[clamy, 2017/06/28 14:08:31]

I don't think the case you mention can happen. In a brand new SiteInstance, we
only get a process when we call GetProcess. This applies whether we are over the
process limit or not: it's the call to GetProcess that triggers the reuse of a
RPH in the former case.

Here, if the function returned a new SiteInstance, it wouldn't have a process at
all - as in its RPH would be nullptr, even if we're above the process limit. In
that case we destroy it at the end of the function. I don't think having the
SiteInstance without an associated process is much worse than a SiteInstance
descriptor.

As for the passing the SiteInstance to RFHM we could do it at commit time. The
code of GetFrameHostForNavigation has a candidate site instance
(https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_...).
This is the SiteInstance of the speculative RFH if there is one. We could expand
it so that it considers the speculative SiteInstance as well. 

[Charlie Reis, 2017/06/28 17:05:22]

Good point-- such a SiteInstance wouldn't have a process yet (even if it's going
to reuse), so we wouldn't store it.
I still think it's less than ideal because it does have side effects (i.e.,
double creation of a BrowsingInstance and SiteInstance), but I think I agree
that it's not currently observable and we can probably get by for now.  I think
we should try to avoid it in the future (as noted below), because we already
have a SiteInstanceObserver class and it's possible that this could become
visible to other code in the future.
Yes, I think that's a good idea.  Let's do it now if it's easy, or leave a TODO
if it's subtle.  Thanks!

[clamy, 2017/06/30 15:12:59]

Since we should still consider the speculative RFH as well, we would need to
modify the code to expect several candidate instances. So I'm leaving it as a
TODO as that seems a bit out of scope for this CL.

+  RenderProcessHost* expected_process =
+      site_instance->HasProcess() ? site_instance->GetProcess() : nullptr;

[clamy, 2017/06/23 12:44:48]

This actually introduces a new issue - the SiteInstance returned here could be
that of another frame in the BrowsingInstance. If that frame goes away before we
commit the navigation, we will end up with a stale entry in our map of expected
sites in RenderProcessHostImpl. I'm thinking of two ways to address this.

1) Make the clean up function on RenderProcessHost deletion of the
SiteCountPerProcessTracker not debug-only. This would also solve the issues we
have with bookkeeping currently.

2) Keep a ref to the SiteInstance we got just above in the NavigationRequest.
This way we know it's not going away before commit, unless the navigation stops.
This would have the potential side effect of keeping the SiteInstance's process
alive longer, but then if we want to commit a navigation in it soon we might
want to do that rather than have to re-create a new one on commit. I think
keeping a ref on the SiteInstance makes sense if we want to move to a
speculative SiteInstance. But it might confuse tests.

I think we want to do 2 eventually, but 1 might be easier for this CL. Wdyt?

[nasko, 2017/06/23 20:23:23]

If 1 is really easier, I don't mind going with it for this CL. However, I do
like 2 a bit better conceptually, as it does the right thing and allows us to
save a process creation cost, which on mobile is costly. It does have the
downside that we are reusing a process that might have too much fragmentation,
which means its performance might not be as good, so it has some downsides to it
too.

[Charlie Reis, 2017/06/27 04:24:53]

Nice find!

I think 2 makes sense, assuming it doesn't cause new problems.  If we're
planning to put the new navigation into this SiteInstance, we might as well
avoid the churn of having its process exit and be recreated, even if there's
some fragmentation.  Presumably we'd replace the expected SiteInstance if an
additional redirect occurs?

Question: Is this possible already today without redirects?  Suppose we have
tabs showing A and B in the same BrowsingInstance, and B starts navigating to A.
 We'd mark that as a pending navigation to A's process in the bookkeeping, but
the first tab to A could close before that commits, theoretically taking the A
process down with it.  Is that a problem in today's code?

[clamy, 2017/06/27 15:23:05]

I'm trying option 2, and running the tests. If they are ok, I'll go with that,
otherwise I'll look at option 1.

I don't think the issue you describe is happening today, because when we decide
to mark the navigation as pending into SiteInstance A at the beginning of the
navigation, we also create a speculative RFH that keeps a ref to SiteInstance A.
So we shouldn't delete SiteInstance A while the speculative RFH is alive.

[Charlie Reis, 2017/06/27 18:46:34]

Great.  The tests look happy.
Good point.  But don't we clear the speculative RFH on redirects today?  Or do
we just leave it in place and let it get skipped at commit time if it doesn't
match?  (I guess the latter would have some advantages if that's how we behave,
since it would let us keep the process on an A->B->A redirect, however uncommon
that is.)

[clamy, 2017/06/28 14:08:32]

No we leave it in place until the navigation is ready to commit.

+
   // It's safe to use base::Unretained because this NavigationRequest owns the
   // NavigationHandle where the callback will be stored.
   bool is_external_protocol =
       !GetContentClient()->browser()->IsHandledURL(common_params_.url);
   navigation_handle_->WillRedirectRequest(
       common_params_.url, common_params_.method, common_params_.referrer.url,
       is_external_protocol, response->head.headers,
-      response->head.connection_info,
+      response->head.connection_info, expected_process,
       base::Bind(&NavigationRequest::OnRedirectChecksComplete,
                  base::Unretained(this)));
 }
 
 void NavigationRequest::OnResponseStarted(
     const scoped_refptr<ResourceResponse>& response,
     std::unique_ptr<StreamHandle> body,
     mojo::ScopedDataPipeConsumerHandle consumer_handle,
     const SSLStatus& ssl_status,
     std::unique_ptr<NavigationData> navigation_data,
@@ skipping 415 matching lines @@
           CSPDirective::FrameSrc, common_params_.url, is_redirect,
           common_params_.source_location.value_or(SourceLocation()),
           CSPContext::CHECK_ENFORCED_CSP)) {
     return CONTENT_SECURITY_POLICY_CHECK_PASSED;
   }
 
   return CONTENT_SECURITY_POLICY_CHECK_FAILED;
 }
 
 }  // namespace content