New issue
Advanced search Search tips

Issue 824966 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 827321

Blocking:
issue 810843



Sign in to add a comment

Make --site-per-process the default for //chrome layer on ToT

Project Member Reported by lukasza@chromium.org, Mar 22 2018

Issue description

Before shipping Strict Site Isolation, we should make --site-per-process the default for //chrome layer on ToT.  Let's use this bug to track preparatory work and the eventual switch (via testing/variations/fieldtrial_testing_config.json).
 
Labels: Proj-SiteIsolation-LaunchBlocking
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 24 2018

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

commit 5e201abdaf41bb510b8033b390981866c2104170
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Sat Mar 24 00:41:06 2018

Move features::kSitePerProcess from //content to //chrome layer.

The move helps ensure that --site-per-process stays turned off by
default in the //content layer (this is needed to avoid opting
into --site-per-process all //content embedders like ChromeCast).
The move enables selectively enabling --site-per-process at the
//chrome layer.

Bug:  824966 
Change-Id: Icc3354dfeb2ff20c6027267890596b929331c1ed
Reviewed-on: https://chromium-review.googlesource.com/976880
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545649}
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/chrome/browser/subresource_filter/subresource_filter_devtools_browsertest.cc
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/chrome/common/chrome_features.cc
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/chrome/common/chrome_features.h
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/components/printing/browser/print_manager_utils.cc
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/content/browser/site_isolation_policy.cc
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/content/public/browser/content_browser_client.h
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/content/public/common/content_features.cc
[modify] https://crrev.com/5e201abdaf41bb510b8033b390981866c2104170/content/public/common/content_features.h

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 27 2018

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

commit c1c039e493c3a2bbde717f71273a9fba792ab498
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Mar 27 16:26:47 2018

Inline site-per-process expectations for //chrome-layer tests.

We plan to make --site-per-process mode the default mode on ToT for
//chrome layer.  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: 671734, 824962,  824966 
Change-Id: I4ce397f5a7be18cc184f081058bf57f2e7c6239f
Reviewed-on: https://chromium-review.googlesource.com/976643
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546128}
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/chrome/browser/ui/browser_browsertest.cc
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/chrome/test/BUILD.gn
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/testing/buildbot/PRESUBMIT.py
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/testing/buildbot/client.v8.chromium.json
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/testing/buildbot/filters/BUILD.gn
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/testing/buildbot/filters/README.md
[delete] https://crrev.com/36cb2980b99b9302adf4e681511431b17f335693/testing/buildbot/filters/site-per-process.browser_tests.filter
[delete] https://crrev.com/36cb2980b99b9302adf4e681511431b17f335693/testing/buildbot/filters/site-per-process.interactive_ui_tests.filter
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/c1c039e493c3a2bbde717f71273a9fba792ab498/testing/buildbot/test_suites.pyl

Blockedon: 827321
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 6 2018

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

commit bf6264a5041160a66a1739a275007f35ea36b0b9
Author: Nick Carter <nick@chromium.org>
Date: Fri Apr 06 02:39:33 2018

Allow extension process reuse in --site-per-process;
make SiteIsolationPolicy public.

The problem was that --site-per-process disabled extension
process sharing, but the site-per-process base::Feature (which
we've been field trialing) did not. This was due to the
extensions code checking only for the flag, and not considering
the field trial state as well.

components/printing actually got the logic right, but only by
reproducing a lot of business logic. Thus, it seems
appropriate to move SiteIsolationPolicy to content/public,
so that we can centralize the "what kind of oopifs are there"
logic. For printing, this change adds a new getter function
specific to oopif compositor, since that's basically a
derived policy of the process model.

For extensions, we've decided to disable LockToOrigin in
--site-per-process (rather than to enable it in the feature),
since origin-locking extensions doesn't help with the spectre
threat, and --site-per-process is about spectre these days.
[Charlie suggests we develop some kind of "extension isolation v2"
proposal, maybe reviving the --isolate-extension flag for that
purpose!]

Bug:  824966 , 766267

Change-Id: Ibf7592c9d522fd0c99057358bcc34b5881780db8
Reviewed-on: https://chromium-review.googlesource.com/949966
Commit-Queue: Nick Carter <nick@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Wei Li <weili@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548645}
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/components/printing/browser/print_manager_utils.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/BUILD.gn
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/browser_main_loop.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/browsing_instance.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/site_instance_impl.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/BUILD.gn
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/content_browser_client.h
[rename] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/site_isolation_policy.cc
[rename] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/site_isolation_policy.h
[rename] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/site_isolation_policy_unittest.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/test/test_utils.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/test/BUILD.gn

Status: Started (was: Assigned)
WIP CL @ https://crrev.com/c/981019
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 21 2018

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

commit fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Sat Apr 21 14:55:20 2018

Make --site-per-process the default on ToT via fieldtrial_testing_config

Note that features::kSitePerProcess is a //chrome-layer feature.
Therefore, after this CL:
- site-per-process is turned-on by default in //chrome layer and above
  (i.e. in code that uses ChromeContentBrowserClient).
- site-per-process stays off by default in //content layer

Also note that fieldtrial_testing_config.json only affects how default
Chromium tests are run, but doesn't affect the default behavior of
Chrome builds shipped to end-users - these will continue to have
site-per-process off by default (and controlled via field trials).

In addition to making site-per-process the default, the CL makes the
following supplementary changes:
- site_per_process_browser_tests test step is removed and replaced
  by not_site_per_process_browser_tests (similarily for a few other
  //chrome-layer test suites where I've verified that
  content::AreAllSitesIsolatedForTesting returns true after this CL).
- A few last-minute changes to disable tests that fail with
  --site-per-process - these changes are limited to site-per-process
  and/or specific platforms.

Bug:  824966 , 831078,  833423 , 833429, 833430
Change-Id: Ie87e48658864203d33d1ae647fb2d98a7c488765
Tbr: zhenw@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/981019
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552589}
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/chrome/browser/extensions/extension_crash_recovery_browsertest.cc
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/chrome/browser/extensions/extension_messages_apitest.cc
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/chrome/browser/policy/site_isolation_policy_browsertest.cc
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe_browsertest.cc
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/content/public/browser/site_isolation_policy.cc
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/testing/buildbot/client.v8.chromium.json
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/testing/buildbot/test_suites.pyl
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/testing/buildbot/waterfalls.pyl
[modify] https://crrev.com/fb1ccf02ee8ca79e1404abfd3a3a7d540b7d2dbd/testing/variations/fieldtrial_testing_config.json

Status: Fixed (was: Started)

Sign in to add a comment