chaopeng: According to the settings prompt team thois issue happens when features in browser tests not are enabled in SetUpInProcessBrowserTestFixture(). Since you've been changing ScopedFeatureList do you have any idea of what could be going on here? There are already four instances of tests being flaky because of this.

Why we need to enable feature in SetUp then disable feature in TestBody? Can we actually turn it on/off after browser start?

If yes, you may want to check the feature_list design doc.

```
Similar to FieldTrialList, the IsEnabled() API will be thread safe (can be used from any thread), but the common case of checking the state of a feature would not require a lock - as the relevant fields will not change values after start up initialization. For features supporting run-time killing, a separate codepath will be used that will use a lock to guarantee thread safety.
```

https://docs.google.com/document/d/1n9tVb0KI2GmHwnvtwCbZjyXgT4-aBDBrUEfhzsV-Twk/preview#

The feature can't actually be turned on/off after start. The tests needs to check what happens after the feature is disabled. So we need to disable the feature during the tests.

Would it be possible to fix the root cause? As mention in the description there are at least three other instances of flaky tests that are enabling/disabling features in the test. These tests have their own reasons as to why they need to enable/disable the feature in the test.

if your stack trace the same as other 2 tests.

Maybe you can try: enable kSettingsResetPrompt in ctor with scoped_feature_list. 

https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc?rcl=d21041d4d1342713aba41f8474cf7cff3647e05b&l=28

That's a nice workaround but I don't think every single test that wants to use ScopedFeatureList inside the test should be doing that.

If this feature need for all tests, we should add it in test_suite.cc. If it is only for some tests, you can set it in ctor.

I'm not quite following what the question/issue is here.  Is it that the thread is enabling the feature on one thread, and checking the state on another?  If so, then I agree that it's the test's responsibility to ensure correct ordering there, either by enabling the feature before the code is multithreaded, or by explicitly sequencing events across threads.

Hi isherman@

It seems ortuno@ saw stack trace same as Issue 767862 and  Issue 729579 .

[ RUN      ] SSLUICaptivePortalListTest.Disabled
[8112:5008:0922/055547.970:WARNING:chrome_browser_main_win.cc(610)] Command line too long for RegisterApplicationRestart:  --brave-new-test-launcher --cfi-diag=0 --gtest_also_run_disabled_tests --gtest_filter=SSLUICaptivePortalListTest.Disabled --single_process --test-launcher-bot-mode --test-launcher-summary-output="e:\b\swarm_slave\w\iolewh96\output.json" --user-data-dir="C:\Users\CHROME~2\AppData\Local\Temp\scoped_dir6476_15126\d6476_14706" --disable-offline-auto-reload --allow-running-insecure-content --process-per-site --no-first-run --no-default-browser-check --enable-logging=stderr --safebrowsing-disable-auto-update --disable-default-apps --wm-window-animations-disabled --disable-component-update --test-type=browser --force-color-profile=srgb --disable-zero-browsers-open-for-tests --ipc-connection-timeout=45 --allow-file-access-from-files --dom-automation --log-gpu-control-list-decisions --disable-backgrounding-occluded-windows --disable-gl-drawing-for-tests --override-use-software-gl-for-tests --force-color-profile=srgb --disable-features=NetworkPrediction --flag-switches-begin --flag-switches-end --restore-last-session about:blank
[5848:148:0922/055548.609:ERROR:direct_composition_surface_win.cc(1095)] Failing to detect HDR, couldn't retrieve D3D11 device from ANGLE.
[5848:148:0922/055548.779:INFO:media_foundation_video_encode_accelerator_win.cc(335)] Windows versions earlier than 8 are not supported.
[8112:5008:0922/055550.640:FATAL:settings_reset_prompt_config.cc(54)] Check failed: IsPromptEnabled().
Backtrace:
	base::debug::StackTrace::StackTrace [0x100AE337+55]
	base::debug::StackTrace::StackTrace [0x100ADFD1+17]
	logging::LogMessage::~LogMessage [0x100FF82E+94]
	safe_browsing::SettingsResetPromptConfig::UrlToResetDomainId [0x0598C704+148]
	safe_browsing::SettingsResetPromptModel::InitDefaultSearchData [0x059907E7+279]
	safe_browsing::SettingsResetPromptModel::SettingsResetPromptModel [0x0598FEC4+948]
	std::make_unique<safe_browsing::SettingsResetPromptModel,Profile * &,std::unique_ptr<safe_browsing::SettingsResetPromptConfig,std::default_delete<safe_browsing::SettingsResetPromptConfig> >,std::unique_ptr<ProfileResetter,std::default_delete<ProfileResett [0x05987EC5+85]
	base::MakeUnique<safe_browsing::SettingsResetPromptModel,Profile * &,std::unique_ptr<safe_browsing::SettingsResetPromptConfig,std::default_delete<safe_browsing::SettingsResetPromptConfig> >,std::unique_ptr<ProfileResetter,std::default_delete<ProfileResett [0x05987AD3+51]
	base::internal::BindState<void (__cdecl*)(std::unique_ptr<safe_browsing::SettingsResetPromptModel,std::default_delete<safe_browsing::SettingsResetPromptModel> >,std::unique_ptr<BrandcodedDefaultSettings,std::default_delete<BrandcodedDefaultSettings> >),ba [0x05988669+281]
	base::internal::FunctorTraits<void (__cdecl*)(std::unique_ptr<safe_browsing::SettingsResetPromptConfig,std::default_delete<safe_browsing::SettingsResetPromptConfig> >),void>::Invoke<std::unique_ptr<safe_browsing::SettingsResetPromptConfig,std::default_del [0x05987957+23]
	base::internal::InvokeHelper<0,void>::MakeItSo<void (__cdecl*)(std::unique_ptr<safe_browsing::SettingsResetPromptConfig,std::default_delete<safe_browsing::SettingsResetPromptConfig> >),std::unique_ptr<safe_browsing::SettingsResetPromptConfig,std::default_ [0x05987A84+36]
	base::internal::Invoker<base::internal::BindState<void (__cdecl*)(std::unique_ptr<safe_browsing::SettingsResetPromptConfig,std::default_delete<safe_browsing::SettingsResetPromptConfig> >),base::internal::PassedWrapper<std::unique_ptr<safe_browsing::Settin [0x05987CA7+135]
	base::internal::Invoker<base::internal::BindState<void (__cdecl*)(std::unique_ptr<safe_browsing::SettingsResetPromptConfig,std::default_delete<safe_browsing::SettingsResetPromptConfig> >),base::internal::PassedWrapper<std::unique_ptr<safe_browsing::Settin [0x05988846+54]
	base::OnceCallback<void __cdecl(void)>::Run [0x1004A1E5+53]
	base::debug::TaskAnnotator::RunTask [0x100B355E+414]
	base::internal::IncomingTaskQueue::RunTask [0x1012A302+146]
	base::MessageLoop::RunTask [0x10133E30+512]
	base::MessageLoop::DeferOrRunPendingTask [0x10132482+50]
	base::MessageLoop::DoDelayedWork [0x101328AE+350]
	base::MessagePumpForUI::DoRunLoop [0x1013AA10+128]
	base::MessagePumpWin::Run [0x1013B96B+123]
	base::MessageLoop::Run [0x10133B2F+191]
	base::RunLoop::Run [0x101F85DA+170]
	content::RunThisRunLoop [0x03E4BC6E+30]
	content::MessageLoopRunner::Run [0x03E4B6D2+162]
	content::TestNavigationObserver::Wait [0x03E7E1AC+28]
	ui_test_utils::NavigateToURLWithDispositionBlockUntilNavigationsComplete [0x03B3A6FE+926]
	ui_test_utils::NavigateToURLWithDisposition [0x03B3A34A+26]
	ui_test_utils::NavigateToURL [0x03B3A2F4+20]
	SSLUICaptivePortalListTest_Disabled_Test::RunTestOnMainThread [0x0164B0E9+873]
	content::BrowserTestBase::ProxyRunTestOnMainThreadLoop [0x03E6F89F+287]
	??$Invoke@PAVBrowserTestBase@content@@$$V@?$FunctorTraits@P8BrowserTestBase@content@@AEXXZX@internal@base@@SAXP8BrowserTestBase@content@@AEXXZ$$QAPAV34@@Z [0x03E6C94B+11]
	base::internal::InvokeHelper<0,void>::MakeItSo<void (__thiscall content::BrowserTestBase::*const &)(void),content::BrowserTestBase *> [0x03E6CA04+36]
	base::internal::Invoker<base::internal::BindState<void (__thiscall content::BrowserTestBase::*)(void),base::internal::UnretainedWrapper<content::BrowserTestBase> >,void __cdecl(void)>::RunImpl<void (__thiscall content::BrowserTestBase::*const &)(void),std [0x03E6CC79+137]
	base::internal::Invoker<base::internal::BindState<void (__thiscall content::BrowserTestBase::*)(void),base::internal::UnretainedWrapper<content::BrowserTestBase> >,void __cdecl(void)>::Run [0x03E6FB64+36]
	base::RepeatingCallback<void __cdecl(void)>::Run [0x00E332C1+33]
	ChromeBrowserMainParts::PreMainMessageLoopRunImpl [0x0440A1C4+5220]
	ChromeBrowserMainParts::PreMainMessageLoopRun [0x04408CB4+244]
	content::BrowserMainLoop::PreMainMessageLoopRun [0x1541E527+215]
	??$Invoke@PAVBrowserMainLoop@content@@$$V@?$FunctorTraits@P8BrowserMainLoop@content@@AEHXZX@internal@base@@SAHP8BrowserMainLoop@content@@AEHXZ$$QAPAV34@@Z [0x15412B2B+11]
	base::internal::InvokeHelper<0,int>::MakeItSo<int (__thiscall content::BrowserMainLoop::*const &)(void),content::BrowserMainLoop *> [0x15412F34+36]
	base::internal::Invoker<base::internal::BindState<int (__thiscall content::BrowserMainLoop::*)(void),base::internal::UnretainedWrapper<content::BrowserMainLoop> >,int __cdecl(void)>::RunImpl<int (__thiscall content::BrowserMainLoop::*const &)(void),std::t [0x15413299+137]
	base::internal::Invoker<base::internal::BindState<int (__thiscall content::BrowserMainLoop::*)(void),base::internal::UnretainedWrapper<content::BrowserMainLoop> >,int __cdecl(void)>::Run [0x1541E9F4+36]
	base::RepeatingCallback<int __cdecl(void)>::Run [0x16273D61+33]
	content::StartupTaskRunner::RunAllTasksNow [0x162BC42B+107]
	content::BrowserMainLoop::CreateStartupTasks [0x1541A51E+638]
	content::BrowserMainRunnerImpl::Initialize [0x15424A1A+890]
	content::BrowserMain [0x154103FF+95]
	content::RunNamedProcessTypeMain [0x172A2667+135]
	content::ContentMainRunnerImpl::Run [0x172A253E+414]
	content::ContentServiceManagerMainDelegate::RunEmbedderProcess [0x172A0114+36]
	service_manager::Main [0x19F59147+823]
	content::ContentMain [0x172A0669+41]
	content::BrowserTestBase::SetUp [0x03E7014E+1294]
	InProcessBrowserTest::SetUp [0x03B33FAD+733]
	SSLUITest::SetUp [0x0166E63B+155]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void> [0x02254F54+52]
	testing::Test::Run [0x0226F7FD+77]
	testing::TestInfo::Run [0x0226FABD+173]
	testing::TestCase::Run [0x0226F95F+191]
	testing::internal::UnitTestImpl::RunAllTests [0x0226FF45+661]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x02255044+52]

Thanks for pasting the stack trace. Note that kSettingsResetPrompt is not a feature that we are currently manually enabling in our tests.

ortuno@, Is kSettingsResetPrompt you work around?

I haven't had a chance to try, will update when I do. That said, it's a bit weird to be manually enabling a feature unrelated to the tests.

Yes, I do not think we should touch unrelated feature in tests. Could you add Prompt folks to this issue, ask if we need kSettingsResetPrompt for all tests?

I don't quite understand why enabling the feature in the constructor would help. Some tests, unrelated to kSettingsResetPrompt, happen to enable or disable a feature during the tests. When doing this the tests become flaky but only on Win7. There seem to be a race rather than a disabled feature. What am I missing?

Features need to be enabled before any threads are started. I believe enabling them in the constructor is the way to guarantee this.

Otherwise, it's undefined behavior if you're enabling features at runtime while other threads are running (and possibly checking feature state). This is not a scenario that exists in the production browser, where all feature stats is finalized in single threaded mode. So writing tests that do this is incorrect.

(Separately, I wonder if we could have DCHECKs that could catch this reliably rather than the flaky behavior on TSAN bots that we currently observe.)

Fair enough. It would be great if, as you proposed, these types of tests would fail reliably and if this type of incorrect usage was called out in the ScopedFeatureList docs.

Do you have any recommendations on how to test enabling and disabling a feature? In this specific test, we need to make sure that once the user disables the feature it is actually disabled; there is some state that gets saved when the feature is enabled and want to be sure that when the feature is disabled the state goes away.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot