I am observing the following:

1. Browser tests initialize base::FeatureList via product code:

      #1 0x7f2d4a0cc01d base::FeatureList::FeatureList()
      #2 0x7f2d4a0cd6d4 base::FeatureList::InitializeInstance()
      #3 0x7f2d4aba7ace content::BrowserMainLoop::PreCreateThreads()
      #4 0x7f2d4aeb83d6 content::StartupTaskRunner::RunAllTasksNow()
      #5 0x7f2d4aba800f content::BrowserMainLoop::CreateStartupTasks()
      #6 0x7f2d4abac49f content::BrowserMainRunnerImpl::Initialize()
      #7 0x00000091520c ShellBrowserMain()
      #8 0x00000090ed68 content::ShellMainDelegate::RunProcess()
      #9 0x7f2d4b1cb18b content::RunNamedProcessTypeMain()
      #10 0x7f2d4b1cbcf7 content::ContentMainRunnerImpl::Run()
      #11 0x7f2d4174d5a8 service_manager::Main()
      #12 0x7f2d4b1cac07 content::ContentMain()
      #13 0x0000008e36cf content::BrowserTestBase::SetUp()
      #14 0x0000008ddfd9 content::ContentBrowserTest::SetUp()
      #15 0x000000791aa5 content::TopDocumentIsolationTest::SetUp()

2. base::test::ScopedFeatureList has to be used *after* #1
   (because only test code is allowed to reinitialize the FeatureList singleton)

3. If #1 spawns threads that inspect feature state, then we have a race with #2.


Maybe the race can be avoided by moving #2 earlier (i.e. from SetUpOnMainThread to SetUp)?

Yep, that sounds like the correct fix to me. SetUpOnMainThread() is too late for it.

Okay - looking at content::BrowserTestBase::SetUp, it seems that base::test::ScopedFeatureList should be initialized from an overriden SetUp method, before calling the parent's SetUp (a comment in content::BrowserTestBase::SetUp says that it takes care to "copy features to the command line" to avoid them being wiped out by BrowserMain).  This helps for TopDocumentIsolationTest and for HostedAppVsTdiTest.

OTOH, the above pattern doesn't work for SubframeTaskTDIBrowserTest, because InProcessBrowserTest::SetUp unconditionally calls base::FeatureList::ClearInstanceForTesting().  I think this call to ClearInstanceForTesting should be removed (since the feature list will be taken care of by content::BrowserTestBase::SetUp).  Any objections to this?  It seems to work fine in local testing.

SG assuming none of the tests rely on clearing the instance and then setting up another instance (which is a scenario that can't happen in Chrome so doesn't make sense to try to test.)

Also would be good to add comments about these subtleties to the header for ScopedFeatureList to provide better guidance to people in the future.

In a WIP CL @ https://codereview.chromium.org/2879293002 I am trying to add a DCHECK that would fire when a test tries to initialize base::test::ScopedFeatureList too late.  It turns out that this DCHECK fires from DataUrlNavigationBrowserTest.HTML_Navigation_Allow_FeatureFlag which overrides features from within a test.  I don't understand why this doesn't get reported by TSAN.

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

commit 0b772bacda31df9e862388f4859f1d56d3d217ae
Author: lukasza <lukasza@chromium.org>
Date: Mon May 15 21:41:01 2017

Move setup of base::test::ScopedFeatureList earlier, to avoid data race.

base::test::ScopedFeatureList has to be setup before
content::BrowserTestBase::SetUp calls content::ContentMain
(because the latter spawns other threads that might start reading
the global state of features).

BUG=722409
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/0b772bacda31df9e862388f4859f1d56d3d217ae/base/test/scoped_feature_list.h
[modify] https://crrev.com/0b772bacda31df9e862388f4859f1d56d3d217ae/content/browser/top_document_isolation_browsertest.cc

There are quite a few tests that (re)initialize features at the beginning of a testcase, while the browser is already running.  Examples:

AppBannerManagerBrowserTest.CheckOnLoadThenNavigate
AppBannerManagerBrowserTest.CheckOnLoadWithSufficientEngagement
AppBannerManagerBrowserTest.CheckOnLoadWithSufficientEngagementCancelBannerAfterPromptInHandler
AppBannerManagerBrowserTest.CheckOnLoadWithSufficientEngagementCancelDirect
AppBannerManagerBrowserTest.CheckOnLoadWithoutSufficientEngagement
JavaScriptDialogTest.ClosingPageSharingRendererDoesntHang
JavaScriptDialogTest.ClosingPageWithSubframeAlertingDoesntCrash
JavaScriptDialogTest.ReloadDoesntHang
(there are ~75 examples in browser_tests;  more in other test executables with browser tests).

I guess one option would be to make a change to initialize the features in SetUp, rather than in a testcase.  Maybe this is safe after all if one is "careful"?  At any rate transitioning to SetUp is not always straightforward - for example it is not immediately obvious to me how to do this in case of the whole PrintPreviewWebUITest test suite, which goes through a code generator:

    chrome/test/data/webui/print_preview.js:

      testGenPreamble: function() {
        // Enable print as image for tests on non Windows/Mac.
        GEN('#if !defined(OS_WINDOWS) && !defined(OS_MACOSX)');
        GEN('  base::FeatureList::ClearInstanceForTesting();');
        GEN('  std::unique_ptr<base::FeatureList>');
        GEN('      feature_list(new base::FeatureList);');
        GEN('  feature_list->InitializeFromCommandLine(');
        GEN('      features::kPrintPdfAsImage.name, std::string());');
        GEN('  base::FeatureList::SetInstance(std::move(feature_list));');
        GEN('#endif');

asvitkine@ - any suggestions on how to proceed here?  FWIW https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20TSan%20Tests/5810 and later are green, but it would be great if we could also have DCHECKs (like the ones sketched in https://codereview.chromium.org/2887523002) that catch incorrect/too-late usage of base::test::ScopedFeatureList.  OTOH, there are quite a few baked-in tests that violate this DCHECK... :-/

We can setup the FeatureList on BrowserTest ctor today. Like this, https://cs.chromium.org/chromium/src/chrome/browser/net/predictor_browsertest.cc?rcl=70b55043fb5df3465f3e7ed81535a0b90b27794c&l=520

Available, but no owner or component? Please find a component, as no one will ever find this without one.