Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954623003:
PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects (Closed)
|
Created:
3 years, 6 months ago by clamy Modified:
3 years, 5 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, rdsmith+watch_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects
This CL is a follow up of the implementation of
ProcessReusePolicy::REUSE_COMMITTED_OR_PENDING_SITE
(https://codereview.chromium.org/2857213005/). It adds cross-site
redirects that should be committed by an existing SiteInstance to the
list of expected sites tracked per RenderProcessHost in
RenderProcessHostImpl.
BUG=705318
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2954623003
Cr-Commit-Position: refs/heads/master@{#483726}
Committed: https://chromium.googlesource.com/chromium/src/+/0304a310bb32a007c09041384cd58f5a38d18e47
Patch Set 1 #
Total comments: 22
Patch Set 2 : Rebase #Patch Set 3 : Addressed comments #
Total comments: 12
Patch Set 4 : Addressed comments #
Total comments: 4
Patch Set 5 : Addressed Charlie's comments #Messages
Total messages: 30 (19 generated)
Description was changed from ========== PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects This CL is a follow up of the implementation of ProcessReusePolicy::REUSE_COMMITTED_OR_PENDING_SITE (https://codereview.chromium.org/2857213005/). It adds cross-site redirects that should be committed by an existing SiteInstance to the list of expected sites tracked per RenderProcessHost in RenderProcessHostImpl. BUG=705318 ========== to ========== PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects This CL is a follow up of the implementation of ProcessReusePolicy::REUSE_COMMITTED_OR_PENDING_SITE (https://codereview.chromium.org/2857213005/). It adds cross-site redirects that should be committed by an existing SiteInstance to the list of expected sites tracked per RenderProcessHost in RenderProcessHostImpl. BUG=705318 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects This CL is a follow up of the implementation of ProcessReusePolicy::REUSE_COMMITTED_OR_PENDING_SITE (https://codereview.chromium.org/2857213005/). It adds cross-site redirects that should be committed by an existing SiteInstance to the list of expected sites tracked per RenderProcessHost in RenderProcessHostImpl. BUG=705318 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects This CL is a follow up of the implementation of ProcessReusePolicy::REUSE_COMMITTED_OR_PENDING_SITE (https://codereview.chromium.org/2857213005/). It adds cross-site redirects that should be committed by an existing SiteInstance to the list of expected sites tracked per RenderProcessHost in RenderProcessHostImpl. BUG=705318 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
@creis, nasko: PTAL. This is a first version of a CL for processes, SW and redirects. I'm having an issue with an edge case described below, on which I'd like your opinion. Thanks! https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:562: site_instance->HasProcess() ? site_instance->GetProcess() : nullptr; 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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I still need to wrap my mind around the UpdateSiteURL pieces, but I wanted to respond to the question you had. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:562: site_instance->HasProcess() ? site_instance->GetProcess() : nullptr; On 2017/06/23 12:44:48, clamy wrote: > 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? 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.
Nice. I didn't get a chance to look closely at render_frame_host_manager.cc or the test yet, but I wanted to send a few comments and questions without delaying. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:287: RenderProcessHost* new_expected_render_process_host, Maybe new_expected_process or post_redirect_process (here and below)? We should also include a comment about this above, and whether it's null in some cases. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:437: // be ChildProcessHost::kInvalidUniqueID. This comment is talking about IDs, but we pass a RenderProcessHost* below. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:557: // RenderProcessHost ID if it has a process. nit: ID vs RenderProcessHost https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:560: *this); 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.) https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:562: site_instance->HasProcess() ? site_instance->GetProcess() : nullptr; On 2017/06/23 20:23:23, nasko (slow) wrote: > On 2017/06/23 12:44:48, clamy wrote: > > 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? > > 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. 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? https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.h:510: // Note: the SiteInstance returned by this function may not have an initalized nit: initialized https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.h:513: // to a process being created for the navigation. As noted, we might have the option of using SiteInstanceDescriptors here to avoid side effects, but I'm not sure if that will be sufficient for our needs (or necessary).
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:287: RenderProcessHost* new_expected_render_process_host, On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > Maybe new_expected_process or post_redirect_process (here and below)? > > We should also include a comment about this above, and whether it's null in some > cases. Done. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:437: // be ChildProcessHost::kInvalidUniqueID. On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > This comment is talking about IDs, but we pass a RenderProcessHost* below. Done. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:557: // RenderProcessHost ID if it has a process. On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > nit: ID vs RenderProcessHost Done. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:560: *this); On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > 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 http://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.) 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. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:562: site_instance->HasProcess() ? site_instance->GetProcess() : nullptr; On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > On 2017/06/23 20:23:23, nasko (slow) wrote: > > On 2017/06/23 12:44:48, clamy wrote: > > > 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? > > > > 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. > > 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? 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. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.h:510: // Note: the SiteInstance returned by this function may not have an initalized On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > nit: initialized I forgot to address this in the latest patchset. Will do in the next one. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.h:513: // to a process being created for the navigation. On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > As noted, we might have the option of using SiteInstanceDescriptors here to > avoid side effects, but I'm not sure if that will be sufficient for our needs > (or necessary). I think we want to return the actual SiteInstance if it exists at least. We need to access its process. If creating a new one is an issue, we could also have a parameter to not create a SiteInstance if there is none we can use.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:566: site_instance->HasProcess() ? site_instance : nullptr; Note: I'm not storing the SiteInstance if its process is not initialized, because I'm worried that someone could carelessly call GetProcess on it and trigger an unwanted process initialization. Since we don't need to keep its process alive (the motivation for storing it), I'd rather we don;t store it in that case.
Thanks! One more concern about double creation of BrowsingInstances, and a few additional questions. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:560: *this); On 2017/06/27 15:23:05, clamy wrote: > On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > > 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 http://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.) > > 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. 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? https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:562: site_instance->HasProcess() ? site_instance->GetProcess() : nullptr; On 2017/06/27 15:23:05, clamy wrote: > On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > > On 2017/06/23 20:23:23, nasko (slow) wrote: > > > On 2017/06/23 12:44:48, clamy wrote: > > > > 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? > > > > > > 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. > > > > 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? > > 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. Great. The tests look happy. > 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. 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.) https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:292: RenderProcessHost* post_redirect_process_host, nit: Drop "_host", as below. https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:566: site_instance->HasProcess() ? site_instance : nullptr; On 2017/06/27 16:43:32, clamy wrote: > Note: I'm not storing the SiteInstance if its process is not initialized, > because I'm worried that someone could carelessly call GetProcess on it and > trigger an unwanted process initialization. Since we don't need to keep its > process alive (the motivation for storing it), I'd rather we don;t store it in > that case. Acknowledged. https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:277: // the speculative RenderFrameHost. I'm not entirely sure we want to do this TODO. I chatted with Nasko to learn that the motivation is to avoid a provisional local frame in the renderer process, which does seem good. However, there's a fair amount of work that we do when creating a RenderFrameHost and RenderFrame, including a possibly large number of IPCs to create any RenderFrameProxies it depends on. It seems useful to me to do this in advance, and in the common case (i.e., new process) there's not really a provisional local frame either. Maybe we should discuss this further before deciding to do it? Is there a bug filed for it where we can discuss? https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:278: scoped_refptr<SiteInstance> speculative_site_instance_; Hmm, is there a reason we need both this and dest_site_instance_? I notice that dest_site_instance_ is cleared at the start of NavigationRequest::OnRequestRedirected, and it seems like it might be a conceptual fit for what we're doing here. Is there a reason we need them to be separate? https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1971: } Interesting that we're changing the order here and doing this first. It's a behavior change, but I think it's an improvement-- it means we won't create the candidate_site_instance if it isn't necessary. (Side note: I hope we can revisit this block in the future, since it's really hard to follow and could easily have unintended behavior.)
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:560: *this); On 2017/06/27 18:46:34, Charlie Reis (slow) wrote: > On 2017/06/27 15:23:05, clamy wrote: > > On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > > > 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 http://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.) > > > > 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. > > 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? 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. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:562: site_instance->HasProcess() ? site_instance->GetProcess() : nullptr; On 2017/06/27 18:46:34, Charlie Reis (slow) wrote: > On 2017/06/27 15:23:05, clamy wrote: > > On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > > > On 2017/06/23 20:23:23, nasko (slow) wrote: > > > > On 2017/06/23 12:44:48, clamy wrote: > > > > > 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? > > > > > > > > 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. > > > > > > 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? > > > > 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. > > Great. The tests look happy. > > > 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. > > 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.) No we leave it in place until the navigation is ready to commit. https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:292: RenderProcessHost* post_redirect_process_host, On 2017/06/27 18:46:34, Charlie Reis (slow) wrote: > nit: Drop "_host", as below. Done. https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:277: // the speculative RenderFrameHost. On 2017/06/27 18:46:35, Charlie Reis (slow) wrote: > I'm not entirely sure we want to do this TODO. I chatted with Nasko to learn > that the motivation is to avoid a provisional local frame in the renderer > process, which does seem good. However, there's a fair amount of work that we > do when creating a RenderFrameHost and RenderFrame, including a possibly large > number of IPCs to create any RenderFrameProxies it depends on. It seems useful > to me to do this in advance, and in the common case (i.e., new process) there's > not really a provisional local frame either. > > Maybe we should discuss this further before deciding to do it? Is there a bug > filed for it where we can discuss? I have removed the TODO. https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:278: scoped_refptr<SiteInstance> speculative_site_instance_; On 2017/06/27 18:46:35, Charlie Reis (slow) wrote: > Hmm, is there a reason we need both this and dest_site_instance_? I notice that > dest_site_instance_ is cleared at the start of > NavigationRequest::OnRequestRedirected, and it seems like it might be a > conceptual fit for what we're doing here. Is there a reason we need them to be > separate? dest_site_instance_ comes from the NavigationEntry - we store it there in case the NavigationEntry goes away while navigating. It is used when we determine the final SiteInstance for navigation. I'm not sure it's safe to store the SiteInstance we compute on redirects there. https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1971: } On 2017/06/27 18:46:35, Charlie Reis (slow) wrote: > Interesting that we're changing the order here and doing this first. It's a > behavior change, but I think it's an improvement-- it means we won't create the > candidate_site_instance if it isn't necessary. > > (Side note: I hope we can revisit this block in the future, since it's really > hard to follow and could easily have unintended behavior.) I thought it made more sense to re-order it :). I agree that the block is hard to read. When PlzNavigate launches and we remove the old navigation code path, we might have the opportunity to improve it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM, but we should either use the speculative SiteInstance at commit or include a TODO for that before landing. https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:560: *this); On 2017/06/28 14:08:31, clamy wrote: > On 2017/06/27 18:46:34, Charlie Reis (slow) wrote: > > On 2017/06/27 15:23:05, clamy wrote: > > > On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > > > > 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 http://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.) > > > > > > 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. > > > > 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? > > 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. Good point-- such a SiteInstance wouldn't have a process yet (even if it's going to reuse), so we wouldn't store it. > 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. 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. > 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. 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! https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:278: scoped_refptr<SiteInstance> speculative_site_instance_; On 2017/06/28 14:08:32, clamy wrote: > On 2017/06/27 18:46:35, Charlie Reis (slow) wrote: > > Hmm, is there a reason we need both this and dest_site_instance_? I notice > that > > dest_site_instance_ is cleared at the start of > > NavigationRequest::OnRequestRedirected, and it seems like it might be a > > conceptual fit for what we're doing here. Is there a reason we need them to > be > > separate? > > dest_site_instance_ comes from the NavigationEntry - we store it there in case > the NavigationEntry goes away while navigating. It is used when we determine the > final SiteInstance for navigation. I'm not sure it's safe to store the > SiteInstance we compute on redirects there. Acknowledged. https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2954623003/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1971: } On 2017/06/28 14:08:32, clamy wrote: > On 2017/06/27 18:46:35, Charlie Reis (slow) wrote: > > Interesting that we're changing the order here and doing this first. It's a > > behavior change, but I think it's an improvement-- it means we won't create > the > > candidate_site_instance if it isn't necessary. > > > > (Side note: I hope we can revisit this block in the future, since it's really > > hard to follow and could easily have unintended behavior.) > > I thought it made more sense to re-order it :). I agree that the block is hard > to read. When PlzNavigate launches and we remove the old navigation code path, > we might have the opportunity to improve it. Acknowledged. https://codereview.chromium.org/2954623003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2954623003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:281: // PlzNavigate: |post_redirect_process_host| is the renderer process we expect nit: Same here (drop _host) https://codereview.chromium.org/2954623003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:564: site_instance->HasProcess() ? site_instance->GetProcess() : nullptr; nit: Please include a comment saying that we should not call GetProcess() if HasProcess() returns false, since we don't want to trigger a process creation.
Thanks! https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:560: *this); On 2017/06/28 17:05:22, Charlie Reis (slow) wrote: > On 2017/06/28 14:08:31, clamy wrote: > > On 2017/06/27 18:46:34, Charlie Reis (slow) wrote: > > > On 2017/06/27 15:23:05, clamy wrote: > > > > On 2017/06/27 04:24:53, Charlie Reis (slow) wrote: > > > > > 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 http://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.) > > > > > > > > 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. > > > > > > 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? > > > > 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. > > Good point-- such a SiteInstance wouldn't have a process yet (even if it's going > to reuse), so we wouldn't store it. > > > 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. > > 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. > > > > 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. > > 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! 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. https://codereview.chromium.org/2954623003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2954623003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:281: // PlzNavigate: |post_redirect_process_host| is the renderer process we expect On 2017/06/28 17:05:22, Charlie Reis (slow) wrote: > nit: Same here (drop _host) Done. https://codereview.chromium.org/2954623003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2954623003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:564: site_instance->HasProcess() ? site_instance->GetProcess() : nullptr; On 2017/06/28 17:05:22, Charlie Reis (slow) wrote: > nit: Please include a comment saying that we should not call GetProcess() if > HasProcess() returns false, since we don't want to trigger a process creation. Done.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2954623003/#ps80001 (title: "Addressed Charlie's comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498835636331150,
"parent_rev": "d6f1f465b8d6388fe7beca00f08c4adc12e63475", "commit_rev":
"0304a310bb32a007c09041384cd58f5a38d18e47"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects This CL is a follow up of the implementation of ProcessReusePolicy::REUSE_COMMITTED_OR_PENDING_SITE (https://codereview.chromium.org/2857213005/). It adds cross-site redirects that should be committed by an existing SiteInstance to the list of expected sites tracked per RenderProcessHost in RenderProcessHostImpl. BUG=705318 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects This CL is a follow up of the implementation of ProcessReusePolicy::REUSE_COMMITTED_OR_PENDING_SITE (https://codereview.chromium.org/2857213005/). It adds cross-site redirects that should be committed by an existing SiteInstance to the list of expected sites tracked per RenderProcessHost in RenderProcessHostImpl. BUG=705318 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2954623003 Cr-Commit-Position: refs/heads/master@{#483726} Committed: https://chromium.googlesource.com/chromium/src/+/0304a310bb32a007c09041384cd5... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0304a310bb32a007c09041384cd5... |
