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

Issue 836442 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Data race in DownloadContentTest.ForceDownloadMessageRfc822Page

Project Member Reported by thestig@chromium.org, Apr 24 2018

Issue description

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=content_browsertests&tests=DownloadContentTest.ForceDownloadMessageRfc822Page shows a couple of failures for this test on the TSAN bot:

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/20712
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/20678 (succeeded on a retry)

[ RUN      ] DownloadContentTest.ForceDownloadMessageRfc822Page
Xlib:  extension "RANDR" missing on display ":99".
DevTools listening on ws://127.0.0.1:39325/devtools/browser/5f0cc992-cbea-490e-90af-498e1cecd041
==================
WARNING: ThreadSanitizer: data race (pid=11191)
  Write of size 8 at 0x7b100001c8c8 by main thread:
    #0 SetBrowser content/public/common/content_client.cc:22:24 (content_browsertests+0x2949505)
    #1 content::SetBrowserClientForTesting(content::ContentBrowserClient*) content/public/common/content_client.cc:48 (content_browsertests+0x2949505)
    #2 content::DownloadContentTest_ForceDownloadMessageRfc822Page_Test::RunTestOnMainThread() content/browser/download/download_browsertest.cc:3403:3 (content_browsertests+0x184a18b)
...
  Previous read of size 8 at 0x7b100001c8c8 by thread T15:
    #0 browser content/public/common/content_client.h:78:44 (content_browsertests+0x4343e4e)
    #1 content::internal::CreateDefaultPosixFilesToMap(int, mojo::edk::PlatformHandle const&, bool, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, base::CommandLine*) content/browser/child_process_launcher_helper_posix.cc:96 (content_browsertests+0x4343e4e)
    #2 content::internal::ChildProcessLauncherHelper::GetFilesToMap() content/browser/child_process_launcher_helper_linux.cc:41:10 (content_browsertests+0x3d84a9d)
    #3 content::internal::ChildProcessLauncherHelper::LaunchOnLauncherThread() content/browser/child_process_launcher_helper.cc:110:60 (content_browsertests+0x3d83e7a)
...
  Location is heap block of size 64 at 0x7b100001c8c0 allocated by main thread:
    #0 operator new(unsigned long) /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/tsan/rtl/tsan_new_delete.cc:57:3 (content_browsertests+0x1711349)
    #1 content::ShellMainDelegate::BasicStartupComplete(int*) content/shell/app/shell_main_delegate.cc (content_browsertests+0x4737d4a)
    #2 content::ContentMainRunnerImpl::Initialize(content::ContentMainParams const&) content/app/content_main_runner.cc:729:33 (content_browsertests+0x3bdf51e)
    #3 content::ContentServiceManagerMainDelegate::Initialize(service_manager::MainDelegate::InitializeParams const&) content/app/content_service_manager_main_delegate.cc:37:32 (content_browsertests+0x2d23207)
    #4 service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:409:29 (content_browsertests+0x67c8d25)
    #5 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10 (content_browsertests+0x2d23c0e)
...
  Thread T15 'TaskSchedulerSi' (tid=11259, running) created by thread T2 at:
    #0 pthread_create /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:965:3 (content_browsertests+0x16a7785)
    #1 base::(anonymous namespace)::CreateThread(unsigned long, bool, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:115:13 (content_browsertests+0x4c472c7)
    #2 base::PlatformThread::CreateWithPriority(unsigned long, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:200:10 (content_browsertests+0x4c471c5)
    #3 base::internal::SchedulerWorker::Start() base/task_scheduler/scheduler_worker.cc:67:3 (content_browsertests+0x4bd307f)
    #4 CreateTaskRunnerWithTraitsImpl<base::internal::(anonymous namespace)::SchedulerWorkerDelegate> base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:490:13 (content_browsertests+0x4bd4867)
    #5 base::internal::SchedulerSingleThreadTaskRunnerManager::CreateSingleThreadTaskRunnerWithTraits(base::TaskTraits const&, base::SingleThreadTaskRunnerThreadMode) base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:426 (content_browsertests+0x4bd4867)
    #6 base::internal::TaskSchedulerImpl::CreateSingleThreadTaskRunnerWithTraits(base::TaskTraits const&, base::SingleThreadTaskRunnerThreadMode) base/task_scheduler/task_scheduler_impl.cc:155:8 (content_browsertests+0x4bcb27c)
    #7 base::CreateSingleThreadTaskRunnerWithTraits(base::TaskTraits const&, base::SingleThreadTaskRunnerThreadMode) base/task_scheduler/post_task.cc:115:40 (content_browsertests+0x4bc979c)
    #8 Create base/task_scheduler/lazy_task_runner.cc:54:10 (content_browsertests+0x4bc8e41)
    #9 base::internal::LazyTaskRunner<base::SingleThreadTaskRunner, false>::CreateRaw(void*) base/task_scheduler/lazy_task_runner.cc:72 (content_browsertests+0x4bc8e41)
    #10 GetOrCreateLazyPointer<base::SingleThreadTaskRunner> base/lazy_instance_helpers.h:83:48 (content_browsertests+0x4bc8daa)
    #11 base::internal::LazyTaskRunner<base::SingleThreadTaskRunner, false>::Get() base/task_scheduler/lazy_task_runner.cc:92 (content_browsertests+0x4bc8daa)
    #12 GetProcessLauncherTaskRunner content/browser/child_process_launcher_helper.cc:219:31 (content_browsertests+0x3d83cdc)
    #13 content::internal::ChildProcessLauncherHelper::StartLaunchOnClientThread() content/browser/child_process_launcher_helper.cc:99 (content_browsertests+0x3d83cdc)
    #14 content::ChildProcessLauncher::ChildProcessLauncher(std::__1::unique_ptr<content::SandboxedProcessLauncherDelegate, std::__1::default_delete<content::SandboxedProcessLauncherDelegate> >, std::__1::unique_ptr<base::CommandLine, std::__1::default_delete<base::CommandLine> >, int, content::ChildProcessLauncher::Client*, std::__1::unique_ptr<mojo::edk::OutgoingBrokerClientInvitation, std::__1::default_delete<mojo::edk::OutgoingBrokerClientInvitation> >, base::RepeatingCallback<void (std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)> const&, bool) content/browser/child_process_launcher.cc:48:12 (content_browsertests+0x3d8315e)
    #15 content::BrowserChildProcessHostImpl::Launch(std::__1::unique_ptr<content::SandboxedProcessLauncherDelegate, std::__1::default_delete<content::SandboxedProcessLauncherDelegate> >, std::__1::unique_ptr<base::CommandLine, std::__1::default_delete<base::CommandLine> >, bool) content/browser/browser_child_process_host_impl.cc:268:28 (content_browsertests+0x3d222dc)
    #16 content::GpuProcessHost::LaunchGpuProcess() content/browser/gpu/gpu_process_host.cc:1327:13 (content_browsertests+0x3f54ced)
...

 
Cc: jcivelli@chromium.org
content::ContentClient probably should DCHECK it is running on a single thread only. Adding DCHECKs in a test CL will likely flush out all the callers from other threads.

Comment 2 by boliu@chromium.org, Apr 25 2018

Cc: jam@chromium.org
I'm pretty sure accessing Content<Foo>Clients are allowed on any thread. I believe they are supposed to be set before any threads are started, and never changed again. Individual methods should documented thread safety concerns.

There are probably lots of example where it's not called on the UI/Main thread, eg:
https://cs.chromium.org/chromium/src/content/browser/browser_child_process_host_impl.cc?rcl=c474af563adbcba225b58f7d674a271aa8cbc2e2&l=166

Umm.. so should these be suppressed somehow?
Given the model is to set a client first, and then allow reading it from multiple threads. This issue shows we are violating that. The read has already happened on another thread, and then a write is happening.

DownloadContentTest probably need to move the client initialization into SetUp() or something. Calling SetBrowserClientForTesting() in the test body is too late.

Comment 4 by tapted@chromium.org, May 28 2018

Cc: qin...@chromium.org
Components: Blink>BackgroundFetch
Labels: -Pri-3 Pri-1
Owner: jianli@chromium.org
Status: Assigned (was: Untriaged)
This is still causing redness

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/22040


Ping @ jianli, still causing redness (and keeping sheriffs busy) https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/22334

Comment 6 by peter@chromium.org, Jun 4 2018

Components: -Blink>BackgroundFetch UI>Browser>Downloads
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 5 2018

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

commit de94d59196994afbb5f26865535b7154163fa53a
Author: Jian Li <jianli@chromium.org>
Date: Tue Jun 05 20:16:50 2018

Move the browser client initialization for MHTML download tests to SetUp

This is to avoid potential data race issues in tests.

Bug:  836442 , 837104 
Change-Id: Ic46e2834e1a50fa3644e6fe5ec0d4ef874e0b0a0
Reviewed-on: https://chromium-review.googlesource.com/1086156
Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Jian Li <jianli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564638}
[modify] https://crrev.com/de94d59196994afbb5f26865535b7154163fa53a/content/browser/download/download_browsertest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment