New issue
Advanced search Search tips

Issue 870761 link

Starred by 3 users
Status: Started
Owner:
lukasza@chromium.org
Cc:
creis@chromium.org
dpranke@chromium.org
alex...@chromium.org
thakis@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: 2018-09-11
OS: ----
Pri: 2
Type: Bug

Blocked on: View detail
issue 671734
issue 824962
issue 901502


Sign in to add a comment

Stop running above-//content-layer tests in both no-isolation-mode and site-per-process mode

Project Member Reported by lukasza@chromium.org, Aug 3 
Since site-per-process has shipped in M67 on desktop, going forward it should be okay to just run above-//content-layer tests (e.g. browser_tests, interactive_ui_tests, components_browsertests) in the default mode (which is: site-per-process on desktop, no isolation on Android).

Note that stopping to run not_site_per_process_browser_tests is blocked on making some tests work in site-per-process mode:
- WebNavigationApiTest.ServerRedirectSingleProcess (issue 671734)
- BrowserTest.OtherRedirectsDontForkProcess (issue 824962)

More discussion (including decision to keep coverage both for 1) no isolation and 2) site-per-process at the //content layer and below) can be found here: https://groups.google.com/a/google.com/d/topic/chrome-site-isolation/JNVnjpwx5QQ/discussion

Comment 1 by lukasza@chromium.org, Aug 3

Blockedon: 671734 824962

Comment 2 by lukasza@chromium.org, Aug 3

Cc: dpranke@chromium.org creis@chromium.org thakis@chromium.org alex...@chromium.org
Labels: -Pri-3 Pri-2
Summary: Stop running above-//content-layer tests in both no-isolation-mode and site-per-process mode (was: Stop running above-//content-layer tests in both default-mode and not_site_per_process_* mode)
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 7 

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

commit ba7d457fb37722c7684e6d93d53f2daf81195acc
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Aug 07 00:41:13 2018

Remove not_site_per_process_browser_tests step from the bots.

This CL removes not_site_per_process_browser_tests from the bots,
because:

- Site Isolation has shipped to the stable channel in Chrome 67.
  Currently //chrome layer defaults to site-per-process mode (see
  r552589 and testing/variations/fieldtrial_testing_config.json).

- After recent changes (see https://crbug.com/671734 and
  https://crbug.com/824962) no known tests are skipped when run in
  site-per-process mode.

Bug: 870761
Change-Id: I158e8980846077c2d1ef03bf03f3b80c8191ca81
Reviewed-on: https://chromium-review.googlesource.com/1164172
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581059}
[modify] https://crrev.com/ba7d457fb37722c7684e6d93d53f2daf81195acc/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/ba7d457fb37722c7684e6d93d53f2daf81195acc/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/ba7d457fb37722c7684e6d93d53f2daf81195acc/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/ba7d457fb37722c7684e6d93d53f2daf81195acc/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/ba7d457fb37722c7684e6d93d53f2daf81195acc/testing/buildbot/client.v8.chromium.json
[modify] https://crrev.com/ba7d457fb37722c7684e6d93d53f2daf81195acc/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/ba7d457fb37722c7684e6d93d53f2daf81195acc/testing/buildbot/test_suites.pyl

Comment 5 by lukasza@chromium.org, Aug 14 

Capturing some of the earlier consensus on treatment of //chrome-layer tests:

creis@, alexmos@ and me talked a few weeks ago and

1. We think it is okay to only support site-per-process in //chrome layer on desktop.
   This is because:
    A) site-per-process has effectively shipped in Chrome 67 (it is enabled for 99% of stable users)
    B) relying on //chrome layer for test coverage of lower layers should be rare.
    C) if needed, most (but not all) coverage can be retained by adding a same-site version of a test
    D) if a test passes with site-per-process it will usually pass with no isolation
    E) at //chrome layer all platforms have the defaults that we ship with (desktop: site-per-process, Android: no isolation, iOS: N/A)

2. We do want to maintain support //content embedders both 1) with no isolation and 2) with site-per-process.
   This is because:
    A) Android continues to run with no isolation
    B) Some 1st party embedders continue to run with no isolation - for example: Cast
       (note that //headless is not a good example - we want //headless to eventually run
        with site-per-process so that tests built with //headless test what we ship in Chrome;
        see also the discussion in issue 869494)
    C) Some 3rd party embedders continue to run with no isolation - for example: Electron (?)

It is entirely clear at the moment what (2.) means for test coverage requirements.

Comment 6 by lukasza@chromium.org, Aug 14 

Status update:

- //chrome-and-above tests are now only run in the default mode (i.e. site-per-process on desktop platforms).  See #c3 and #c4 above.

- //content defaults to site-per-process now (on desktop platforms only + some embedders are opted-out).  See https://crrev.com/1174855 (hopefully it sticks this time around).

The latter item above opens a way toward removing more not_site_per_process_... test steps from CQ and waterfall:
- extension_browsertests, extension_unittests, components_browsertests, components_unittests
- content_browsertests, content_unittests


Arguments for only running the test suites above in the default mode:

- not_site_per_process is already effectively covered by embedder-specific bots on CQ:
    - cast_shell_linux (hooked into CastContentBrowserClient::ShouldEnableStrictSiteIsolation I assume):
      components_browser/unittests, content_browsertests/unittests
    - android-kitkat-arm-rel: components_browser/unittests, content_browsertests/unittests
    - linux_chromium_headless_rel: none of the above... :-( (but maybe the coverage above is sufficient)

- If some coverage is missing when running a test only in site-per-process mode,
  then _some_ coverage can be regained by adding a same-origin version of the test

- Tests that explicitly set the isolation mode are needlessly run twice in the same mode
  (e.g. all tests from content/browser/site_per_process_browsertest.cc)

Comment 7 by lukasza@chromium.org, Aug 14

NextAction: 2018-09-11
Let's decide what to do here when everyone gets back from vacation...

Comment 8 by dpranke@chromium.org, Aug 14 

I asked the following question somewhere, but apparently not on this bug: to what extent can we get away with covering the non-isolated code paths by just testing the appropriate platforms (Android, Cast, etc.).

I.e., do we really need to run the tests on platforms that don't use those code paths?

Somewhat related: I suppose I'm fine w/ Electron and/or other embedders not using isolation as long as we're supporting it for other reasons, but I'd be less fine with us supporting it (and continuing to test it) if they were the only reason we'd be keeping it around. I tend to want to try and avoid paying the costs for platforms that aren't ours.
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 30 

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

commit abaa38816647d34ac5ead44ad795d096857cd031
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Aug 30 17:40:19 2018

Stop running not_site_per_process_[content/components/extensions]_...

This CL removes the following test steps:
- not_site_per_process_content_browsertests
- not_site_per_process_content_unittests
- not_site_per_process_components_browsertests
- not_site_per_process_components_unittests
- not_site_per_process_extensions_browsertests
- not_site_per_process_extensions_unittests

These test steps are not needed as explained in
https://crbug.com/870761#c6 (it is sufficient to test with the default
mode: site-per-process on desktop, and not-site-per-process on Android,
Cast).

Bug: 870761
Bug: 852442
Change-Id: I120057abbe85e425fb141d35dcea8045db881234
Reviewed-on: https://chromium-review.googlesource.com/1194335
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587648}
[modify] https://crrev.com/abaa38816647d34ac5ead44ad795d096857cd031/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/abaa38816647d34ac5ead44ad795d096857cd031/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/abaa38816647d34ac5ead44ad795d096857cd031/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/abaa38816647d34ac5ead44ad795d096857cd031/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/abaa38816647d34ac5ead44ad795d096857cd031/testing/buildbot/client.v8.chromium.json
[modify] https://crrev.com/abaa38816647d34ac5ead44ad795d096857cd031/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/abaa38816647d34ac5ead44ad795d096857cd031/testing/buildbot/test_suites.pyl

Comment 10 by monor...@bugs.chromium.org, Sep 11 

The NextAction date has arrived: 2018-09-11

Comment 11 by lukasza@chromium.org, Sep 11

Status: Available (was: Untriaged)
The remaining step here is to figure out what to do with layout tests.  I think that the most promising plan is to:

1. Stop turning off site-per-process for layout tests and instead run with the default mode of a given platform.
   
   We need to retain some coverage of site isolation in WPT and therefore I propose to
   include WPT origins in --isolate-origins param on Linux.
   
2. Retain coverage for tests turned off today by FlagExpectations/site-per-process (there are ~60 entries there) by creating a virtual/not-site-per-process test suite that covers only the affected directories (which cover together ~4400 tests AFAICT):
    external/wpt/content-security-policy/securitypolicyviolation
    external/wpt/dom/events
    external/wpt/FileAPI/url
    external/wpt/html/browsers
    external/wpt/html/infrastructure
    external/wpt/html/syntax
    external/wpt/payment-request/allowpaymentrequest
    external/wpt/wasm/serialization
    fast/css/acid2.html (see  issue 769508  - maybe this can be fixed first)
    http/tests/devtools
    http/tests/images
    http/tests/inspector-protocol
    http/tests/loading
    http/tests/local
    http/tests/media/autoplay
    http/tests/misc
    http/tests/navigation
    http/tests/perf
    http/tests/printing
    http/tests/security/frameNavigation
    http/tests/security/mixedContent
    http/tests/security/upgrade-insecure-requests
    http/tests/text-autosizing
    http/tests/webaudio
    http/tests/xmlhttprequest

Comment 12 by lukasza@chromium.org, Oct 26

Owner: lukasza@chromium.org
Status: Started (was: Available)
WIP CL @ https://crrev.com/c/1302662 tries to tackle layout tests...
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 2 

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

commit b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Nov 02 16:45:06 2018

Make layout tests use default isolation (e.g. site-per-process on desktop).

Summary
=======

This CL makes layout tests use the default site isolation from the
platform they are run on (instead of opting out of strict site isolation
via LayoutTestContentBrowserClient::ShouldEnableStrictSiteIsolation).
Additionally, on platforms where strict site isolation is enabled,
layout tests opt into slightly stricter isolation by enabling isolation
of same-site origins used by Web Platform Tests - this ensures that
features covered by WPT also get sufficient coverage of out-of-process
iframes (OOPIFs).

After this CL, expectations for tests that differ in behavior with and
without OOPIFs are being moved from
LayoutTests/FlagExpectations/site-per-process to:
- LayoutTests/VirtualTestSuites (virtual/not-site-per-process suite)
- LayoutTests/virtual/not-site-per-process/README.md
- LayoutTests/TestExpectations and LayoutTests/NeverFixTests
  ("Site Isolation failures" section)


Desirability
============

The CL helps with the following:

- Focusing on testing the mode that is actually shipped to end users
- Helping ensure that newly developed features get site-per-process
  coverage without having to set up a separate step (i.e. it is
  sufficient to set-up a bot that runs layout tests with
  --enable-features=NewFeature without also having to have a separate
  test step for runing layout tests with *both*
  --enable-features=NewFeature *and* --site-per-process

This CL does *not* help with reducing requirements for CQ capacity,
because we need to maintain a separate
not_site_per_process_webkit_layout_tests step to make sure that tests
pass without isolation (which is the mode Chrome ships on Android).
Also note that layout test coverage on Android is very sparse - see
https://groups.google.com/a/chromium.org/d/topic/blink-dev/SOXhTYysYkE/discussion


Preserving test coverage
========================

Most tests
----------

The CL preserves covering most layout tests with and without OOPIFs, by
relying on the fact that CQ/waterfall run layout tests on both kinds of
platforms - ones that default to strict site isolation (desktop
platforms) and ones that default to no site isolation (Android).


Tests that used to be excluded FlagExpectations/site-per-process
----------------------------------------------------------------

Around 40 tests fail when run in presence of OOPIFs.  Such tests are
disabled by this CL by moving test expectations from
FlagExpectations/site-per-process into the main TestExpectations file.
The CL preserves non-OOPIF test coverage provided by the disabled tests
by introducing virtual/not-site-per-process directory which runs all
such tests with site isolation disabled.  Using a virtual test suites
for preserving the test coverage relies on the ability to have separate
test expectations for these tests (i.e. relying on the fact that
disabling these tests in TestExpectations doesn't disable their
virtual/not-site-per-process equivalents).

Note that the CL keeps isolating "oopif.test" site even in
virtual/not-site-per-process suite.  This site should only be used by
tests that require an OOPIF.


Preserving site-per-process-specific test expectations
------------------------------------------------------

Some tests have site-per-process-specific expectations:
- http/tests/inspector-protocol/network/security-info-on-response.js
- http/tests/inspector-protocol/network/raw-headers-for-protected-document.js

The tests above highlight that cross-origin cookies are not displayed in
site-per-process mode (a known regression tracked by
https://crbug.com/849483).  This CL preserves expectations and coverage
by shuffling things around:
- old, main expectation -> android expectation
- old, site-per-process expectation -> main expectation


There is one additional test with site-per-process-specific
expectations:
- external/wpt/dom/events/EventListener-addEventListener.sub.window.js

Unlike the other 2 tests, it seems less important to preserve exact test
expectations for the case when the test fails with Site Isolation.
Therefore this test is covered by virtual/not-site-per-process test
suite and an expectation for this test is added to the main
TestExpectations.


Lost test coverage
------------------

Even with extra caution described above, some test coverage may be lost:

- Features covered by tests only on one kind of platform (e.g. disabled
  on Android) are at risk of losing OOPIF or non-OOPIF coverage.

- Before this CL, site-per-process was also applied to all other
  `virtual/...` test suites.  After this CL,
  `virtual/not-site-per-process` will not provide such coverage.


Cleaned up test expectations
----------------------------

Some additional test expectations clean-up is done, while preserving
test coverage:

- The http/tests/perf/large-inlined-script.html test has been already
  present in `SlowTests` and therefore I didn't include this test in the
  new `virtual/not-site-per-process` suite.

- The http/tests/devtools/network/network-datareceived.js test was
  already marked as expecting a `[Failure]` in TestExpectations and
  therefore I didn't include this test in the new
  `virtual/not-site-per-process` suite.

- The http/tests/devtools/console-cross-origin-iframe-logging.js test
  was already marked as `[Timeout]` in the old TestExpectations but for
  Win only.  Since I can repro a timeout on Linux (with and without site
  isolation), I just extended the old expectation to all platforms and I
  didn't include this test in the new `virtual/not-site-per-process`
  suite.  I also removed the test from SlowTests (since timeouts in the
  test are not expected everywhere).


Bug: 870761
Change-Id: I74d8ac4ebee8f0402d449fda2cec46ba3b49cf64
Reviewed-on: https://chromium-review.googlesource.com/c/1302662
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604955}
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/content/shell/browser/layout_test/layout_test_content_browser_client.cc
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/content/shell/browser/layout_test/layout_test_content_browser_client.h
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/testing/buildbot/test_suites.pyl
[delete] https://crrev.com/bc16ed3dfbd530ea0b1bae272e7dfd9606625a99/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/NeverFixTests
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/SlowTests
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/VirtualTestSuites
[rename] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/flag-specific/disable-site-isolation-trials/http/tests/inspector-protocol/network/raw-headers-for-protected-document-expected.txt
[rename] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/flag-specific/disable-site-isolation-trials/http/tests/inspector-protocol/network/security-info-on-response-expected.txt
[delete] https://crrev.com/bc16ed3dfbd530ea0b1bae272e7dfd9606625a99/third_party/WebKit/LayoutTests/flag-specific/site-per-process/external/wpt/dom/events/EventListener-addEventListener.sub.window-expected.txt
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/raw-headers-for-protected-document-expected.txt
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/security-info-on-response-expected.txt
[add] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/WebKit/LayoutTests/virtual/not-site-per-process/README.md
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/blink/tools/blinkpy/third_party/wpt/README.chromium
[modify] https://crrev.com/b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea/third_party/blink/tools/blinkpy/web_tests/lint_test_expectations.py
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 2 

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

commit 80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16
Author: Mike Wittman <wittman@chromium.org>
Date: Fri Nov 02 20:32:53 2018

Revert "Make layout tests use default isolation (e.g. site-per-process on desktop)."

This reverts commit b2457b268d03aa4a6f91bbe4d4d2d46255e6eaea.

Reason for revert: suspected of causing assertion failure running merge_web_test_results.py in not_site_per_process_webkit_layout_tests

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29%2832%29/53956

2018-11-02 10:44:57,220 - root: [INFO] Running with isolated arguments
Traceback (most recent call last):
  File "/b/swarming/w/ir/cache/builder/src/third_party/blink/tools/merge_web_test_results.py", line 12, in <module>
    main(sys.argv[1:])
  File "/b/swarming/w/ir/cache/builder/src/third_party/blink/tools/blinkpy/web_tests/merge_results.py", line 775, in main
    assert args.positional
AssertionError


Original change's description:
> Make layout tests use default isolation (e.g. site-per-process on desktop).
> 
> Summary
> =======
> 
> This CL makes layout tests use the default site isolation from the
> platform they are run on (instead of opting out of strict site isolation
> via LayoutTestContentBrowserClient::ShouldEnableStrictSiteIsolation).
> Additionally, on platforms where strict site isolation is enabled,
> layout tests opt into slightly stricter isolation by enabling isolation
> of same-site origins used by Web Platform Tests - this ensures that
> features covered by WPT also get sufficient coverage of out-of-process
> iframes (OOPIFs).
> 
> After this CL, expectations for tests that differ in behavior with and
> without OOPIFs are being moved from
> LayoutTests/FlagExpectations/site-per-process to:
> - LayoutTests/VirtualTestSuites (virtual/not-site-per-process suite)
> - LayoutTests/virtual/not-site-per-process/README.md
> - LayoutTests/TestExpectations and LayoutTests/NeverFixTests
>   ("Site Isolation failures" section)
> 
> 
> Desirability
> ============
> 
> The CL helps with the following:
> 
> - Focusing on testing the mode that is actually shipped to end users
> - Helping ensure that newly developed features get site-per-process
>   coverage without having to set up a separate step (i.e. it is
>   sufficient to set-up a bot that runs layout tests with
>   --enable-features=NewFeature without also having to have a separate
>   test step for runing layout tests with *both*
>   --enable-features=NewFeature *and* --site-per-process
> 
> This CL does *not* help with reducing requirements for CQ capacity,
> because we need to maintain a separate
> not_site_per_process_webkit_layout_tests step to make sure that tests
> pass without isolation (which is the mode Chrome ships on Android).
> Also note that layout test coverage on Android is very sparse - see
> https://groups.google.com/a/chromium.org/d/topic/blink-dev/SOXhTYysYkE/discussion
> 
> 
> Preserving test coverage
> ========================
> 
> Most tests
> ----------
> 
> The CL preserves covering most layout tests with and without OOPIFs, by
> relying on the fact that CQ/waterfall run layout tests on both kinds of
> platforms - ones that default to strict site isolation (desktop
> platforms) and ones that default to no site isolation (Android).
> 
> 
> Tests that used to be excluded FlagExpectations/site-per-process
> ----------------------------------------------------------------
> 
> Around 40 tests fail when run in presence of OOPIFs.  Such tests are
> disabled by this CL by moving test expectations from
> FlagExpectations/site-per-process into the main TestExpectations file.
> The CL preserves non-OOPIF test coverage provided by the disabled tests
> by introducing virtual/not-site-per-process directory which runs all
> such tests with site isolation disabled.  Using a virtual test suites
> for preserving the test coverage relies on the ability to have separate
> test expectations for these tests (i.e. relying on the fact that
> disabling these tests in TestExpectations doesn't disable their
> virtual/not-site-per-process equivalents).
> 
> Note that the CL keeps isolating "oopif.test" site even in
> virtual/not-site-per-process suite.  This site should only be used by
> tests that require an OOPIF.
> 
> 
> Preserving site-per-process-specific test expectations
> ------------------------------------------------------
> 
> Some tests have site-per-process-specific expectations:
> - http/tests/inspector-protocol/network/security-info-on-response.js
> - http/tests/inspector-protocol/network/raw-headers-for-protected-document.js
> 
> The tests above highlight that cross-origin cookies are not displayed in
> site-per-process mode (a known regression tracked by
> https://crbug.com/849483).  This CL preserves expectations and coverage
> by shuffling things around:
> - old, main expectation -> android expectation
> - old, site-per-process expectation -> main expectation
> 
> 
> There is one additional test with site-per-process-specific
> expectations:
> - external/wpt/dom/events/EventListener-addEventListener.sub.window.js
> 
> Unlike the other 2 tests, it seems less important to preserve exact test
> expectations for the case when the test fails with Site Isolation.
> Therefore this test is covered by virtual/not-site-per-process test
> suite and an expectation for this test is added to the main
> TestExpectations.
> 
> 
> Lost test coverage
> ------------------
> 
> Even with extra caution described above, some test coverage may be lost:
> 
> - Features covered by tests only on one kind of platform (e.g. disabled
>   on Android) are at risk of losing OOPIF or non-OOPIF coverage.
> 
> - Before this CL, site-per-process was also applied to all other
>   `virtual/...` test suites.  After this CL,
>   `virtual/not-site-per-process` will not provide such coverage.
> 
> 
> Cleaned up test expectations
> ----------------------------
> 
> Some additional test expectations clean-up is done, while preserving
> test coverage:
> 
> - The http/tests/perf/large-inlined-script.html test has been already
>   present in `SlowTests` and therefore I didn't include this test in the
>   new `virtual/not-site-per-process` suite.
> 
> - The http/tests/devtools/network/network-datareceived.js test was
>   already marked as expecting a `[Failure]` in TestExpectations and
>   therefore I didn't include this test in the new
>   `virtual/not-site-per-process` suite.
> 
> - The http/tests/devtools/console-cross-origin-iframe-logging.js test
>   was already marked as `[Timeout]` in the old TestExpectations but for
>   Win only.  Since I can repro a timeout on Linux (with and without site
>   isolation), I just extended the old expectation to all platforms and I
>   didn't include this test in the new `virtual/not-site-per-process`
>   suite.  I also removed the test from SlowTests (since timeouts in the
>   test are not expected everywhere).
> 
> 
> Bug: 870761
> Change-Id: I74d8ac4ebee8f0402d449fda2cec46ba3b49cf64
> Reviewed-on: https://chromium-review.googlesource.com/c/1302662
> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#604955}

TBR=dpranke@chromium.org,alexmos@chromium.org,lukasza@chromium.org

Change-Id: I54b6a36922bebfbc8a3f1467bc3d4807c2322604
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 870761
Reviewed-on: https://chromium-review.googlesource.com/c/1315960
Reviewed-by: Mike Wittman <wittman@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605039}
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/content/shell/browser/layout_test/layout_test_content_browser_client.cc
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/content/shell/browser/layout_test/layout_test_content_browser_client.h
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/testing/buildbot/test_suites.pyl
[add] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/NeverFixTests
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/SlowTests
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/VirtualTestSuites
[add] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/flag-specific/site-per-process/external/wpt/dom/events/EventListener-addEventListener.sub.window-expected.txt
[rename] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/flag-specific/site-per-process/http/tests/inspector-protocol/network/raw-headers-for-protected-document-expected.txt
[rename] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/flag-specific/site-per-process/http/tests/inspector-protocol/network/security-info-on-response-expected.txt
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/raw-headers-for-protected-document-expected.txt
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/security-info-on-response-expected.txt
[delete] https://crrev.com/3d990748160be9a0e551f73febac3f02eb9a124d/third_party/WebKit/LayoutTests/virtual/not-site-per-process/README.md
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/blink/tools/blinkpy/third_party/wpt/README.chromium
[modify] https://crrev.com/80c67924a48d5d2bf6ef01ff09f3eb35b29e9f16/third_party/blink/tools/blinkpy/web_tests/lint_test_expectations.py

Comment 15 by lukasza@chromium.org, Nov 5

Blockedon: 901502
Project Member

Comment 16 by bugdroid1@chromium.org, Nov 19 

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

commit 04bc630f5cdbf0ff08a98bdccd04cbe46774cff7
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Nov 19 19:04:28 2018

[reland] Make layout tests use default isolation (e.g. site-per-process)

This CL is 1) a reland of r604955 + 2) fixes.  Original CL description
follows below.

Summary
=======

This CL makes layout tests use the default site isolation from the
platform they are run on (instead of opting out of strict site isolation
via LayoutTestContentBrowserClient::ShouldEnableStrictSiteIsolation).
Additionally, on platforms where strict site isolation is enabled,
layout tests opt into slightly stricter isolation by enabling isolation
of same-site origins used by Web Platform Tests - this ensures that
features covered by WPT also get sufficient coverage of out-of-process
iframes (OOPIFs).

After this CL, expectations for tests that differ in behavior with and
without OOPIFs are being moved from
LayoutTests/FlagExpectations/site-per-process to:
- LayoutTests/VirtualTestSuites (virtual/not-site-per-process suite)
- LayoutTests/virtual/not-site-per-process/README.md
- LayoutTests/TestExpectations and LayoutTests/NeverFixTests
  ("Site Isolation failures" section)


Desirability
============

The CL helps with the following:

- Focusing on testing the mode that is actually shipped to end users
- Helping ensure that newly developed features get site-per-process
  coverage without having to set up a separate step (i.e. it is
  sufficient to set-up a bot that runs layout tests with
  --enable-features=NewFeature without also having to have a separate
  test step for runing layout tests with *both*
  --enable-features=NewFeature *and* --site-per-process

This CL does *not* help with reducing requirements for CQ capacity,
because we need to maintain a separate
not_site_per_process_webkit_layout_tests step to make sure that tests
pass without isolation (which is the mode Chrome ships on Android).
Also note that layout test coverage on Android is very sparse - see
https://groups.google.com/a/chromium.org/d/topic/blink-dev/SOXhTYysYkE/discussion


Preserving test coverage
========================

Most tests
----------

The CL preserves covering most layout tests with and without OOPIFs, by
relying on the fact that CQ/waterfall run layout tests on both kinds of
platforms - ones that default to strict site isolation (desktop
platforms) and ones that default to no site isolation (Android).


Tests that used to be excluded FlagExpectations/site-per-process
----------------------------------------------------------------

Around 40 tests fail when run in presence of OOPIFs.  Such tests are
disabled by this CL by moving test expectations from
FlagExpectations/site-per-process into the main TestExpectations file.
The CL preserves non-OOPIF test coverage provided by the disabled tests
by introducing virtual/not-site-per-process directory which runs all
such tests with site isolation disabled.  Using a virtual test suites
for preserving the test coverage relies on the ability to have separate
test expectations for these tests (i.e. relying on the fact that
disabling these tests in TestExpectations doesn't disable their
virtual/not-site-per-process equivalents).

Note that the CL keeps isolating "oopif.test" site even in
virtual/not-site-per-process suite.  This site should only be used by
tests that require an OOPIF.


Preserving site-per-process-specific test expectations
------------------------------------------------------

Some tests have site-per-process-specific expectations:
- http/tests/inspector-protocol/network/security-info-on-response.js
- http/tests/inspector-protocol/network/raw-headers-for-protected-document.js

The tests above highlight that cross-origin cookies are not displayed in
site-per-process mode (a known regression tracked by
https://crbug.com/849483).  This CL preserves expectations and coverage
by shuffling things around:
- old, main expectation -> android expectation
- old, site-per-process expectation -> main expectation


There is one additional test with site-per-process-specific
expectations:
- external/wpt/dom/events/EventListener-addEventListener.sub.window.js

Unlike the other 2 tests, it seems less important to preserve exact test
expectations for the case when the test fails with Site Isolation.
Therefore this test is covered by virtual/not-site-per-process test
suite and an expectation for this test is added to the main
TestExpectations.


Lost test coverage
------------------

Even with extra caution described above, some test coverage may be lost:

- Features covered by tests only on one kind of platform (e.g. disabled
  on Android) are at risk of losing OOPIF or non-OOPIF coverage.

- Before this CL, site-per-process was also applied to all other
  `virtual/...` test suites.  After this CL,
  `virtual/not-site-per-process` will not provide such coverage.


Cleaned up test expectations
----------------------------

Some additional test expectations clean-up is done, while preserving
test coverage:

- The http/tests/perf/large-inlined-script.html test has been already
  present in `SlowTests` and therefore I didn't include this test in the
  new `virtual/not-site-per-process` suite.

- The http/tests/devtools/network/network-datareceived.js test was
  already marked as expecting a `[Failure]` in TestExpectations and
  therefore I didn't include this test in the new
  `virtual/not-site-per-process` suite.

- The http/tests/devtools/console-cross-origin-iframe-logging.js test
  was already marked as `[Timeout]` in the old TestExpectations but for
  Win only.  Since I can repro a timeout on Linux (with and without site
  isolation), I just extended the old expectation to all platforms and I
  didn't include this test in the new `virtual/not-site-per-process`
  suite.  I also removed the test from SlowTests (since timeouts in the
  test are not expected everywhere).


Bug: 870761, 477150
Tbr: alexmos@chromium.org
Change-Id: If3c6fbc58ccdac8bc64a81963238db13c0bff391
Reviewed-on: https://chromium-review.googlesource.com/c/1318750
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609379}
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/content/shell/browser/layout_test/layout_test_content_browser_client.cc
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/content/shell/browser/layout_test/layout_test_content_browser_client.h
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/testing/buildbot/test_suites.pyl
[delete] https://crrev.com/8bb6cda9777051d5d84d827f8d8e63894ab3a59c/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/NeverFixTests
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/SlowTests
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/VirtualTestSuites
[rename] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/flag-specific/disable-site-isolation-trials/http/tests/inspector-protocol/network/raw-headers-for-protected-document-expected.txt
[rename] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/flag-specific/disable-site-isolation-trials/http/tests/inspector-protocol/network/security-info-on-response-expected.txt
[delete] https://crrev.com/8bb6cda9777051d5d84d827f8d8e63894ab3a59c/third_party/WebKit/LayoutTests/flag-specific/site-per-process/external/wpt/dom/events/EventListener-addEventListener.sub.window-expected.txt
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/raw-headers-for-protected-document-expected.txt
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/security-info-on-response-expected.txt
[add] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/WebKit/LayoutTests/virtual/not-site-per-process/README.md
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/blink/tools/blinkpy/common/net/buildbot_unittest.py
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/blink/tools/blinkpy/third_party/wpt/README.chromium
[modify] https://crrev.com/04bc630f5cdbf0ff08a98bdccd04cbe46774cff7/third_party/blink/tools/blinkpy/web_tests/lint_test_expectations.py
►

Sign in to add a comment