New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 782037 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug

Blocked on:
issue 786626



Sign in to add a comment

Data races in base/feature_list.cc

Project Member Reported by meade@chromium.org, Nov 7 2017

Issue description

Components: Blink>Loader
Owner: yhirano@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 7 2017

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

commit e6da2386eb37999d6b86bd515010e18193bb29ec
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Tue Nov 07 12:04:15 2017

Initialize ScopedFeatureList on Test::SetUpOnMainThread

... in order to fix flakiness on TSAN bots.

Bug: 782037
Change-Id: Id0193726c10f54e49bccfce5914b0be10e13ca65
Reviewed-on: https://chromium-review.googlesource.com/756659
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514451}
[modify] https://crrev.com/e6da2386eb37999d6b86bd515010e18193bb29ec/content/browser/renderer_host/render_process_host_browsertest.cc

Comment 3 by sky@chromium.org, Nov 7 2017

The bots are still failing:
https://chromium-swarm.appspot.com/task?id=39b15e2d48e7d810&refresh=10&show_raw=1&wide_logs=true
output:
WARNING: ThreadSanitizer: data race (pid=27845)
  Write of size 8 at 0x000009d96350 by main thread:
    #0 base::FeatureList::ClearInstanceForTesting() base/feature_list.cc:307:14 (content_browsertests+0x42aaed3)
    #1 base::test::ScopedFeatureList::InitWithFeatureList(std::__1::unique_ptr<base::FeatureList, std::__1::default_delete<base::FeatureList> >) base/test/scoped_feature_list.cc:106:28 (content_browsertests+0x3e51916)
    #2 InitFromCommandLine base/test/scoped_feature_list.cc:115:3 (content_browsertests+0x3e521e1)
    #3 base::test::ScopedFeatureList::InitWithFeaturesAndFieldTrials(std::initializer_list<base::Feature> const&, std::initializer_list<base::FieldTrial*> const&, std::initializer_list<base::Feature> const&) base/test/scoped_feature_list.cc:184 (content_browsertests+0x3e521e1)
    #4 InitAndEnableFeatureWithFieldTrialOverride base/test/scoped_feature_list.cc:131:3 (content_browsertests+0x3e5298b)
    #5 base::test::ScopedFeatureList::InitAndEnableFeatureWithParameters(base::Feature const&, std::__1::map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > const&) base/test/scoped_feature_list.cc:201 (content_browsertests+0x3e5298b)
    #6 content::(anonymous namespace)::RenderProcessHostWithKeepAliveOptionEnabledTest::SetUpOnMainThread() content/browser/renderer_host/render_process_host_browsertest.cc:144:19 (content_browsertests+0x1a735be)
    #7 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() content/public/test/browser_test_base.cc:328:5 (content_browsertests+0x3e12264)
    #8 Invoke<content::BrowserTestBase *> base/bind_internal.h:194:12 (content_browsertests+0x3e13408)
    #9 MakeItSo<void (content::BrowserTestBase::*const &)(), content::BrowserTestBase *> base/bind_internal.h:277 (content_browsertests+0x3e13408)
    #10 RunImpl<void (content::BrowserTestBase::*const &)(), const std::__1::tuple<base::internal::UnretainedWrapper<content::BrowserTestBase> > &, 0> base/bind_internal.h:349 (content_browsertests+0x3e13408)
    #11 base::internal::Invoker<base::internal::BindState<void (content::BrowserTestBase::*)(), base::internal::UnretainedWrapper<content::BrowserTestBase> >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:331 (content_browsertests+0x3e13408)
    #12 Run base/callback.h:92:12 (content_browsertests+0x3edb8a8)
    #13 content::ShellBrowserMainParts::PreMainMessageLoopRun() content/shell/browser/shell_browser_main_parts.cc:180 (content_browsertests+0x3edb8a8)
    #14 content::BrowserMainLoop::PreMainMessageLoopRun() content/browser/browser_main_loop.cc:1189:13 (content_browsertests+0x3521120)
    #15 Invoke<content::BrowserMainLoop *> base/bind_internal.h:194:12 (content_browsertests+0x3524818)
    #16 MakeItSo<int (content::BrowserMainLoop::*const &)(), content::BrowserMainLoop *> base/bind_internal.h:277 (content_browsertests+0x3524818)
    #17 RunImpl<int (content::BrowserMainLoop::*const &)(), const std::__1::tuple<base::internal::UnretainedWrapper<content::BrowserMainLoop> > &, 0> base/bind_internal.h:349 (content_browsertests+0x3524818)
    #18 base::internal::Invoker<base::internal::BindState<int (content::BrowserMainLoop::*)(), base::internal::UnretainedWrapper<content::BrowserMainLoop> >, int ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:331 (content_browsertests+0x3524818)
    #19 Run base/callback.h:92:12 (content_browsertests+0x3a9abfe)
    #20 content::StartupTaskRunner::RunAllTasksNow() content/browser/startup_task_runner.cc:45 (content_browsertests+0x3a9abfe)
    #21 content::BrowserMainLoop::CreateStartupTasks() content/browser/browser_main_loop.cc:973:25 (content_browsertests+0x351ecb8)
    #22 content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) content/browser/browser_main_runner.cc:119:17 (content_browsertests+0x3525ca8)
    #23 ShellBrowserMain(content::MainFunctionParams const&, std::__1::unique_ptr<content::BrowserMainRunner, std::__1::default_delete<content::BrowserMainRunner> > const&) content/shell/browser/shell_browser_main.cc:23:32 (content_browsertests+0x3edb187)
    #24 content::ShellMainDelegate::RunProcess(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&) content/shell/app/shell_main_delegate.cc:326:16 (content_browsertests+0x3e7fa43)
    #25 content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:414:35 (content_browsertests+0x33db2d1)
    #26 content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:705:12 (content_browsertests+0x33dc01d)
    #27 content::ContentServiceManagerMainDelegate::RunEmbedderProcess() content/app/content_service_manager_main_delegate.cc:51:32 (content_browsertests+0x25cd6bf)
    #28 service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:456:29 (content_browsertests+0x5f2b5b3)
    #29 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10 (content_browsertests+0x25cde6b)
    #30 content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:293:3 (content_browsertests+0x3e11e6f)
    #31 content::ContentBrowserTest::SetUp() content/public/test/content_browser_test.cc:92:20 (content_browsertests+0x3e02876)
    #32 testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc (content_browsertests+0x1df46d6)
    #33 testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2654:11 (content_browsertests+0x1df554d)
    #34 testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2772:28 (content_browsertests+0x1df5d96)
    #35 testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:4677:43 (content_browsertests+0x1dff1b6)
    #36 testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc (content_browsertests+0x1dfebb5)
    #37 RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2237:46 (content_browsertests+0x3e53b76)
    #38 base::TestSuite::Run() base/test/test_suite.cc:270 (content_browsertests+0x3e53b76)
    #39 content::ContentTestLauncherDelegate::RunTestSuite(int, char**) content/test/content_test_launcher.cc:106:48 (content_browsertests+0x3e081ab)
    #40 content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) content/public/test/test_launcher.cc:639:31 (content_browsertests+0x3e343ee)
    #41 main content/test/content_test_launcher.cc:136:10 (content_browsertests+0x3e0813d)

  Previous read of size 8 at 0x000009d96350 by thread T17:
    #0 base::FeatureList::IsEnabled(base::Feature const&) base/feature_list.cc:223:8 (content_browsertests+0x42aa8fd)
    #1 net::CertVerifyProc::CertVerifyProc() net/cert/cert_verify_proc.cc:442:32 (content_browsertests+0x44f478a)
    #2 net::CertVerifyProcNSS::CertVerifyProcNSS() net/cert/cert_verify_proc_nss.cc:757:20 (content_browsertests+0x44fa6bd)
    #3 net::CertVerifyProc::CreateDefault() net/cert/cert_verify_proc.cc:425:14 (content_browsertests+0x44f4717)
    #4 net::CertVerifier::CreateDefault() net/cert/cert_verifier.cc:84:11 (content_browsertests+0x4438b23)
    #5 content::ShellURLRequestContextGetter::GetCertVerifier() content/shell/browser/shell_url_request_context_getter.cc:109:10 (content_browsertests+0x3ee4f49)
    #6 content::ShellURLRequestContextGetter::GetURLRequestContext() content/shell/browser/shell_url_request_context_getter.cc:147:29 (content_browsertests+0x3ee52dc)
    #7 content::ChromeAppCacheService::InitializeOnIOThread(base::FilePath const&, content::ResourceContext*, net::URLRequestContextGetter*, scoped_refptr<storage::SpecialStoragePolicy>) content/browser/appcache/chrome_appcache_service.cc:38:49 (content_browsertests+0x34bc181)
    #8 Invoke<scoped_refptr<content::ChromeAppCacheService>, base::FilePath, content::ResourceContext *, net::URLRequestContextGetter *, storage::SpecialStoragePolicy *> base/bind_internal.h:194:12 (content_browsertests+0x3aa5667)
    #9 MakeItSo<void (content::ChromeAppCacheService::*)(const base::FilePath &, content::ResourceContext *, net::URLRequestContextGetter *, scoped_refptr<storage::SpecialStoragePolicy>), scoped_refptr<content::ChromeAppCacheService>, base::FilePath, content::ResourceContext *, net::URLRequestContextGetter *, storage::SpecialStoragePolicy *> base/bind_internal.h:277 (content_browsertests+0x3aa5667)
    #10 RunImpl<void (content::ChromeAppCacheService::*)(const base::FilePath &, content::ResourceContext *, net::URLRequestContextGetter *, scoped_refptr<storage::SpecialStoragePolicy>), std::__1::tuple<scoped_refptr<content::ChromeAppCacheService>, base::FilePath, content::ResourceContext *, base::internal::RetainedRefWrapper<net::URLRequestContextGetter>, base::internal::RetainedRefWrapper<storage::SpecialStoragePolicy> >, 0, 1, 2, 3, 4> base/bind_internal.h:349 (content_browsertests+0x3aa5667)
    #11 base::internal::Invoker<base::internal::BindState<void (content::ChromeAppCacheService::*)(base::FilePath const&, content::ResourceContext*, net::URLRequestContextGetter*, scoped_refptr<storage::SpecialStoragePolicy>), scoped_refptr<content::ChromeAppCacheService>, base::FilePath, content::ResourceContext*, base::internal::RetainedRefWrapper<net::URLRequestContextGetter>, base::internal::RetainedRefWrapper<storage::SpecialStoragePolicy> >, void ()>::RunOnce(base::internal::BindStateBase*) base/bind_internal.h:318 (content_browsertests+0x3aa5667)
    #12 Run base/callback.h:64:12 (content_browsertests+0x42a74e6)
    #13 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) base/debug/task_annotator.cc:57 (content_browsertests+0x42a74e6)
    #14 base::internal::IncomingTaskQueue::RunTask(base::PendingTask*) base/message_loop/incoming_task_queue.cc:130:19 (content_browsertests+0x42d6bdb)
    #15 base::MessageLoop::RunTask(base::PendingTask*) base/message_loop/message_loop.cc:394:25 (content_browsertests+0x42d4db7)
    #16 DeferOrRunPendingTask base/message_loop/message_loop.cc:406:5 (content_browsertests+0x42d54f1)
    #17 base::MessageLoop::DoWork() base/message_loop/message_loop.cc:450 (content_browsertests+0x42d54f1)
    #18 base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_libevent.cc:220:31 (content_browsertests+0x42da6b0)
    #19 Run base/message_loop/message_loop.cc:345:12 (content_browsertests+0x42d4780)
    #20 non-virtual thunk to base::MessageLoop::Run(bool) base/message_loop/message_loop.cc (content_browsertests+0x42d4780)
    #21 base::RunLoop::Run() base/run_loop.cc:114:14 (content_browsertests+0x43099e0)
    #22 base::Thread::Run(base::RunLoop*) base/threading/thread.cc:255:13 (content_browsertests+0x434e999)
    #23 content::BrowserThreadImpl::IOThreadRun(base::RunLoop*) content/browser/browser_thread_impl.cc:248:11 (content_browsertests+0x3530f5f)
    #24 content::BrowserThreadImpl::Run(base::RunLoop*) content/browser/browser_thread_impl.cc:283:14 (content_browsertests+0x35311d4)
    #25 base::Thread::ThreadMain() base/threading/thread.cc:338:3 (content_browsertests+0x434ec61)
    #26 base::(anonymous namespace)::ThreadFunc(void*) base/threading/platform_thread_posix.cc:75:13 (content_browsertests+0x4345c7d)

I'm going to disable this test for the time being.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 8 2017

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

commit 723199015c18e535373bda0b299f0702295cc012
Author: Scott Violet <sky@chromium.org>
Date: Wed Nov 08 00:38:49 2017

Disables RenderProcessHostWithKeepAliveOptionEnabledTest.KeepAliveRendererProcess

It's been failing on the tsan bot all day.

BUG=782037
TEST=test only change
TBR=yhirano@chromium.org

Change-Id: Id2ad16de97931206b22cb4d3a0d16c6dba7a714a
Reviewed-on: https://chromium-review.googlesource.com/756832
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514673}
[modify] https://crrev.com/723199015c18e535373bda0b299f0702295cc012/content/browser/renderer_host/render_process_host_browsertest.cc

Labels: -Sheriff-Chromium
Removing Sheriff-Chromium now that the bug is assigned and being investigated.
Cc: asvitk...@chromium.org
Labels: Pri-1 Type-Bug
Unless I am missing something, base::FeatureList is supposed to be thread-safe. Right now it is not, g_instance needs to be protected with either atomics or locks.
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 9 2017

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

commit 191b2267b37903d3bda773716c9a28214db1fca7
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Thu Nov 09 10:32:40 2017

Introduce RenderFrameHostImpl::SetKeepAliveTimeoutForTesting

This CL introduces the function from two reasons:

 1. Using InitAndEnableFeatureWithParameters in
    IN_PROC_BROWSER_TEST_F is hard.
 2. We will remove the feature in the future and hence we should not
    rely on the feature to set the timeout value.

This is a speculative fix for the flakiness in TSAN bots.

Bug: 782037
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I29b41f63e4124aeb4cef26b9a9cab8b9d9f1e6f7
Reviewed-on: https://chromium-review.googlesource.com/758191
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515130}
[modify] https://crrev.com/191b2267b37903d3bda773716c9a28214db1fca7/content/browser/frame_host/keep_alive_handle_factory.cc
[modify] https://crrev.com/191b2267b37903d3bda773716c9a28214db1fca7/content/browser/frame_host/keep_alive_handle_factory.h
[modify] https://crrev.com/191b2267b37903d3bda773716c9a28214db1fca7/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/191b2267b37903d3bda773716c9a28214db1fca7/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/191b2267b37903d3bda773716c9a28214db1fca7/content/browser/renderer_host/render_process_host_browsertest.cc

"Unless I am missing something, base::FeatureList is supposed to be thread-safe. Right now it is not, g_instance needs to be protected with either atomics or locks."

base::FeatureList is thread safe if used correctly. The correct usage pattern is its state should be initialized in single-threaded mode and then it can be queried afterwards from any threads. This is how things work in the production browser. And many browser tests do this correctly too. But some don't - hence the issues. Usually it's caused by trying to change the FeatureList state once a thread has already been started - i.e. too late during browser test set up.
ContentBrowserTest::SetUp has 

// Please setup FieldTrial at SetUpOnMainThread.
DCHECK_EQ(base::FieldTrialList::GetFieldTrialCount(), 0u);

which prevents us from using ScopedFeatureList::InitAndEnableFeatureWithParameters IIUC.
Owner: ----
Status: Available (was: Assigned)
.*KeepAlive.* tests look recovered.
Cc: xingliu@chromium.org
Owner: qin...@chromium.org
Status: Assigned (was: Available)
ParallelDownloadTest.* are still failing. Example build: https://build.chromium.org/p/chromium.memory/builders/Linux%20TSan%20Tests/builds/14090.

@qinmin, xingliu: could you please take a look / triage?
Cc: dtrainor@chromium.org
I'm disabling the test, but we should try to figure out what's going on here ...
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 14 2017

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

commit a6d35ea4b4f9e600f067dc39b13c7449b53c4866
Author: Dirk Pranke <dpranke@chromium.org>
Date: Tue Nov 14 20:42:12 2017

Mark ParallelDownload{Complete,Resumption} as flaky under TSAN.

The content_browsertests ParallelDownloadTest/ParallelDownloadComplete
and ParallelDownloadTest/ParallelDownloadResumption appear to have
a flaky data race. Until this is fixed, I'm disabling these tests.

TBR=qinmin@chromium.org
BUG=782037

Change-Id: I0b4e934c37cae15df8efe6dc50ee6b1c4d890298
Reviewed-on: https://chromium-review.googlesource.com/769435
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516413}
[modify] https://crrev.com/a6d35ea4b4f9e600f067dc39b13c7449b53c4866/content/browser/download/download_browsertest.cc

Trying to move the ScopedFeatureList::InitAndEnableFeatureWithParameters to ctor of ParallelDownloadTest but hit the DCHECK in ContentBrowserTest::SetUp, as mentioned in #9, does it make sense to remove the that DCHECK? 

Chrome layer browser tests all call InitAndEnableFeatureWithParameters in the ctor. 
Cc: qin...@chromium.org
 Issue 784405  has been merged into this issue.
Cc: chaopeng@chromium.org
+chaopeng: Can you comment on the question above? Why do we have the DCHECK_EQ(base::FieldTrialList::GetFieldTrialCount(), 0u); DCHECK in ContentBrowserTest::SetUp()?

What's wrong in doing the setup in the ctor?
Cc: jam@chromium.org
+jam for the data racing.

Why we have data racing in ContentBrowserTest? Is is all tests will run on new browser?

The DCHECK is because ContentBrowserTest will not re-create field trials params when browser start. part_ in BrowserMainLoop [1] is not ChromeBrowserMainParts. For the BrowserTestBase consistency, we forbid use InitAndEnableFeatureWithParameters in ctor. I have a patch [2] for fix it and allow use InitAndEnableFeatureWithParameters in ctor.

[1] https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?rcl=9a494a4e49188d2870df05b3e94c665743d984ff&l=841
[2] https://chromium-review.googlesource.com/c/chromium/src/+/723802/25
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 17 2017

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

commit 3955ed343cdbaca4f54a305a0ceb4567e976ef16
Author: Xing Liu <xingliu@chromium.org>
Date: Fri Nov 17 22:23:51 2017

Fix ParallelDownloadTest on TSAN bots.

There is a threading issue for ScopedFeatureList that it can't be called
in SetupOnMainThread in content browser test. From thread santinizer
log, it seems some early IO thread code is reading while main thread
ScopedFeatureList is writing into FeatureList.

This CL tries to write to FeatureList after the test started to run.

There is some conflicting comment and DCHECK in ScopedFeatureList and
ContentBrowserTest, that the comment in ScopedFeatureList suggests init
in Setup, where ContentBrowserTest::Setup has a DCHECK to suggest to init
in SetupOnMainThread. However, lots of tests do the init in test case
and don't have the flaky issue.

Bug: 782037
Change-Id: Ia6ca649b7df2b49f7e88de6a114d7d099290454b
Reviewed-on: https://chromium-review.googlesource.com/769793
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517588}
[modify] https://crrev.com/3955ed343cdbaca4f54a305a0ceb4567e976ef16/content/browser/download/download_browsertest.cc

We have  bug 786626  for ParallelDownloadTest.ParallelDownloadComplete.
ParallelDownloadTest.ParallelDownloadResumption has been still flaky on "Linux TSan Tests".

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=content_browsertests&tests=ParallelDownloadTest.ParallelDownloadResumption
It's also flaky on "Lollipop Phone Tester"

I have another CL in review for this issue. https://chromium-review.googlesource.com/c/chromium/src/+/772191
Project Member

Comment 24 by bugdroid1@chromium.org, Nov 22 2017

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

commit 90e2c60490709a47d331794f4319878dd64bf0c9
Author: chaopeng <chaopeng@chromium.org>
Date: Wed Nov 22 22:47:43 2017

Allow InitAndEnableFeatureWithParameters in the ContentBrowserTest ctor.

Currently we can not call InitAndEnableFeatureWithParameters in test ctor
because ContentBrowserTest does not recreate FieldTrials during BrowserMain.
Calling InitAndEnableFeatureWithParameters in testbody after browser start may
call data racing.

In this patch, we recreate FieldTrials in ContentShell so we can call
InitAndEnableFeatureWithParameters in test ctor.

Bug: 782037
Change-Id: Idb58587402f952e914d226e9bd5f7d530dd327cc
Reviewed-on: https://chromium-review.googlesource.com/772191
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Jianpeng Chao <chaopeng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518779}
[modify] https://crrev.com/90e2c60490709a47d331794f4319878dd64bf0c9/content/browser/download/download_browsertest.cc
[modify] https://crrev.com/90e2c60490709a47d331794f4319878dd64bf0c9/content/public/test/content_browser_test.cc
[modify] https://crrev.com/90e2c60490709a47d331794f4319878dd64bf0c9/content/shell/browser/shell_browser_main_parts.cc
[modify] https://crrev.com/90e2c60490709a47d331794f4319878dd64bf0c9/content/shell/browser/shell_browser_main_parts.h

Cc: -tyoshino@chromium.org
-cc as keepalive related ones have been fixed (#10)
It looks the flaky is gone. qinmin@ would you like to mark it fixed?
Cc: kbr@chromium.org jmad...@chromium.org
Labels: Hotlist-PixelWrangler
Seeing a high flake rate on linux_chromium_tsan_rel_ng[1], seems to be a different root cause, but will follow up on this issue since it seems to have all the right people cc'ed. 

[1]: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng?numbuilds=200

Flakes seem to happen always with WebRtcGetUserMediaBrowserTest.TwoGetUserMediaAndVerifyFrameRate:

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/203992
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/203991

From javascript: Error: Expected frame rate 15 for element local-view-1, but got 0
    at failTest (http://127.0.0.1:51822/media/webrtc_test_utilities.js:30:17)
    at navigator.mediaDevices.getUserMedia.then.then.catch.then.catch.unexpectedFrameRateMsg (http://127.0.0.1:51822/media/getusermedia.html:313:11)
    at <anonymous>
When executing 'twoGetUserMediaAndVerifyFrameRate({video: {mandatory: {minWidth:640 , minHeight: 480, minFrameRate : 15, maxFrameRate : 15}}},{video: {mandatory: {maxWidth:320 , maxHeight: 240,minFrameRate : 7, maxFrameRate : 7}}}, 15, 7)'
../../content/browser/webrtc/webrtc_content_browsertest_base.cc:92: Failure
Failed
[  FAILED  ] WebRtcGetUserMediaBrowserTest.TwoGetUserMediaAndVerifyFrameRate, where TypeParam =  and GetParam() =  (23322 ms)

Seems to be severely affecting CQ stability. Can anyone cc'ed on this issue help triage?

Comment 28 by kbr@chromium.org, Nov 29 2017

Components: Blink>WebRTC
Labels: -Hotlist-PixelWrangler

Comment 29 by kbr@chromium.org, Nov 29 2017

Labels: Stability-Memory-MemorySanitizer OS-Linux

Comment 30 by kbr@chromium.org, Dec 1 2017

Blockedon: 786626
Is this really a Pri-1?
#27 is not related with FeatureList should start a new issue. The root cause of this original issue is fixed. This issue should close.
Status: Fixed (was: Assigned)
the original bug should have been fixed by #19 and #24, closing this now
Owner: ----
Status: Available (was: Fixed)
Looking at https://ci.chromium.org/buildbot/chromium.memory/Linux%20TSan%20Tests/?limit=200 there are three flakes in the past 200 builds. Is there another bug open to cover the persistent flakiness?
Blockedon: -786626
Blockedon: 786626
Looks like the blocking bug has been fixed for some time now.
Summary: Data races in base/feature_list.cc (was: content_browsertests flaky on chromium.memory/Linux TSan Tests)
Alexei, is it possible to introduce a thread-safe (e.g. atomic) check that fires if the FeatureList is used before it's initialized
This will help us detect all the tests that are using it incorrectly, if that's the case.
Issue 722409 also looks related.
glider@, you 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
Components: -Blink>WebRTC
Removing Blink>WebRTC component since the flaky WebRTC test referred to in #27 was removed. The flakiness was inherent in the test and not related to this bug. See  bug 789121 . 

Sign in to add a comment