Chromium Code Reviews
        
  DescriptionRevert of Pass task runners to AudioManager constructor. (patchset #50 id:980001 of https://codereview.chromium.org/1806313003/ )
Reason for revert:
Broke TSan: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Tests/builds/19362
WARNING: ThreadSanitizer: data race (pid=25863)
  Write of size 8 at 0x0000081b8060 by main thread:
    #0 media::AudioManagerDeleter::operator()(media::AudioManager const*) const media/audio/audio_manager.cc:264:20 (content_browsertests+0x000001e77078)
    #1 reset buildtools/third_party/libc++/trunk/include/memory:2735:13 (content_browsertests+0x000004e5f2af)
    #2 ~unique_ptr buildtools/third_party/libc++/trunk/include/memory:2703 (content_browsertests+0x000004e5f2af)
    #3 content::BrowserMainLoop::~BrowserMainLoop() content/browser/browser_main_loop.cc:428 (content_browsertests+0x000004e5f2af)
    #4 content::BrowserMainLoop::~BrowserMainLoop() content/browser/browser_main_loop.cc:424:37 (content_browsertests+0x000004e5f779)
    #5 operator() buildtools/third_party/libc++/trunk/include/memory:2529:13 (content_browsertests+0x000004e68120)
    #6 reset buildtools/third_party/libc++/trunk/include/memory:2735 (content_browsertests+0x000004e68120)
    #7 content::BrowserMainRunnerImpl::Shutdown() content/browser/browser_main_runner.cc:223 (content_browsertests+0x000004e68120)
    #8 ShellBrowserMain(content::MainFunctionParams const&, std::__1::unique_ptr<content::BrowserMainRunner, std::__1::default_delete<content::BrowserMainRunner> > const&) content/shell/browser/shell_browser_main.cc:33:3 (content_browsertests+0x000000c97ce7)
    #9 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:288:16 (content_browsertests+0x000000c8bec4)
    #10 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:368:25 (content_browsertests+0x000005558812)
    #11 content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:742:12 (content_browsertests+0x000005559511)
    #12 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:20:15 (content_browsertests+0x00000555797e)
    #13 content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:287:3 (content_browsertests+0x000000c55601)
    #14 content::ContentBrowserTest::SetUp() content/public/test/content_browser_test.cc:92:3 (content_browsertests+0x000000c5e8e6)
    #15 SetUp content/browser/renderer_host/render_widget_host_view_browsertest.cc:221:5 (content_browsertests+0x000000739618)
    #16 content::(anonymous namespace)::CompositingRenderWidgetHostViewBrowserTestTabCapture::SetUp() content/browser/renderer_host/render_widget_host_view_browsertest.cc:403 (content_browsertests+0x000000739618)
    #17 HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2458:12 (content_browsertests+0x000000f3168d)
    #18 testing::Test::Run() testing/gtest/src/gtest.cc:2470 (content_browsertests+0x000000f3168d)
    #19 testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5 (content_browsertests+0x000000f32833)
    #20 testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5 (content_browsertests+0x000000f33118)
    #21 testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:11 (content_browsertests+0x000000f3c562)
    #22 HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 (content_browsertests+0x000000f3bfc6)
    #23 testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255 (content_browsertests+0x000000f3bfc6)
    #24 RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10 (content_browsertests+0x000000d209eb)
    #25 base::TestSuite::Run() base/test/test_suite.cc:230 (content_browsertests+0x000000d209eb)
    #26 content::ContentTestLauncherDelegate::RunTestSuite(int, char**) content/test/content_test_launcher.cc:105:12 (content_browsertests+0x000000c677db)
    #27 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:517:12 (content_browsertests+0x000000cfb2fc)
    #28 main content/test/content_test_launcher.cc:131:10 (content_browsertests+0x000000c67762)
  Previous read of size 8 at 0x0000081b8060 by thread T15:
    #0 media::(anonymous namespace)::AudioManagerHelper::UpdateLastAudioThreadTimeTick() media/audio/audio_manager.cc:179:5 (content_browsertests+0x000001e77986)
    #1 Run<> base/bind_internal.h:181:12 (content_browsertests+0x000001e77df2)
    #2 MakeItSo<media::(anonymous namespace)::AudioManagerHelper *> base/bind_internal.h:321 (content_browsertests+0x000001e77df2)
    #3 base::internal::Invoker<base::IndexSequence<0ul>, base::internal::BindState<base::internal::RunnableAdapter<void (media::(anonymous namespace)::AudioManagerHelper::*)()>, void (media::(anonymous namespace)::AudioManagerHelper*), base::internal::UnretainedWrapper<media::(anonymous namespace)::AudioManagerHelper> >, base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void (media::(anonymous namespace)::AudioManagerHelper::*)()> >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:372 (content_browsertests+0x000001e77df2)
    #4 Run base/callback.h:397:12 (content_browsertests+0x00000111aa7d)
    #5 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) base/debug/task_annotator.cc:51 (content_browsertests+0x00000111aa7d)
    #6 base::MessageLoop::RunTask(base::PendingTask const&) base/message_loop/message_loop.cc:479:3 (content_browsertests+0x0000010958e6)
    #7 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop/message_loop.cc:488:5 (content_browsertests+0x000001095ecd)
    #8 base::MessageLoop::DoDelayedWork(base::TimeTicks*) base/message_loop/message_loop.cc:638:10 (content_browsertests+0x00000109662a)
    #9 base::MessagePumpDefault::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_default.cc:37:17 (content_browsertests+0x000001099875)
    #10 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:443:3 (content_browsertests+0x00000109528b)
    #11 base::RunLoop::Run() base/run_loop.cc:35:3 (content_browsertests+0x0000010b5cc6)
    #12 base::MessageLoop::Run() base/message_loop/message_loop.cc:295:3 (content_browsertests+0x000001094a95)
    #13 base::Thread::Run(base::MessageLoop*) base/threading/thread.cc:202:3 (content_browsertests+0x0000010de279)
    #14 base::Thread::ThreadMain() base/threading/thread.cc:254:3 (content_browsertests+0x0000010de449)
    #15 base::(anonymous namespace)::ThreadFunc(void*) base/threading/platform_thread_posix.cc:70:3 (content_browsertests+0x0000010d88bd)
  Location is global 'media::(anonymous namespace)::g_last_created' of size 8 at 0x0000081b8060 (content_browsertests+0x0000081b8060)
  Thread T15 'AudioThread' (tid=25884, running) created by main thread at:
    #0 pthread_create <null> (content_browsertests+0x0000004a15c5)
    #1 base::(anonymous namespace)::CreateThread(unsigned long, bool, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:109:13 (content_browsertests+0x0000010d860a)
    #2 base::PlatformThread::CreateWithPriority(unsigned long, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:190:10 (content_browsertests+0x0000010d8515)
    #3 base::Thread::StartWithOptions(base::Thread::Options const&) base/threading/thread.cc:116:10 (content_browsertests+0x0000010ddfa0)
    #4 base::Thread::Start() base/threading/thread.cc:86:10 (content_browsertests+0x0000010ddde4)
    #5 content::BrowserMainLoop::CreateAudioManager() content/browser/browser_main_loop.cc:1468:3 (content_browsertests+0x000004e665c6)
    #6 content::BrowserMainLoop::BrowserThreadsStarted() content/browser/browser_main_loop.cc:1228:5 (content_browsertests+0x000004e633b5)
    #7 Run<> base/bind_internal.h:181:12 (content_browsertests+0x000004e67612)
    #8 MakeItSo<content::BrowserMainLoop *> base/bind_internal.h:313 (content_browsertests+0x000004e67612)
    #9 base::internal::Invoker<base::IndexSequence<0ul>, base::internal::BindState<base::internal::RunnableAdapter<int (content::BrowserMainLoop::*)()>, int (content::BrowserMainLoop*), base::internal::UnretainedWrapper<content::BrowserMainLoop> >, base::internal::InvokeHelper<false, int, base::internal::RunnableAdapter<int (content::BrowserMainLoop::*)()> >, int ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:372 (content_browsertests+0x000004e67612)
    #10 Run base/callback.h:397:12 (content_browsertests+0x0000050dd941)
    #11 content::StartupTaskRunner::RunAllTasksNow() content/browser/startup_task_runner.cc:45 (content_browsertests+0x0000050dd941)
    #12 content::BrowserMainLoop::CreateStartupTasks() content/browser/browser_main_loop.cc:803:3 (content_browsertests+0x000004e622db)
    #13 content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) content/browser/browser_main_runner.cc:139:5 (content_browsertests+0x000004e67c63)
    #14 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:19 (content_browsertests+0x000000c97ca7)
    #15 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:288:16 (content_browsertests+0x000000c8bec4)
    #16 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:368:25 (content_browsertests+0x000005558812)
    #17 content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:742:12 (content_browsertests+0x000005559511)
    #18 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:20:15 (content_browsertests+0x00000555797e)
    #19 content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:287:3 (content_browsertests+0x000000c55601)
    #20 content::ContentBrowserTest::SetUp() content/public/test/content_browser_test.cc:92:3 (content_browsertests+0x000000c5e8e6)
    #21 SetUp content/browser/renderer_host/render_widget_host_view_browsertest.cc:221:5 (content_browsertests+0x000000739618)
    #22 content::(anonymous namespace)::CompositingRenderWidgetHostViewBrowserTestTabCapture::SetUp() content/browser/renderer_host/render_widget_host_view_browsertest.cc:403 (content_browsertests+0x000000739618)
    #23 HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2458:12 (content_browsertests+0x000000f3168d)
    #24 testing::Test::Run() testing/gtest/src/gtest.cc:2470 (content_browsertests+0x000000f3168d)
    #25 testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5 (content_browsertests+0x000000f32833)
    #26 testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5 (content_browsertests+0x000000f33118)
    #27 testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:11 (content_browsertests+0x000000f3c562)
    #28 HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 (content_browsertests+0x000000f3bfc6)
    #29 testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255 (content_browsertests+0x000000f3bfc6)
    #30 RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10 (content_browsertests+0x000000d209eb)
    #31 base::TestSuite::Run() base/test/test_suite.cc:230 (content_browsertests+0x000000d209eb)
    #32 content::ContentTestLauncherDelegate::RunTestSuite(int, char**) content/test/content_test_launcher.cc:105:12 (content_browsertests+0x000000c677db)
    #33 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:517:12 (content_browsertests+0x000000cfb2fc)
    #34 main content/test/content_test_launcher.cc:131:10 (content_browsertests+0x000000c67762)
SUMMARY: ThreadSanitizer: data race media/audio/audio_manager.cc:264:20 in media::AudioManagerDeleter::operator()(media::AudioManager const*) const
Original issue's description:
> Pass task runners to AudioManager constructor.
>
> This patch replaces the internal AudioManagerBase::audio_thread with
> an external task runner. The old implementation made it harder to test
> and override the internal audio thread.
>
> AudioManagerBase::Shutdown had to complete on the audio thread before
> the AudioManager instance could be deleted. It relied on Thread::Stop
> on the main thread to wait for the audio thread to shutdown. A subclass
> using a different thread shared with other modules could not do that.
>
> Passing a task runner into AudioManager and making GetTaskRunner
> non-virtual ensures that the task runner will not change for the
> lifetime of AudioManager instance.
>
> BUG=594234
>
> Committed: https://crrev.com/e05981bd06b4694b426990acd41be8ab6d9a1659
> Cr-Commit-Position: refs/heads/master@{#388048}
TBR=dalecurtis@chromium.org,tommi@chromium.org,xhwang@chromium.org,rkc@chromium.org,jam@chromium.org,ckehoe@chromium.org,alokp@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=594234
Committed: https://crrev.com/78655ccbbd314f7ceb4133cd0d395dd35502fdf9
Cr-Commit-Position: refs/heads/master@{#388096}
   
  Patch Set 1 #Messages
    Total messages: 6 (2 generated)
     
  
  
       |