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

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 873780



Sign in to add a comment
link

Issue 856734: Make site-per-process the default at the //content layer

Reported by lukasza@chromium.org, Jun 26 2018 Project Member

Issue description

We'd like to make site-per-process the default at the //content layer.  At the code level, this can be done by tweaking the default implementation of ContentBrowserClient::ShouldEnableStrictSiteIsolation (although we also have to be careful to preserve for the next few milestones the ability to test site-per-process via field trials).

Making site-per-process the default everywhere is desirable for:

- Unified test steps (e.g. content_browsertests and not_site_per_process_content_browsertests - just like browser_tests and not_site_per_process_browser_tests)

- Secure-by-default + testing-what-we-ship-by-default (e.g. unless //headless uses site-per-process it doesn't really test Chrome that ships to end users).
 

Comment 1 by creis@chromium.org, Jun 27 2018

Cc: creis@chromium.org a...@chromium.org
This is mainly a problem for other content embedders, who don't expect Site Isolation to be enabled by default (and have had concerns about this in the past).  We could attempt to do this with enough outreach to the embedders, giving them ample time to opt out before we make it the default.  (I'm already planning to reach out to them to say we've enabled it in Chrome and it's available if they want to use it for Spectre, so we could lay out a timeframe there of making it opt-out rather than opt-in if we want.)

Still, I think we should probably fix the testing problem in the short term by ensuring our own bots are running what we ship, perhaps with some coverage of the non Site Isolation path on desktop on certain bots as well.

Comment 3 by creis@chromium.org, Jul 16 2018

Cc: lukasza@chromium.org nasko@chromium.org alex...@chromium.org
Labels: -Pri-3 Target-70 Pri-2
Owner: creis@chromium.org
Status: Assigned (was: Untriaged)
As discussed in the link in comment 2, we'll plan to move forward with this by sending a heads-up to the Content Embedders list (embedder-dev@chromium.org) before making the change.  I think we can probably target M70.

Comment 5 by lukasza@chromium.org, Jul 26 2018

Quite a few layout tests are disabled via third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.  (quite a few = 136 - this excludes virtual/... variants of tests to avoid double-counting).  If site-per-process is the default at the //content layer, then I assume that these expectations would have to move to third_party/WebKit/LayoutTests/TestExpectations.  This seems undesirable, because:
1. These tests would be effectively deleted/disabled
2. We could in theory re-enable these tests in not_site_per_process test step, but
    2.a. Having these tests run only on a single CQ/Waterfall bot seems undesirable and
    2.b. I don't see how to re-enable these tests for not_site_per_process step (based on my past experience with CORB adding a [ Pass ] expectation for FlagExpectations/disable-site-isolation-trials will not fully override other expectations from TestExpectations).

Hmmm... maybe this trouble can be avoided (postponed / kicked down the road...) by keeping site-per-process disabled by default in LayoutTestContentBrowserClient...

Comment 6 by thakis@chromium.org, Jul 30 2018

We're shipping site-per-process. If we disable it in the layout test runner, we're lying to ourselves: These tests are broken in chrome. We should disable them, and reenable when we fix them.

Comment 7 by dpranke@chromium.org, Jul 30 2018

Cc: dpranke@chromium.org jam@chromium.org
If we need to keep both code paths around for the indefinite future, I don't actually care too much which is the default, since we'll presumably have to test both.

However, we're talking about very expensive test suites here. I'm guessing that we don't have a feasible way of figuring out which subsets of the suites are really affected by the setting?

Comment 8 by thakis@chromium.org, Jul 30 2018

My understanding of the thread was that we can stop running the not_site_per_process tests after this is done (https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/25pO_47w9R0/Qg-FhtaGCAAJ), which includes not_site_per_process_browser_tests and not_site_per_process_unit_tests, the largest of the test suites.

Comment 9 by dpranke@chromium.org, Jul 30 2018

I can see that conclusion in the thread, but not why it'd be justified :).

Comment 10 by bugdroid1@chromium.org, Jul 31 2018

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

commit 7d7da7f2faea241f9cff38c8ce4014d38a3b996e
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Jul 31 00:21:59 2018

Inline site-per-process expectations for content_browsertests.

We plan to make --site-per-process mode the default mode on ToT for
//content layer (//chrome layer will continue to be controlled by field
trials and disable --site-per-process on Android).  To make this switch
slightly easier, this CL inlines the test expectations from
//testing/buildbot/filters into the source code of the tests - this
should avoid having to keep passing the filter files to the default test
steps after making the switch.

Bug: 856734
Change-Id: Ic0983cb667d7904caeb18d46705f7721147a6354
Reviewed-on: https://chromium-review.googlesource.com/1153975
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579246}
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/content/browser/loader/loader_browsertest.cc
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/testing/buildbot/client.v8.chromium.json
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/testing/buildbot/filters/BUILD.gn
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/testing/buildbot/filters/README.md
[delete] https://crrev.com/fb29bf60fc9fb1e7312165a78fe8035a98d15e44/testing/buildbot/filters/site-per-process.content_browsertests.filter
[modify] https://crrev.com/7d7da7f2faea241f9cff38c8ce4014d38a3b996e/testing/buildbot/test_suites.pyl

Comment 11 by nasko@chromium.org, Jul 31 2018

As of the last CL, we don't have an easy way to list which tests are disabled for site-per-process. Do we have bugs filed for all of those that will make it easier to track how many are left?

Comment 12 by lukasza@chromium.org, Jul 31 2018

RE: #c11 from nasko@: See https://crbug.com/417518#c74.  Even before the CL in #c10 we didn't have a comprehensive/complete list...  Yes - hopefully all disabled (or partially-disabled) tests are tracked by a bug.

Comment 13 by bugdroid1@chromium.org, Aug 10

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

commit 3b8db25cc597a642ee4b7cbaf2655c631353a799
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Aug 10 17:23:35 2018

Make //content layer default to --site-per-process mode on desktop.

After this CL site-per-process is the default in all layers on desktop
platforms, except:

- Layout Tests which still run with no isolation by default
  (see https://crbug.com/856734#c5 for the explanation why)

- //content embedders that don't want to use site-per-process: //chromecast

- //content embedders that don't yet support site-per-process, but will
  need to migrate eventually: //headless


Also note that even after this CL:

- //content on Android still defaults to no isolation.
  This is compatible with not_site_per_process_* test steps
  because such steps are not run on Android bots...

- //chrome layer ChromeContentBrowserClient controls usage of
  site-per-process in the Chrome browser and continues to be controlled
  by a field trial (note that Android is not covered by the field trial
  or by testing/variations/fieldtrial_testing_config.json).


Bug: 856734
Change-Id: Iebd46e5502583f84a4b0a7c2ee8d8b0e081c6ff2
Reviewed-on: https://chromium-review.googlesource.com/1153981
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Stephen Lanham <slan@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582223}
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/android_webview/browser/aw_content_browser_client.cc
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/android_webview/browser/aw_content_browser_client.h
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/chromecast/browser/cast_content_browser_client.cc
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/chromecast/browser/cast_content_browser_client.h
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/content/browser/geolocation/geolocation_service_impl_unittest.cc
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/content/browser/web_package/signed_exchange_request_handler_browsertest.cc
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/content/public/browser/content_browser_client.h
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/content/shell/browser/layout_test/layout_test_content_browser_client.cc
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/content/shell/browser/layout_test/layout_test_content_browser_client.h
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/headless/lib/browser/headless_content_browser_client.cc
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/headless/lib/browser/headless_content_browser_client.h
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/testing/buildbot/client.v8.chromium.json
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/3b8db25cc597a642ee4b7cbaf2655c631353a799/testing/buildbot/test_suites.pyl

Comment 14 by bugdroid1@chromium.org, Aug 11

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2595d82ddfcf666d41d33e1ce881e52401d31454

commit 2595d82ddfcf666d41d33e1ce881e52401d31454
Author: Peter Kasting <pkasting@chromium.org>
Date: Sat Aug 11 02:23:57 2018

Revert "Make //content layer default to --site-per-process mode on desktop."

This reverts commit 3b8db25cc597a642ee4b7cbaf2655c631353a799.

Reason for revert: Checking to see whether this caused https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8938549477343009904/+/steps/content_browsertests_on_Intel_GPU_on_Mac_on_Mac-10.12.6/0/logs/WebRtcAudioDebugRecordingsBrowserTest.CallWithAudioDebugRecordings/0 and similar.

Original change's description:
> Make //content layer default to --site-per-process mode on desktop.
> 
> After this CL site-per-process is the default in all layers on desktop
> platforms, except:
> 
> - Layout Tests which still run with no isolation by default
>   (see https://crbug.com/856734#c5 for the explanation why)
> 
> - //content embedders that don't want to use site-per-process: //chromecast
> 
> - //content embedders that don't yet support site-per-process, but will
>   need to migrate eventually: //headless
> 
> 
> Also note that even after this CL:
> 
> - //content on Android still defaults to no isolation.
>   This is compatible with not_site_per_process_* test steps
>   because such steps are not run on Android bots...
> 
> - //chrome layer ChromeContentBrowserClient controls usage of
>   site-per-process in the Chrome browser and continues to be controlled
>   by a field trial (note that Android is not covered by the field trial
>   or by testing/variations/fieldtrial_testing_config.json).
> 
> 
> Bug: 856734
> Change-Id: Iebd46e5502583f84a4b0a7c2ee8d8b0e081c6ff2
> Reviewed-on: https://chromium-review.googlesource.com/1153981
> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
> Reviewed-by: Stephen Lanham <slan@chromium.org>
> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
> Reviewed-by: Bo <boliu@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#582223}

TBR=thakis@chromium.org,boliu@chromium.org,skyostil@chromium.org,alexmos@chromium.org,slan@chromium.org,lukasza@chromium.org

Change-Id: Ieca3a36422b0f90dccc0040eeece799523c954e0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 856734
Reviewed-on: https://chromium-review.googlesource.com/1171705
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582424}
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/android_webview/browser/aw_content_browser_client.cc
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/android_webview/browser/aw_content_browser_client.h
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/chromecast/browser/cast_content_browser_client.cc
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/chromecast/browser/cast_content_browser_client.h
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/content/browser/geolocation/geolocation_service_impl_unittest.cc
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/content/browser/web_package/signed_exchange_request_handler_browsertest.cc
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/content/public/browser/content_browser_client.h
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/content/shell/browser/layout_test/layout_test_content_browser_client.cc
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/content/shell/browser/layout_test/layout_test_content_browser_client.h
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/headless/lib/browser/headless_content_browser_client.cc
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/headless/lib/browser/headless_content_browser_client.h
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/testing/buildbot/client.v8.chromium.json
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/2595d82ddfcf666d41d33e1ce881e52401d31454/testing/buildbot/test_suites.pyl

Comment 15 by lukasza@chromium.org, Aug 13

Blockedon: 873780

Comment 16 by bugdroid1@chromium.org, Aug 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/120cd4dc009d718f1e14e9409028441b31505641

commit 120cd4dc009d718f1e14e9409028441b31505641
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Aug 14 21:57:33 2018

[reland] Make //content layer default to --site-per-process mode on desktop.

This is a reland of unmodified r582223, after separately taking care of
 https://crbug.com/873780  which caused the earlier revert.  Original CL
description follows below:

After this CL site-per-process is the default in all layers on desktop
platforms, except:

- Layout Tests which still run with no isolation by default
  (see https://crbug.com/856734#c5 for the explanation why)

- //content embedders that don't want to use site-per-process: //chromecast

- //content embedders that don't yet support site-per-process, but will
  need to migrate eventually: //headless

Also note that even after this CL:

- //content on Android still defaults to no isolation.
  This is compatible with not_site_per_process_* test steps
  because such steps are not run on Android bots...

- //chrome layer ChromeContentBrowserClient controls usage of
  site-per-process in the Chrome browser and continues to be controlled
  by a field trial (note that Android is not covered by the field trial
  or by testing/variations/fieldtrial_testing_config.json).

Bug: 856734
Change-Id: I0cfd7e342c44a36d22a885b46892a9e4e3f2c758
Tbr: Stephen Lanham <slan@chromium.org>
Tbr: Sami Kyöstilä <skyostil@chromium.org>
Tbr: Bo <boliu@chromium.org>
Tbr: Nico Weber <thakis@chromium.org>
Tbr: Alex Moshchuk <alexmos@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1174855
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583047}
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/android_webview/browser/aw_content_browser_client.cc
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/android_webview/browser/aw_content_browser_client.h
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/chromecast/browser/cast_content_browser_client.cc
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/chromecast/browser/cast_content_browser_client.h
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/content/browser/geolocation/geolocation_service_impl_unittest.cc
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/content/browser/web_package/signed_exchange_request_handler_browsertest.cc
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/content/public/browser/content_browser_client.h
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/content/shell/browser/layout_test/layout_test_content_browser_client.cc
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/content/shell/browser/layout_test/layout_test_content_browser_client.h
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/headless/lib/browser/headless_content_browser_client.cc
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/headless/lib/browser/headless_content_browser_client.h
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/testing/buildbot/client.v8.chromium.json
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/120cd4dc009d718f1e14e9409028441b31505641/testing/buildbot/test_suites.pyl

Sign in to add a comment