New issue
Advanced search Search tips

Issue 874998 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocking:
issue 739418



Sign in to add a comment

Post launch tweaks of site-per-process features/switches/defaults

Project Member Reported by lukasza@chromium.org, Aug 16

Issue description

Site Isolation has launched to the stable channel with M67.  We still want to retain to control site-per-process via field trials (e.g. to facilitate field trials on Android), but we should ensure that

1) site-per-process is the default on desktop even in absence of field trials

2) current field trials have a max_version based on the CL doing #1

3) field trials work not only for code based on ChromeContentBrowserClient, but for all //content embedders (e.g. including AwContentBrowserClient)
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 18

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

commit f6a6fa6c2133597586b8e698fe771d71d44a2f33
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Sat Aug 18 00:12:37 2018

site-per-process is the default on desktop even without of field trials.

This CL flips the default value of features::kSitePerProcess on desktop
to base::FEATURE_ENABLED_BY_DEFAULT.

Bug: 874998
Change-Id: Ia1a999072e87cc32fbde1bb70f8bb39e9b3053a7
Reviewed-on: https://chromium-review.googlesource.com/1178622
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584254}
[modify] https://crrev.com/f6a6fa6c2133597586b8e698fe771d71d44a2f33/chrome/common/chrome_features.cc
[modify] https://crrev.com/f6a6fa6c2133597586b8e698fe771d71d44a2f33/testing/variations/fieldtrial_testing_config.json

Commit f6a6fa6c... initially landed in 70.0.3526.0
rnk@ has reported (thanks!) that r584254 from #c1 broke tests on official builds.  I plan to keep r584254 on trunk and fix this forward - this is most likely just a matter of getting rid of an official-build exception here:

    class WebDriverSitePerProcessPolicyBrowserTest
    ...
      void SetUpInProcessBrowserTestFixture() override {
        // First take note if tests are running in site isolated environment as this
        // will change the outcome of the test. We can't just call this method after
        // the call to the base setup method because setting the Site Isolation
        // policy is indistinguishable from setting the the command line flag
        // directly.
    #if defined(OFFICIAL_BUILD)
        // Official builds still default to no site isolation (i.e. official builds
        // are not covered by testing/variations/fieldtrial_testing_config.json).
        // See also  https://crbug.com/836261 .
        are_sites_isolated_for_testing_ = false;
    #else
        // Otherwise, site-per-process is turned on by default, via field trial
        // configured with testing/variations/fieldtrial_testing_config.json.
        // The only exception is the not_site_per_process_browser_tests step run on
        // some trybots - in this step the --disable-site-isolation-trials flag
        // counteracts the effects of fieldtrial_testing_config.json.
        are_sites_isolated_for_testing_ =
            !base::CommandLine::ForCurrentProcess()->HasSwitch(
                switches::kDisableSiteIsolationTrials);
    #endif
Cc: r...@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 20

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

commit 35264e311018e40cd631483b8d5477f9224bcb9c
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Aug 20 17:54:31 2018

Adjust test expectations - site-per-process is no longer differs on official bots

r584254 removed differences between waterfall/cq and official bots
(by removing testing/variations/fieldtrial_testing_config.json entries
related to site-per-process and making site-per-process the default
even in absence of field trials).  This makes it possible to remove
the |#if defined(OFFICIAL_BUILD)|-related expectations from
WebDriverSitePerProcessPolicyBrowserTest.

r581059 removed the not_site_per_process_browser_tests step from the bots.
This makes it possible to remove the kDisableSiteIsolationTrials-related
expectations from WebDriverSitePerProcessPolicyBrowserTest

Bug: 874998
Tbr: pastarmovj@chromium.org
Change-Id: I87988884dfe945f16e10680637b26fb4efcf3f2c
Reviewed-on: https://chromium-review.googlesource.com/1181526
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584494}
[modify] https://crrev.com/35264e311018e40cd631483b8d5477f9224bcb9c/chrome/browser/policy/site_isolation_policy_browsertest.cc

Cc: lukasza@chromium.org
Owner: ----
Status: Available (was: Started)
I am not actively working on this, but let's keep this bug open - removing chrome://flags/#enable-site-per-process on desktop is another thing that might fall under this bug's umbrella.
Cc: creis@chromium.org
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
#6: see also  issue 879633 , where I'm landing an update to the description of chrome://flags/#enable-site-per-process, and which discusses eventually removing the flag on desktop, depending on how that impacts embedders.

Sign in to add a comment