Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861433002:
Implement ProcessReusePolicy for SiteInstances (Closed)
|
Created:
3 years, 7 months ago by clamy Modified:
3 years, 7 months ago CC:
chromium-reviews, creis+watch_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org, ncarter (slow) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement ProcessReusePolicy for SiteInstances
This CL is the first step in an effort to add new policies for reusing
processes when creating SiteInstances. It adds a ProcessReusePolicy that
defines the rules for reusing processes, and which is used by
RenderProcessHostImpl when attempting to create a RenderProcessHost for
the SiteInstance. As a side-effect, the code for creating a RPH has been
moved from SiteInstanceImpl::GetProcess to a static method in
RenderProcessHostImpl. See
https://docs.google.com/a/chromium.org/document/d/1Zujvf5jcUtm1hyh-xhXYlyfPjdO2oEplz22fOhSj660/edit?usp=sharing
for the motivation behind this change.
BUG=705318
Review-Url: https://codereview.chromium.org/2861433002
Cr-Commit-Position: refs/heads/master@{#470740}
Committed: https://chromium.googlesource.com/chromium/src/+/63960c37c94873829b5937bf512809800f6eef6a
Patch Set 1 #Patch Set 2 : Updated comments #Patch Set 3 : Rebase #
Total comments: 40
Patch Set 4 : Addressed comments #Patch Set 5 : Rebase #Patch Set 6 : Reverted default subframe process code #Patch Set 7 : Fixed ChromeOS issue #Messages
Total messages: 35 (24 generated)
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...
clamy@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
@creis, nasko: PTAL. This is what I had in mind for the process reuse policy + SiteInstance + RPH. (Note: this is not a fully polished CL). Let me know what you think and if it's going in the right direction.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/02 16:58:47, clamy wrote: > @creis, nasko: PTAL. This is what I had in mind for the process reuse policy + > SiteInstance + RPH. (Note: this is not a fully polished CL). Let me know what > you think and if it's going in the right direction. Thanks! This is looking really good so far. I've only had a chance to skim it so far and don't have concrete comments yet (since my day was full of meetings), but I like the approach. I'll try to provide some more detailed comments tomorrow afternoon.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! A few thoughts below, but nothing major. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2792: const GURL url = site_instance->GetSiteURL(); nit: site_url (This is a meaningful difference in what the GURL is.) https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2798: // First, attempt to reuse an existing RenderProcessHost if applicable. nit: s/applicable/necessary/ (We may also reuse when over the limit, but that's not handled in this block.) https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2804: render_process_host = GetDefaultSubframeProcessHost( DCHECK(SiteIsolationPolicy::IsTopDocumentIsolationEnabled()), perhaps? https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2811: if (!render_process_host && Please keep the old comment: // If not (or if none found), see if we should reuse an existing process. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:261: // The policy to apply when selecting a RenderProcessHost for a site. I'm unclear if this is just about process-per-site style reuse or if we'll extend it for all the types of reuse in https://docs.google.com/a/chromium.org/document/d/1UrrmlcqwM5qIpqBM3ud3gYY5uG.... For example, normal SiteInstances don't proactively reuse processes, but when over the limit, they reuse randomly, not based on site. In contrast, process-per-site and ServiceWorkers proactively join a process that already contains the site. The "for a site" part of this comment seems to imply it might be specifically for the cases where affinity is based on site, but then again, it's handling TDI's default subframe process already. Maybe we should rephrase the comment about how much we expect this will cover in the future? https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:262: enum class ProcessReusePolicy { It's a bit odd that this enum is not used in any APIs here. Should we declare it in SiteInstance(Impl?) instead? https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:264: DONT, Let's clarify if this really means "never reuse, even when over the process limit," since at first I thought it might mean "don't proactively reuse, unless we're over the process limit" (until I saw DEFAULT). Is there a case we know we need this for? I see that it's easy to implement, but I think there's still some open questions about whether it's needed. For example, even a site that requires a dedicated process could still share with other instances of that site when over the limit. Same for non-tab features that use WebContents. I could imagine implementing that more as pools (e.g., defined by site), rather than saying "never reuse under any circumstances." In other words, we might want to leave this out until there's a clear need for it. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:266: // In this mode, all instances of the site will be rendered in the same Is the idea to also use this value for ServiceWorker SiteInstances, to make them join one (of possibly many) existing processes? And for process-per-site sites (e.g., WebUI), we use it for all SiteInstances in the site? If so, we should rephrase this-- in the ServiceWorker case, we'll set this value for the ServiceWorker SiteInstance but not "all instances of the site." The normal tabs for the site will still get the default value. Since we're choosing whether to apply this value to one or all SiteInstances for a site to define different policies, that's a second reason to move this enum to SiteInstance instead of RenderProcessHost. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:272: USE_DEFAULT_SUBFRAME_INSTANCE, s/INSTANCE/PROCESS/, perhaps? There are multiple default subframe SiteInstances (one per BrowsingInstance), but only one RenderProcessHost. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:275: // limit has been reached. Let's mention that reuse after the limit is random, not based on site. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:280: // on the SiteInstance ProcessReusePolicy and its url, this may be an existing nit: SiteInstance's https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:329: static void set_render_process_host_factory( Thanks-- this makes more sense here. https://codereview.chromium.org/2861433002/diff/40001/content/browser/site_in... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/site_in... content/browser/site_instance_impl.cc:108: RenderProcessHostImpl::RegisterProcessHostForSite(browser_context, Ah, I suppose there's still a fair amount needed before we can use PROCESS_PER_SITE on a per-SiteInstance basis (e.g., for ServiceWorker), since we'll also need to register sites in each RPH in all cases. But that's fine to do later-- the other description updates in the enum should still apply.
https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:478: host->SetIsNeverSuitableForReuse(); What about different GuestView instances that share a StoragePartition? They need to be in the same process. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:483: // If we already have a shared host for the default storage partition, use nit: I'd put the comment above the if statement and skip having {}. nit: s/storage partition/StoragePartition/ https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:497: void RenderProcessHostDestroyed(RenderProcessHost* host) override { nit: Add a comment above that this is an implementation of the RenderProcessHostObserver interface. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3165: bool is_for_guests_only) { Thinking more about this, does this bool param make sense in the context of subframes? Guests don't have subframes that live in separate processes. We want to change that, but it will be very big undertaking. I think it will be much easier to reason about this code if we just skip that parameter. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:264: DONT, nit: DO_NOT https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:266: // In this mode, all instances of the site will be rendered in the same nit: s/rendererd/hosted/? https://codereview.chromium.org/2861433002/diff/40001/content/browser/site_in... File content/browser/site_instance_impl.cc (left): https://codereview.chromium.org/2861433002/diff/40001/content/browser/site_in... content/browser/site_instance_impl.cc:149: bool is_for_guests_only) { Ok, I didn't realize this is code that just moved over when I read it as new in the other file. I'm fine leaving it as is, since it is a move.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! For the record, I have uploaded a first draft of the change for ServiceWorkers at https://codereview.chromium.org/2857213005/. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:478: host->SetIsNeverSuitableForReuse(); On 2017/05/03 23:46:16, nasko wrote: > What about different GuestView instances that share a StoragePartition? They > need to be in the same process. Following the removing of is_for_guest only asked below, I now keep a map of default subframe RPH per StoragePartition. So if different instances share a StoragePartition they should end up in the same RPH. Is that what you had in mind? https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:483: // If we already have a shared host for the default storage partition, use On 2017/05/03 23:46:16, nasko wrote: > nit: I'd put the comment above the if statement and skip having {}. > nit: s/storage partition/StoragePartition/ Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:497: void RenderProcessHostDestroyed(RenderProcessHost* host) override { On 2017/05/03 23:46:17, nasko wrote: > nit: Add a comment above that this is an implementation of the > RenderProcessHostObserver interface. Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2792: const GURL url = site_instance->GetSiteURL(); On 2017/05/03 23:28:00, Charlie Reis wrote: > nit: site_url > (This is a meaningful difference in what the GURL is.) Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2798: // First, attempt to reuse an existing RenderProcessHost if applicable. On 2017/05/03 23:28:00, Charlie Reis wrote: > nit: s/applicable/necessary/ > (We may also reuse when over the limit, but that's not handled in this block.) Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2804: render_process_host = GetDefaultSubframeProcessHost( On 2017/05/03 23:28:00, Charlie Reis wrote: > DCHECK(SiteIsolationPolicy::IsTopDocumentIsolationEnabled()), perhaps? Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2811: if (!render_process_host && On 2017/05/03 23:28:00, Charlie Reis wrote: > Please keep the old comment: > // If not (or if none found), see if we should reuse an existing process. Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3165: bool is_for_guests_only) { On 2017/05/03 23:46:17, nasko wrote: > Thinking more about this, does this bool param make sense in the context of > subframes? Guests don't have subframes that live in separate processes. We want > to change that, but it will be very big undertaking. > I think it will be much easier to reason about this code if we just skip that > parameter. Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:261: // The policy to apply when selecting a RenderProcessHost for a site. On 2017/05/03 23:28:00, Charlie Reis wrote: > I'm unclear if this is just about process-per-site style reuse or if we'll > extend it for all the types of reuse in > https://docs.google.com/a/chromium.org/document/d/1UrrmlcqwM5qIpqBM3ud3gYY5uG.... > > For example, normal SiteInstances don't proactively reuse processes, but when > over the limit, they reuse randomly, not based on site. In contrast, > process-per-site and ServiceWorkers proactively join a process that already > contains the site. > > The "for a site" part of this comment seems to imply it might be specifically > for the cases where affinity is based on site, but then again, it's handling > TDI's default subframe process already. Maybe we should rephrase the comment > about how much we expect this will cover in the future? I have rephrased the comment. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:262: enum class ProcessReusePolicy { On 2017/05/03 23:28:00, Charlie Reis wrote: > It's a bit odd that this enum is not used in any APIs here. Should we declare > it in SiteInstance(Impl?) instead? Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:264: DONT, On 2017/05/03 23:46:17, nasko wrote: > nit: DO_NOT I initially thought we might want to change RenderProcessHostImpl::ShouldTryToUseExistingProcessHost so that when SiteIsolationPolicy::UseDedicatedProcessesForAllSites() is true (and not in single process mode), we intialize the SiteInstance with this value instead of default. But this is probably out-of-scope for this CL, so I'm removing it. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:266: // In this mode, all instances of the site will be rendered in the same On 2017/05/03 23:46:17, nasko wrote: > nit: s/rendererd/hosted/? Done. For the naming question, I'm planning a different value for ServiceWorkers, since we also have to take into account pending navigations (I've called it REUSE_CHECKING_FRAMES_AND_NAVIGATIONS). If we want, we can eventually name this one REUSE_CHECKING_FRAMES. Note that in this CL I'm trying not to make changes to the functionality, and in particular I'm not changing the implementation of process-per-site. I'd rather leave it named as it is, and change the name once we change the underlying implemetation. Wdyt? https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:272: USE_DEFAULT_SUBFRAME_INSTANCE, On 2017/05/03 23:28:00, Charlie Reis wrote: > s/INSTANCE/PROCESS/, perhaps? > > There are multiple default subframe SiteInstances (one per BrowsingInstance), > but only one RenderProcessHost. Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:275: // limit has been reached. On 2017/05/03 23:28:00, Charlie Reis wrote: > Let's mention that reuse after the limit is random, not based on site. Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:280: // on the SiteInstance ProcessReusePolicy and its url, this may be an existing On 2017/05/03 23:28:00, Charlie Reis wrote: > nit: SiteInstance's Done. https://codereview.chromium.org/2861433002/diff/40001/content/browser/site_in... File content/browser/site_instance_impl.cc (left): https://codereview.chromium.org/2861433002/diff/40001/content/browser/site_in... content/browser/site_instance_impl.cc:149: bool is_for_guests_only) { On 2017/05/03 23:46:17, nasko wrote: > Ok, I didn't realize this is code that just moved over when I read it as new in > the other file. I'm fine leaving it as is, since it is a move. I did the modifications as requested in the other file before reading this comment. I'm uploading them, let me know what you prefer. https://codereview.chromium.org/2861433002/diff/40001/content/browser/site_in... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/site_in... content/browser/site_instance_impl.cc:108: RenderProcessHostImpl::RegisterProcessHostForSite(browser_context, On 2017/05/03 23:28:00, Charlie Reis wrote: > Ah, I suppose there's still a fair amount needed before we can use > PROCESS_PER_SITE on a per-SiteInstance basis (e.g., for ServiceWorker), since > we'll also need to register sites in each RPH in all cases. But that's fine to > do later-- the other description updates in the enum should still apply. Indeed. The goal of this patch is to move the code in a state where it's easier to modify.
Great. I think this is almost ready, except that I'd recommend not doing the is_for_guests_only change (at least in this CL). Are the ChromeOS test failures real? I see that they're recurring across patchsets: UserTypeInstantiation/AccessibilityManagerUserTypeTest.EnableOnLoginScreenAndLogin/2 UserTypeInstantiation/AccessibilityManagerUserTypeTest.EnableOnLoginScreenAndLogin/1 UserTypeInstantiation/AccessibilityManagerUserTypeTest.EnableOnLoginScreenAndLogin/0 https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3165: bool is_for_guests_only) { On 2017/05/04 15:56:18, clamy wrote: > On 2017/05/03 23:46:17, nasko wrote: > > Thinking more about this, does this bool param make sense in the context of > > subframes? Guests don't have subframes that live in separate processes. We > want > > to change that, but it will be very big undertaking. > > I think it will be much easier to reason about this code if we just skip that > > parameter. > > Done. I'm pretty nervous about this change, for two reasons. 1) This CL is mostly about moving code, so it's probably better not to change behavior as we do it. (If we decide we want to make the change, I think it's better to do it in a separate CL.) 2) I'm not actually sure which approach is safer. This was added in Avi's https://codereview.chromium.org/2573213002. From talking with Nick, I don't think they were considering whether TDI would create a subframe process for guests-- just what plumbing was needed to create a RPHI. It's true that we prevent OOPIFs at a different layer, but we're kind of considering changing that at some point. Assuming this is false here means we're making the OOPIFs-in-guests policy decision in two places, and we might forget this one if we change things later. It seems like the risk with Avi's approach is that it might make us create a subframe process for guests in TDI mode and bypass the other no-OOPIFs-in-guests policy, but that doesn't seem to happen in practice (I just tested). I think I'd recommend undoing this part of the change and sticking to only moving the code for now. We can discuss the tradeoffs of the change on a separate CL if we want to do it. https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:266: // In this mode, all instances of the site will be rendered in the same On 2017/05/04 15:56:19, clamy wrote: > On 2017/05/03 23:46:17, nasko wrote: > > nit: s/rendererd/hosted/? > Done. > > For the naming question, I'm planning a different value for ServiceWorkers, > since we also have to take into account pending navigations (I've called it > REUSE_CHECKING_FRAMES_AND_NAVIGATIONS). > > If we want, we can eventually name this one REUSE_CHECKING_FRAMES. Note that in > this CL I'm trying not to make changes to the functionality, and in particular > I'm not changing the implementation of process-per-site. I'd rather leave it > named as it is, and change the name once we change the underlying implemetation. > Wdyt? Works for me, thanks.
https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2861433002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3165: bool is_for_guests_only) { On 2017/05/04 23:12:23, Charlie Reis wrote: > On 2017/05/04 15:56:18, clamy wrote: > > On 2017/05/03 23:46:17, nasko wrote: > > > Thinking more about this, does this bool param make sense in the context of > > > subframes? Guests don't have subframes that live in separate processes. We > > want > > > to change that, but it will be very big undertaking. > > > I think it will be much easier to reason about this code if we just skip > that > > > parameter. > > > > Done. > > I'm pretty nervous about this change, for two reasons. > > 1) This CL is mostly about moving code, so it's probably better not to change > behavior as we do it. (If we decide we want to make the change, I think it's > better to do it in a separate CL.) > > 2) I'm not actually sure which approach is safer. This was added in Avi's > https://codereview.chromium.org/2573213002. From talking with Nick, I don't > think they were considering whether TDI would create a subframe process for > guests-- just what plumbing was needed to create a RPHI. It's true that we > prevent OOPIFs at a different layer, but we're kind of considering changing that > at some point. Assuming this is false here means we're making the > OOPIFs-in-guests policy decision in two places, and we might forget this one if > we change things later. > > It seems like the risk with Avi's approach is that it might make us create a > subframe process for guests in TDI mode and bypass the other no-OOPIFs-in-guests > policy, but that doesn't seem to happen in practice (I just tested). > > I think I'd recommend undoing this part of the change and sticking to only > moving the code for now. We can discuss the tradeoffs of the change on a > separate CL if we want to do it. As I pointed out in one of my comments, I added the comments in this file before I realized it is a move. I definitely agree that this CL should just move the code around to minimize changes. The bool can always be investigated separately. I just reviewed this file as brand new code initially.
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! I've come back to the original default subframe process code. I have also fixed the ChromeOS issue by having the policy be set to process-per-site at the same place where it used to be set (issue was we weren't treating as process-per-site when getting the processes, so we ended up creating too many of them and they leaked).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM.
The CQ bit was checked by creis@chromium.org
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": 120001, "attempt_start_ts": 1494448434354380,
"parent_rev": "6c27b49745a0438d33ca75f939a90a3bf8205d77", "commit_rev":
"63960c37c94873829b5937bf512809800f6eef6a"}
Message was sent while issue was closed.
Description was changed from ========== Implement ProcessReusePolicy for SiteInstances This CL is the first step in an effort to add new policies for reusing processes when creating SiteInstances. It adds a ProcessReusePolicy that defines the rules for reusing processes, and which is used by RenderProcessHostImpl when attempting to create a RenderProcessHost for the SiteInstance. As a side-effect, the code for creating a RPH has been moved from SiteInstanceImpl::GetProcess to a static method in RenderProcessHostImpl. See https://docs.google.com/a/chromium.org/document/d/1Zujvf5jcUtm1hyh-xhXYlyfPjd... for the motivation behind this change. BUG=705318 ========== to ========== Implement ProcessReusePolicy for SiteInstances This CL is the first step in an effort to add new policies for reusing processes when creating SiteInstances. It adds a ProcessReusePolicy that defines the rules for reusing processes, and which is used by RenderProcessHostImpl when attempting to create a RenderProcessHost for the SiteInstance. As a side-effect, the code for creating a RPH has been moved from SiteInstanceImpl::GetProcess to a static method in RenderProcessHostImpl. See https://docs.google.com/a/chromium.org/document/d/1Zujvf5jcUtm1hyh-xhXYlyfPjd... for the motivation behind this change. BUG=705318 Review-Url: https://codereview.chromium.org/2861433002 Cr-Commit-Position: refs/heads/master@{#470740} Committed: https://chromium.googlesource.com/chromium/src/+/63960c37c94873829b5937bf5128... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/63960c37c94873829b5937bf5128... |
