+Alexei as an FYI

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

commit 9c04ed553bd7abe820a6a93c5e8981e6738881a9
Author: chaopeng <chaopeng@chromium.org>
Date: Sat Apr 29 02:07:43 2017

Change ScopedFeatureList to overrides FeatureList not reset

The current situation is that using ScopedFeatureList resets to an
empty feature list and then enables/disables an explicit list of
features.

That's never what you want for browser tests (or other higher-level
tests) since it effectively overrides higher-level test configurations
(e.g. those in fieldtrial_testing_config.json, or a bot set up to
specifically test a feature).

In this patch:

1. Keep SFL::Init, SFL::InitWithFeatureList,
   SFL::InitFromCommandLine reset to empty list but add warning to
   remind developer should use them with care.
2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
   SFL::InitWithFeatures to not reset but override current FeatureList
   with given enables/disables.

We also add unit tests for ScopedFeatureList.

BUG= 713390 

Review-Url: https://codereview.chromium.org/2834583002
Cr-Commit-Position: refs/heads/master@{#468210}

[modify] https://crrev.com/9c04ed553bd7abe820a6a93c5e8981e6738881a9/base/BUILD.gn
[modify] https://crrev.com/9c04ed553bd7abe820a6a93c5e8981e6738881a9/base/test/scoped_feature_list.cc
[modify] https://crrev.com/9c04ed553bd7abe820a6a93c5e8981e6738881a9/base/test/scoped_feature_list.h
[add] https://crrev.com/9c04ed553bd7abe820a6a93c5e8981e6738881a9/base/test/scoped_feature_list_unittest.cc

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

commit 6c14f269fb196627c72a810205488d694b63a7d5
Author: thestig <thestig@chromium.org>
Date: Mon May 01 00:00:07 2017

Revert of Change ScopedFeatureList to overrides FeatureList not reset (patchset #6 id:140001 of https://codereview.chromium.org/2834583002/ )

Reason for revert:
Mac ASAN bots reporting use-after-free errors.

Original issue's description:
> Change ScopedFeatureList to overrides FeatureList not reset
>
> The current situation is that using ScopedFeatureList resets to an
> empty feature list and then enables/disables an explicit list of
> features.
>
> That's never what you want for browser tests (or other higher-level
> tests) since it effectively overrides higher-level test configurations
> (e.g. those in fieldtrial_testing_config.json, or a bot set up to
> specifically test a feature).
>
> In this patch:
>
> 1. Keep SFL::Init, SFL::InitWithFeatureList,
>    SFL::InitFromCommandLine reset to empty list but add warning to
>    remind developer should use them with care.
> 2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
>    SFL::InitWithFeatures to not reset but override current FeatureList
>    with given enables/disables.
>
> We also add unit tests for ScopedFeatureList.
>
> BUG= 713390 
>
> Review-Url: https://codereview.chromium.org/2834583002
> Cr-Commit-Position: refs/heads/master@{#468210}
> Committed: https://chromium.googlesource.com/chromium/src/+/9c04ed553bd7abe820a6a93c5e8981e6738881a9

TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,chaopeng@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 713390 

Review-Url: https://codereview.chromium.org/2850073002
Cr-Commit-Position: refs/heads/master@{#468263}

[modify] https://crrev.com/6c14f269fb196627c72a810205488d694b63a7d5/base/BUILD.gn
[modify] https://crrev.com/6c14f269fb196627c72a810205488d694b63a7d5/base/test/scoped_feature_list.cc
[modify] https://crrev.com/6c14f269fb196627c72a810205488d694b63a7d5/base/test/scoped_feature_list.h
[delete] https://crrev.com/1d2108cce478cac9751c40efc18e06d12a7a1f2b/base/test/scoped_feature_list_unittest.cc

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

commit 648090c24cfe95b072fe2099ffd4af9bd04aa724
Author: chaopeng <chaopeng@chromium.org>
Date: Wed May 10 20:27:04 2017

Fix SetUp of HostedAppTest and AppShimMenuControllerBrowserTest

We got a ASAN issue when we land crrev.com/2834583002. The ASAN issue shows
ScopedFeatureList::* calls happend after Shutdown/Test finish. In the stack
trace we also know, the test is run in InProcessBrowserTest::SetUp so all the
SetUp stuff should happen before SuperClass::SetUp.

In this patch, we only fix SetUp of HostedAppTest and
AppShimMenuControllerBrowserTest.

BUG= 713390 

Review-Url: https://codereview.chromium.org/2874693002
Cr-Commit-Position: refs/heads/master@{#470685}

[modify] https://crrev.com/648090c24cfe95b072fe2099ffd4af9bd04aa724/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm
[modify] https://crrev.com/648090c24cfe95b072fe2099ffd4af9bd04aa724/chrome/browser/ui/extensions/hosted_app_browsertest.cc

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

commit 04afdfd65fe04ef71fa9918d8fea0f2bc6356836
Author: chaopeng <chaopeng@chromium.org>
Date: Fri Jun 30 04:47:19 2017

Reland of Change ScopedFeatureList to overrides FeatureList not reset (patchset #1 id:1 of https://codereview.chromium.org/2850073002/ )

Reason for revert:
The ASAN failure is fixed at https://codereview.chromium.org/2874693002/.

This patch is ready for reland.

Original issue's description:
> Revert of Change ScopedFeatureList to overrides FeatureList not reset (patchset #6 id:140001 of https://codereview.chromium.org/2834583002/ )
>
> Reason for revert:
> Mac ASAN bots reporting use-after-free errors.
>
> Original issue's description:
> > Change ScopedFeatureList to overrides FeatureList not reset
> >
> > The current situation is that using ScopedFeatureList resets to an
> > empty feature list and then enables/disables an explicit list of
> > features.
> >
> > That's never what you want for browser tests (or other higher-level
> > tests) since it effectively overrides higher-level test configurations
> > (e.g. those in fieldtrial_testing_config.json, or a bot set up to
> > specifically test a feature).
> >
> > In this patch:
> >
> > 1. Keep SFL::Init, SFL::InitWithFeatureList,
> >    SFL::InitFromCommandLine reset to empty list but add warning to
> >    remind developer should use them with care.
> > 2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
> >    SFL::InitWithFeatures to not reset but override current FeatureList
> >    with given enables/disables.
> >
> > We also add unit tests for ScopedFeatureList.
> >
> > BUG= 713390 
> >
> > Review-Url: https://codereview.chromium.org/2834583002
> > Cr-Commit-Position: refs/heads/master@{#468210}
> > Committed: https://chromium.googlesource.com/chromium/src/+/9c04ed553bd7abe820a6a93c5e8981e6738881a9
>
> TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,chaopeng@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 713390 
>
> Review-Url: https://codereview.chromium.org/2850073002
> Cr-Commit-Position: refs/heads/master@{#468263}
> Committed: https://chromium.googlesource.com/chromium/src/+/6c14f269fb196627c72a810205488d694b63a7d5

TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,thestig@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 713390 

Review-Url: https://codereview.chromium.org/2962963002
Cr-Commit-Position: refs/heads/master@{#483631}

[modify] https://crrev.com/04afdfd65fe04ef71fa9918d8fea0f2bc6356836/base/BUILD.gn
[modify] https://crrev.com/04afdfd65fe04ef71fa9918d8fea0f2bc6356836/base/test/scoped_feature_list.cc
[modify] https://crrev.com/04afdfd65fe04ef71fa9918d8fea0f2bc6356836/base/test/scoped_feature_list.h
[add] https://crrev.com/04afdfd65fe04ef71fa9918d8fea0f2bc6356836/base/test/scoped_feature_list_unittest.cc

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

commit cdb47416618522534d42103efe545aea462536e9
Author: timloh <timloh@chromium.org>
Date: Fri Jun 30 06:02:45 2017

Revert of Change ScopedFeatureList to overrides FeatureList not reset (patchset #1 id:1 of https://codereview.chromium.org/2962963002/ )

Reason for revert:
Causing Mac bots to OOM on AppShimInteractiveTest tests

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.10_Tests%2F20150%2F%2B%2Frecipes%2Fsteps%2Finteractive_ui_tests%2F0%2Fstdout

Original issue's description:
> Reland of Change ScopedFeatureList to overrides FeatureList not reset (patchset #1 id:1 of https://codereview.chromium.org/2850073002/ )
>
> Reason for revert:
> The ASAN failure is fixed at https://codereview.chromium.org/2874693002/.
>
> This patch is ready for reland.
>
> Original issue's description:
> > Revert of Change ScopedFeatureList to overrides FeatureList not reset (patchset #6 id:140001 of https://codereview.chromium.org/2834583002/ )
> >
> > Reason for revert:
> > Mac ASAN bots reporting use-after-free errors.
> >
> > Original issue's description:
> > > Change ScopedFeatureList to overrides FeatureList not reset
> > >
> > > The current situation is that using ScopedFeatureList resets to an
> > > empty feature list and then enables/disables an explicit list of
> > > features.
> > >
> > > That's never what you want for browser tests (or other higher-level
> > > tests) since it effectively overrides higher-level test configurations
> > > (e.g. those in fieldtrial_testing_config.json, or a bot set up to
> > > specifically test a feature).
> > >
> > > In this patch:
> > >
> > > 1. Keep SFL::Init, SFL::InitWithFeatureList,
> > >    SFL::InitFromCommandLine reset to empty list but add warning to
> > >    remind developer should use them with care.
> > > 2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
> > >    SFL::InitWithFeatures to not reset but override current FeatureList
> > >    with given enables/disables.
> > >
> > > We also add unit tests for ScopedFeatureList.
> > >
> > > BUG= 713390 
> > >
> > > Review-Url: https://codereview.chromium.org/2834583002
> > > Cr-Commit-Position: refs/heads/master@{#468210}
> > > Committed: https://chromium.googlesource.com/chromium/src/+/9c04ed553bd7abe820a6a93c5e8981e6738881a9
> >
> > TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,chaopeng@chromium.org
> > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > BUG= 713390 
> >
> > Review-Url: https://codereview.chromium.org/2850073002
> > Cr-Commit-Position: refs/heads/master@{#468263}
> > Committed: https://chromium.googlesource.com/chromium/src/+/6c14f269fb196627c72a810205488d694b63a7d5
>
> TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,thestig@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 713390 
>
> Review-Url: https://codereview.chromium.org/2962963002
> Cr-Commit-Position: refs/heads/master@{#483631}
> Committed: https://chromium.googlesource.com/chromium/src/+/04afdfd65fe04ef71fa9918d8fea0f2bc6356836

TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,thestig@chromium.org,chaopeng@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 713390 

Review-Url: https://codereview.chromium.org/2964623002
Cr-Commit-Position: refs/heads/master@{#483641}

[modify] https://crrev.com/cdb47416618522534d42103efe545aea462536e9/base/BUILD.gn
[modify] https://crrev.com/cdb47416618522534d42103efe545aea462536e9/base/test/scoped_feature_list.cc
[modify] https://crrev.com/cdb47416618522534d42103efe545aea462536e9/base/test/scoped_feature_list.h
[delete] https://crrev.com/4d2bde053d9260694e9fc9c02752a2990cb09f59/base/test/scoped_feature_list_unittest.cc

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

commit e86c74ee8e06e2a7538b273efe5e4f17fc0c38f9
Author: chaopeng <chaopeng@chromium.org>
Date: Fri Jul 07 14:44:35 2017

Reland of Change ScopedFeatureList to overrides FeatureList not reset

The current situation is that using ScopedFeatureList resets to an
empty feature list and then enables/disables an explicit list of
features.

That's never what you want for browser tests (or other higher-level
tests) since it effectively overrides higher-level test configurations
(e.g. those in fieldtrial_testing_config.json, or a bot set up to
specifically test a feature).

In this patch:

1. Keep SFL::Init, SFL::InitWithFeatureList,
   SFL::InitFromCommandLine reset to empty list but add warning to
   remind developer should use them with care.
2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
   SFL::InitWithFeatures to not reset but override current FeatureList
   with given enables/disables.

We also add unit tests for ScopedFeatureList.

BUG= 713390 
Original-Review-Url: https://codereview.chromium.org/2834583002
Review-Url: https://codereview.chromium.org/2968793002
Cr-Commit-Position: refs/heads/master@{#484921}

[modify] https://crrev.com/e86c74ee8e06e2a7538b273efe5e4f17fc0c38f9/base/BUILD.gn
[modify] https://crrev.com/e86c74ee8e06e2a7538b273efe5e4f17fc0c38f9/base/test/scoped_feature_list.cc
[modify] https://crrev.com/e86c74ee8e06e2a7538b273efe5e4f17fc0c38f9/base/test/scoped_feature_list.h
[add] https://crrev.com/e86c74ee8e06e2a7538b273efe5e4f17fc0c38f9/base/test/scoped_feature_list_unittest.cc

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

commit 83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b
Author: chaopeng <chaopeng@chromium.org>
Date: Fri Jul 07 23:25:03 2017

Support Using ScopedFeatureList in SetUp for Browser Test

Currently we don't support using ScopedFeatureList::* in SetUp for InProcessBrowserTest since we clear FeatureList at the beginning of InProcessBrowserTest::SetUp.

In this patch, we remove the FeatureList::ClearInstanceForTesting() and move the feature change in InProcessBrowserTest to ScopedFeatureList. So we are able to move all feature list change calls SetUp and use ScopedFeatureList::*.

BUG= 713390 

Review-Url: https://codereview.chromium.org/2876153002
Cr-Commit-Position: refs/heads/master@{#485096}

[modify] https://crrev.com/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b/base/test/test_suite.cc
[modify] https://crrev.com/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b/base/test/test_suite.h
[modify] https://crrev.com/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b/chrome/browser/net/predictor.cc
[modify] https://crrev.com/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b/chrome/common/chrome_features.cc
[modify] https://crrev.com/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b/chrome/common/chrome_features.h
[modify] https://crrev.com/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b/chrome/test/base/in_process_browser_test.cc
[modify] https://crrev.com/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b/chrome/test/base/in_process_browser_test.h
[modify] https://crrev.com/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b/chrome/test/base/test_launcher_utils.cc

Hi! I don't know if it's intended, but the CL from comment 8 makes it impossible to disable a feature from browser tests if we pass --disable-features as a command line flag.

For example, if I pass "--disable-features=browser-side-navigation", the test ends up InProcessBrowserTest::SetUp with the following command line:
"--disable-features=browser-side-navigation --disable-features=NetworkPrediction". When the feature list gets initialized in the main thread, it will only take into account the latest of the disable-features (NetworkPrediction) effectivelt overriding what I passed to the browser_tests.

Could you have a look at this? We nearly landed a CL that was supposed to keep some bots testing with our feature disabled (in case we reverted) and just found by chance that the flag we were passing was actually useless. Also it makes debugging browser tests harder.

Thanks!

#10 this issue intent to fix the issue your facing. It stopped since a another conflict approach, the conflict approach seems removed now, I will restart this.

Hi Ilya, I still have 2 test suites not able to move to ScopedFeatureList. 

- In chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc, dom_size[1] become 1 after my change[2] and I dont understand. Emailed to lpz@ for help.
- In chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc, after my change I found, these tests we dont have a proper setup function can use, SetUp not support field trial and SetUpOnMainThread is call after MaybeEnableResourcePrefetching[3] which we determine create predictor.

What do you think for the second case?

[1] https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc?rcl=04c767fee7b760e04e468016a02582af461ebcb1&l=985

[2] https://chromium-review.googlesource.com/c/chromium/src/+/665657/3/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

[3] https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefetch_common.cc?rcl=04c767fee7b760e04e468016a02582af461ebcb1&l=34

I'd recommend landing everything from your change outside of these two files, and splitting them off to separate CLs.

For the second case, why isn't it possible to set up the field trial in the constructor?

- If we setup field trial in constructor, we need to setup a FieldTrialList before create FieldTrial then we hit a DCHECK fail[1] because we recreate FieldTrialList when browser start. It seems because ContentBrowserTest run tests on render process, we can do this on ContentBrowserTest. [2]

- If we just setup scoped feature list and setup field trials in command line, the field trial will not associate with feature.[3]

[1] https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=ee82805ddb0782e7c8940192ebd7ba602e2b3d54&l=502

[2] https://cs.chromium.org/chromium/src/content/browser/download/download_browsertest.cc?rcl=7dd05b8243f89a1af4927912e5ca9750bdc9bdad&l=763

[3] https://cs.chromium.org/chromium/src/base/feature_list.cc?rcl=1954a79197b824a6983d05e25ddd6103892d1dcf&l=368

Hey, I'm actually getting blocked on the same issue as in #10 - I need to explicitly disable an default-enabled feature to make sure functionality works correctly with a feature disabled. 
Where are we on a fix here?

Hi clamy@ and rkaplow@ Please try to use ScopedFeatureList to modify features in BrowserTest. see crrev.com/c/672141. 

crbug.com/767862 seems possibly related to this.

I think that chaopeng@ meant to link to one of these CLs in comment #17:
[1] https://chromium-review.googlesource.com/c/chromium/src/+/665657
[2] https://chromium-review.googlesource.com/c/chromium/src/+/665817

 Issue 753410  has been merged into this issue.

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

commit 831558e75e852a9225cbb109da5baf52503eb0f5
Author: chaopeng <chaopeng@chromium.org>
Date: Fri Nov 10 20:34:03 2017

Forbid use CommandLine to change FeatureList in BrowserTest

This patch will load features from CommandLine before BrowserTest
initialize then remove them from CommandLine and check before BrowserTest
Browser start.

After this patch, change FeatureList by CommandLine will get DCHECK
fail.

Bug:  713390 
Change-Id: I90c8fb4bd56b90f1b88470c04f83c6f9f72d8104
Reviewed-on: https://chromium-review.googlesource.com/665817
Commit-Queue: Jianpeng Chao <chaopeng@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515663}
[modify] https://crrev.com/831558e75e852a9225cbb109da5baf52503eb0f5/base/test/test_suite.cc
[modify] https://crrev.com/831558e75e852a9225cbb109da5baf52503eb0f5/content/public/test/browser_test_base.cc
[modify] https://crrev.com/831558e75e852a9225cbb109da5baf52503eb0f5/content/test/content_browser_test_test.cc

The final patch for this issue land. 

Thank you for your persistence in seeing this work through to completion!