New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 713444 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 513036

Blocking:
issue 716053



Sign in to add a comment

Support site isolation for specific origins rather than sites

Project Member Reported by alex...@chromium.org, Apr 20 2017

Issue description

Some OOPIF modes that we're pursuing require us to support process isolation for specific origins rather than sites.  For example, we would like to be able to specify that https://isolated.foo.com is an isolated origin which should be placed in a different process from https://www.foo.com or https://foo.com.  When making process model decisions for such origins, our SiteInstance logic (such as GetSiteForURL) will need to use the full scheme+host+port tuple rather than just the eTLD+1.  This bug will track progress for this work.
 

Comment 1 by ew...@chromium.org, Apr 28 2017

Cc: msarda@chromium.org ew...@chromium.org

Comment 2 by ew...@chromium.org, Apr 28 2017

Blocking: 716053
Project Member

Comment 3 by bugdroid1@chromium.org, May 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3b9ad10d35e7cb293f559d262bf417d7aa099f4d

commit 3b9ad10d35e7cb293f559d262bf417d7aa099f4d
Author: alexmos <alexmos@chromium.org>
Date: Fri May 26 23:41:08 2017

Introduce support for origins that require process isolation.

This CL changes logic in SiteInstance to support isolating specific
origins based on the full scheme+host+port tuple rather than just
scheme and eTLD+1.  The global list of such origins is maintained by
SiteInstanceImpl and currently not exposed outside of content.

This CL also adds a command-line option to add isolated origins.
Sample usage:
  --isolate-origins=https://isolated.foo.com,https://bar.com

Design doc: https://goo.gl/99ynqK

BUG= 713444 

Review-Url: https://codereview.chromium.org/2831683002
Cr-Commit-Position: refs/heads/master@{#475191}

[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/browser_main_loop.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/child_process_security_policy_unittest.cc
[add] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/site_instance_impl.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/site_instance_impl.h
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/common/site_isolation_policy.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/common/site_isolation_policy.h
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/public/common/content_switches.cc
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/public/common/content_switches.h
[modify] https://crrev.com/3b9ad10d35e7cb293f559d262bf417d7aa099f4d/content/test/BUILD.gn

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b7555fc8a5b8eddc838c28595199eb663e2587a

commit 7b7555fc8a5b8eddc838c28595199eb663e2587a
Author: alexmos <alexmos@chromium.org>
Date: Thu Jun 01 22:58:32 2017

Consolidate isolated origin subframes into existing processes.

Previously, isolated origins subframes from unrelated tabs would
always get their own process.  This results in too many tabs for
scenarios like sign-in, where there's a sign-in subframe on every
Google tab.  To address this, set the process reuse policy to allow
isolated origin subframes to reuse existing processes for that origin.

BUG= 713444 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2920473005
Cr-Commit-Position: refs/heads/master@{#476467}

[modify] https://crrev.com/7b7555fc8a5b8eddc838c28595199eb663e2587a/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/7b7555fc8a5b8eddc838c28595199eb663e2587a/content/browser/isolated_origin_browsertest.cc

Blockedon: 513036
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13fe1960e0819a5b5e642de1b5b845875601f769

commit 13fe1960e0819a5b5e642de1b5b845875601f769
Author: alexmos <alexmos@chromium.org>
Date: Wed Jun 28 04:25:12 2017

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}

[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/site_instance_impl.cc
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/site_instance_impl.h
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/browser/web_contents/web_contents_impl_unittest.cc
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/public/browser/render_process_host.h
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/13fe1960e0819a5b5e642de1b5b845875601f769/content/public/test/mock_render_process_host.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4bc26323b196f3b8957e85fc1eb48c08456fc84d

commit 4bc26323b196f3b8957e85fc1eb48c08456fc84d
Author: alexmos <alexmos@chromium.org>
Date: Sat Jul 01 00:57:14 2017

Keep subdomains of an isolated origin in the isolated origin's SiteInstance.

Previously, if bar.foo.com was an isolated origin, subdomains like
subdomain.bar.foo.com would end up in a different SiteInstance (using
"foo.com" for its site URL) than the isolated origin, which was
confusing/undesirable.  There was also confusion with subdomains if an
etld+1 (e.g., isolated.com) was marked as an isolated origin: we would
try to assign a different SiteInstance to foo.isolated.com than
isolated.com, yet the site URL would still resolve to "isolated.com".

This CL changes this behavior to keep subdomains in the isolated
origin's SiteInstance.  It also adds conflict resolution, which allows
adding multiple isolated origins with a common domain (e.g., c.b.a.com
and a.com), where the most specific isolated origin is used when
picking the SiteInstance site URL for a particular URL (e.g., b.a.com
would use a.com, but d.c.b.a.com would use c.b.a.com).

For more discussion about this, see the isolated origin design doc:
https://goo.gl/99ynqK

BUG= 713444 

Review-Url: https://codereview.chromium.org/2891443002
Cr-Commit-Position: refs/heads/master@{#483881}

[modify] https://crrev.com/4bc26323b196f3b8957e85fc1eb48c08456fc84d/content/browser/BUILD.gn
[modify] https://crrev.com/4bc26323b196f3b8957e85fc1eb48c08456fc84d/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/4bc26323b196f3b8957e85fc1eb48c08456fc84d/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/4bc26323b196f3b8957e85fc1eb48c08456fc84d/content/browser/isolated_origin_browsertest.cc
[add] https://crrev.com/4bc26323b196f3b8957e85fc1eb48c08456fc84d/content/browser/isolated_origin_util.cc
[add] https://crrev.com/4bc26323b196f3b8957e85fc1eb48c08456fc84d/content/browser/isolated_origin_util.h
[modify] https://crrev.com/4bc26323b196f3b8957e85fc1eb48c08456fc84d/content/browser/site_instance_impl.cc
[modify] https://crrev.com/4bc26323b196f3b8957e85fc1eb48c08456fc84d/content/browser/site_instance_impl_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d252c196594fd41bf5bd4e78a63ed344640992c4

commit d252c196594fd41bf5bd4e78a63ed344640992c4
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Jul 17 22:03:48 2017

Set up an experiment for requiring a dedicated process for sign-in.

This CL introduces a new base::Feature to control whether the sign-in
origin (https://accounts.google.com) requires a dedicated process, to
be used for a Finch experiment.  As part of this, during
initialization content/ will call out to a new ContentBrowserClient
method to get a whitelist of origins requiring a dedicated process.
These origins will be process-isolated in all profiles for the
lifetime of the browser.

Bug:  713444 , 739418
Change-Id: I308cd1eb209394a9990c5eb5793a95a78d76485a
Reviewed-on: https://chromium-review.googlesource.com/569519
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487263}
[modify] https://crrev.com/d252c196594fd41bf5bd4e78a63ed344640992c4/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/d252c196594fd41bf5bd4e78a63ed344640992c4/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/d252c196594fd41bf5bd4e78a63ed344640992c4/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/d252c196594fd41bf5bd4e78a63ed344640992c4/content/browser/browser_main_loop.cc
[modify] https://crrev.com/d252c196594fd41bf5bd4e78a63ed344640992c4/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/d252c196594fd41bf5bd4e78a63ed344640992c4/content/public/browser/content_browser_client.h
[modify] https://crrev.com/d252c196594fd41bf5bd4e78a63ed344640992c4/content/public/common/content_features.cc
[modify] https://crrev.com/d252c196594fd41bf5bd4e78a63ed344640992c4/content/public/common/content_features.h

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9cf5a45d77f299616e5bfd086348bd6b35386f05

commit 9cf5a45d77f299616e5bfd086348bd6b35386f05
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Aug 14 18:50:57 2017

Add sanity checks for LockToOrigin.

These checks ensure that:

1. When determining a process for a SiteInstance, we never return an
existing process with a wrong StoragePartition.

2. We never attempt to lock a process to an origin if the process has
already been locked to a different origin.

3. When locking a process to an origin, the process has not previously
been used to host any other web sites.

This is a followup to https://chromium-review.googlesource.com/c/602707

These checks revealed a few unit tests that were broken for
--site-per-process:

- tests like TabsApiUnitTest.QueryWithHostPermission ran in
  --single-process mode, but that wasn't propagating to the decision
  made in ShouldLockToOrigin, because it was set via
  SetRunRendererInProcess, while ShouldLockToOrigin checks for the
  command-line flag.  This has been fixed by checking
  run_renderer_in_process() from ShouldLockToOrigin instead.

- two tests in render_process_host_unittest.cc were directly
  committing cross-site navigations into the same RenderFrameHost,
  which is invalid with --site-per-process.  The tests were fixed
  to utilize pending/speculative RFHs if available.

Bug: 751916, 751920,  713444 
Change-Id: I0e754e65731c7871adc872b8dce42253842480c6
Reviewed-on: https://chromium-review.googlesource.com/604481
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494114}
[modify] https://crrev.com/9cf5a45d77f299616e5bfd086348bd6b35386f05/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/9cf5a45d77f299616e5bfd086348bd6b35386f05/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/9cf5a45d77f299616e5bfd086348bd6b35386f05/content/browser/site_instance_impl.cc
[modify] https://crrev.com/9cf5a45d77f299616e5bfd086348bd6b35386f05/content/browser/site_instance_impl.h

Status: Fixed (was: Started)
The core support for isolated origins is complete.  I'll mark this as fixed, and we can file new bugs for any new issues.  Note that the launch bug for using this for sign-in is issue 739418.

Sign in to add a comment