Make site-per-process the default at the //content layer |
||||
Issue descriptionWe'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).
,
Jun 28 2018
There's a parallel discussion at https://groups.google.com/a/chromium.org/forum/#!topic/site-isolation-dev/25pO_47w9R0
,
Jul 16
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.
,
Jul 16
,
Jul 26
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...
,
Jul 30
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.
,
Jul 30
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?
,
Jul 30
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.
,
Jul 30
I can see that conclusion in the thread, but not why it'd be justified :).
,
Jul 31
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
,
Jul 31
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?
,
Jul 31
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.
,
Aug 10
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
,
Aug 11
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
,
Aug 13
,
Aug 14
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 |
||||
Comment 1 by creis@chromium.org
, Jun 27 2018