Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(159)

Issue 2921063003: Fix process reuse for dedicated processes when over process limit. (Closed)

Created:
3 years, 6 months ago by alexmos
Modified:
3 years, 5 months ago
Reviewers:
Charlie Reis
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix process reuse for dedicated processes when over process limit. Previously, process reuse was broken for --isolate-origins, where an isolated origin was allowed to reuse a process containing content from other non-isolated sites when over process limit. It was also overly restrictive for --site-per-process, where a site could never reuse an existing process when over process limit, even if the process was dedicated to the same site. This CL fixes these cases by modifying RenderProcessHostImpl::IsSuitableHost() to consult ChildProcessSecurityPolicy and check whether the process is locked to a compatible site. For --site-per-process, it removes the "return false" in ShouldTryToUseExistingProcessHost(), which allows using IsSuitableHost(). One tricky case was caused by lazily-initialied SiteInstances: when a new WebContents is created with a new SiteInstance that has no site, that SiteInstance's process should be allowed to host a site that requires a dedicated process until something commits in it and the SiteInstance sets its site. Looking purely at origin locks isn't sufficient here, as we can't tell apart the case where the process is brand new and hasn't yet hosted any content vs. process that has committed URLs that don't require dedicated processes -- both end up with empty origin locks. To track this state, a new flag is added to RenderProcessHost. Another tricky case is when a page navigates itself to about:blank. This ought to be allowed to stay in-process, even for dedicated processes. This required adding an extra check to HasWrongProcessForURL() so it allows about:blank to stay in-process before it consults IsSuitableHost(), which would otherwise prevent this. The changes in this CL also revealed a bug in RenderProcessHostImpl::GetProcessHostForSite, which passed in a full URL instead of site URL to IsSuitableHost. BUG=513036, 713444 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2921063003 Cr-Commit-Position: refs/heads/master@{#482879} Committed: https://chromium.googlesource.com/chromium/src/+/13fe1960e0819a5b5e642de1b5b845875601f769

Patch Set 1 #

Patch Set 2 : Test fixes #

Patch Set 3 : More fixes #

Patch Set 4 : Fix some about:blank cases #

Patch Set 5 : Cleanup #

Total comments: 42

Patch Set 6 : Charlie's comments #

Patch Set 7 : Rebase #

Patch Set 8 : Fix IsSuitableHost for sites that require a dedicated process but don't set an origin lock #

Total comments: 16

Patch Set 9 : Charlie's comments #

Patch Set 10 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+421 lines, -47 lines) Patch
M content/browser/child_process_security_policy_impl.h View 1 2 3 4 5 6 7 8 1 chunk +17 lines, -1 line 0 comments Download
M content/browser/child_process_security_policy_impl.cc View 1 2 3 4 5 6 7 8 2 chunks +24 lines, -0 lines 0 comments Download
M content/browser/isolated_origin_browsertest.cc View 1 2 3 4 5 1 chunk +115 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.h View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 5 chunks +32 lines, -19 lines 0 comments Download
M content/browser/renderer_host/render_process_host_unittest.cc View 1 2 3 4 5 1 chunk +30 lines, -0 lines 0 comments Download
M content/browser/site_instance_impl.h View 1 2 3 4 5 6 7 8 2 chunks +14 lines, -1 line 0 comments Download
M content/browser/site_instance_impl.cc View 1 2 3 4 5 6 7 8 5 chunks +59 lines, -26 lines 0 comments Download
M content/browser/site_per_process_browsertest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +92 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M content/public/browser/render_process_host.h View 1 2 3 4 5 6 7 8 1 chunk +14 lines, -0 lines 0 comments Download
M content/public/test/mock_render_process_host.h View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download
M content/public/test/mock_render_process_host.cc View 1 2 3 4 5 6 7 8 9 2 chunks +9 lines, -0 lines 0 comments Download

Messages

Total messages: 63 (48 generated)
alexmos
Charlie, can you please take a look? This attempts to fix process reuse for both ...
3 years, 6 months ago (2017-06-12 17:35:08 UTC) #25
Charlie Reis
Nice. I'm really excited about the problems we're solving here (and the other ones you're ...
3 years, 6 months ago (2017-06-12 23:08:18 UTC) #26
alexmos
Thanks Charlie - responses below. For HasWrongProcessForURL and CPSP API, I'll wait for your feedback ...
3 years, 6 months ago (2017-06-14 22:39:05 UTC) #33
alexmos
https://codereview.chromium.org/2921063003/diff/80001/content/browser/site_instance_impl.cc File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2921063003/diff/80001/content/browser/site_instance_impl.cc#newcode472 content/browser/site_instance_impl.cc:472: process_->UnsetCanBecomeDedicatedProcess(); On 2017/06/14 22:39:05, alexmos wrote: > On 2017/06/12 ...
3 years, 6 months ago (2017-06-15 16:40:54 UTC) #36
alexmos
https://codereview.chromium.org/2921063003/diff/80001/content/browser/site_instance_impl.cc File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2921063003/diff/80001/content/browser/site_instance_impl.cc#newcode472 content/browser/site_instance_impl.cc:472: process_->UnsetCanBecomeDedicatedProcess(); On 2017/06/15 16:40:54, alexmos wrote: > On 2017/06/14 ...
3 years, 6 months ago (2017-06-15 17:27:59 UTC) #39
Charlie Reis
Thanks! This basically looks good, and let's go with your enum idea for the CPSP ...
3 years, 6 months ago (2017-06-17 23:13:53 UTC) #42
alexmos
Thanks for reviewing! More responses below. https://codereview.chromium.org/2921063003/diff/80001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2921063003/diff/80001/content/browser/renderer_host/render_process_host_impl.cc#newcode2874 content/browser/renderer_host/render_process_host_impl.cc:2874: return policy->IsLockedToOrigin(host->GetID(), site_url); ...
3 years, 6 months ago (2017-06-19 20:03:59 UTC) #45
Charlie Reis
Wow, sorry for the longer-than-expected delay. Thanks for updating the comments and CPSP API. LGTM! ...
3 years, 5 months ago (2017-06-28 00:08:38 UTC) #48
alexmos
Thanks for the review! https://codereview.chromium.org/2921063003/diff/140001/content/browser/site_instance_impl.h File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/2921063003/diff/140001/content/browser/site_instance_impl.h#newcode156 content/browser/site_instance_impl.h:156: // cases, most of which ...
3 years, 5 months ago (2017-06-28 00:33:26 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2921063003/160001
3 years, 5 months ago (2017-06-28 00:34:07 UTC) #51
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/241082) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 5 months ago (2017-06-28 00:36:31 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2921063003/180001
3 years, 5 months ago (2017-06-28 00:52:47 UTC) #56
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/489574)
3 years, 5 months ago (2017-06-28 02:42:58 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2921063003/180001
3 years, 5 months ago (2017-06-28 03:42:19 UTC) #60
commit-bot: I haz the power
3 years, 5 months ago (2017-06-28 04:25:30 UTC) #63
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/13fe1960e0819a5b5e642de1b5b8...

Powered by Google App Engine
This is Rietveld 408576698