Avoid deadlock between Pipeline and RendererImpl.

Avoid deadlock between Pipeline and RendererImpl.

Both Pipeline and RendererImpl use it's own lock. We could end up with a dead
lock in the following scenario:
- On main thread, Pipeline holds it's lock (lock A) and calls into RendererImpl,
  which also requires the RendererImpl's lock (lock B).
- On media thread, RendererImpl holds it's lock B and calls callback provided by
  the Pipeline, which requires lock A.

This CL makes sure Pipeline never calls into RendererImpl while holding lock A
and RendererImpl nevers calls (back) into Pipeline while holding lock B. So
deadlock should not happen.

This CL also reverts 3 CLs that disabled various tests due to the bug:
- Revert "Exclude tests that deadlock under DrMemory (and Tsan)"
   This reverts commit 0e014b9ad964236d67db60488168dbc0811655e9.
- Revert "Disable deadlock-y tests in TSan instead of just suppressing the error."
   This reverts commit f712d106692d782f2a590a8e752f720ad76ee608.
- Revert "Suppress a deadlock report through media::Pipeline::GetMediaTime."
   This reverts commit 5c72e0f67ce7e42804d809183bdae190024056ef.

BUG=407452
TEST=Ran PipelineIntegrationTest.ChunkDemuxerAbortRead_VideoOnly in a TSAN build for 10 mins and didn't see any issue.

Committed: https://crrev.com/7def98aadf7cac26952eae1fb3294f56510d67d7
Cr-Commit-Position: refs/heads/master@{#292332}

[xhwang, 2014-08-27 23:26:52]

xhwang@chromium.org changed reviewers:
+ scherkus@chromium.org

[xhwang, 2014-08-27 23:26:52]

I am making TSAN build to verify the fix. Meanwhile, PTAL!

[scherkus (not reviewing), 2014-08-27 23:41:11]

https://codereview.chromium.org/512973002/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/512973002/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:293: duration_ = duration;
I'm pretty sure this runs on the media thread

It seems silly that we both:
  a) store the duration and have a getter under lock
  b) fire a callback that the duration has changed

We also pass the locked version when initializing Renderer. It'd be good to
stick to a single idiom (polling for duration w/ lock OR callback that contains
the new duration) ... but not both.

Any thoughts whether cleaning that up would also fix the deadlock?

[xhwang, 2014-08-28 00:46:28]

https://codereview.chromium.org/512973002/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/512973002/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:293: duration_ = duration;
On 2014/08/27 23:41:11, scherkus wrote:
> I'm pretty sure this runs on the media thread
> 
> It seems silly that we both:
>   a) store the duration and have a getter under lock
>   b) fire a callback that the duration has changed
> 
> We also pass the locked version when initializing Renderer. It'd be good to
> stick to a single idiom (polling for duration w/ lock OR callback that
contains
> the new duration) ... but not both.
> 
> Any thoughts whether cleaning that up would also fix the deadlock?

That's true for FFmepgDemuxer, but not for ChunkDemuxer:

#0 0x7f6713187c1e base::debug::StackTrace::StackTrace()
#1 0x7f671321ee55 logging::LogMessage::~LogMessage()
#2 0x7f670aedcec0 media::Pipeline::SetDuration()
#3 0x7f670af64dd6 media::ChunkDemuxer::UpdateDuration()
#4 0x7f670af611ae media::ChunkDemuxer::OnSourceInitDone()
#5 0x7f670af6b358 base::internal::RunnableAdapter<>::Run()
#6 0x7f670af6b289 base::internal::InvokeHelper<>::MakeItSo()
#7 0x7f670af6b207 base::internal::Invoker<>::Run()
#8 0x7f670af6680b base::Callback<>::Run()
#9 0x7f670af58fb2 media::SourceState::OnSourceInitDone()
#10 0x7f670af710d8 base::internal::RunnableAdapter<>::Run()
#11 0x7f670af71029 base::internal::InvokeHelper<>::MakeItSo()
#12 0x7f670af70fc7 base::internal::Invoker<>::Run()
#13 0x7f670af6680b base::Callback<>::Run()
#14 0x7f670b09e769 media::WebMStreamParser::ParseInfoAndTracks()
#15 0x7f670b09d9af media::WebMStreamParser::Parse()
#16 0x7f670af5ad9a media::SourceState::Append()
#17 0x7f670af61dff media::ChunkDemuxer::AppendData()
#18 0x7f670854aceb content::WebSourceBufferImpl::append()
#19 0x7f670c419d25 blink::SourceBuffer::appendBufferAsyncPart()
#20 0x7f670c41f537 blink::AsyncMethodRunner<>::fired()
#21 0x7f670c41f66f blink::Timer<>::fired()
#22 0x7f67155c8260 blink::ThreadTimers::sharedTimerFiredInternal()
#23 0x7f67155c7e95 blink::ThreadTimers::sharedTimerFired()
#24 0x7f671583e318 blink::Scheduler::tickSharedTimer()
#25 0x7f671583e781 blink::Scheduler::sharedTimerAdapter()
#26 0x7f6707ffad43 content::BlinkPlatformImpl::DoTimeout()
#27 0x7f6707ffe162 base::internal::RunnableAdapter<>::Run()
#28 0x7f6707ffe0cc base::internal::InvokeHelper<>::MakeItSo()
#29 0x7f6707ffe07a base::internal::Invoker<>::Run()
#30 0x7f671317044e base::Callback<>::Run()
#31 0x7f67133289e6 base::Timer::RunScheduledTask()
#32 0x7f6713328b1c base::BaseTimerTaskInternal::Run()
#33 0x7f6713328de2 base::internal::RunnableAdapter<>::Run()
#34 0x7f6713328d4c base::internal::InvokeHelper<>::MakeItSo()
#35 0x7f6713328cf5 base::internal::Invoker<>::Run()
#36 0x7f671317044e base::Callback<>::Run()
#37 0x7f671318e083 base::debug::TaskAnnotator::RunTask()
#38 0x7f6713244615 base::MessageLoop::RunTask()
#39 0x7f671324475b base::MessageLoop::DeferOrRunPendingTask()
#40 0x7f6713244985 base::MessageLoop::DoWork()
#41 0x7f6713255408 base::MessagePumpDefault::Run()
#42 0x7f6713244270 base::MessageLoop::RunHandler()
#43 0x7f67132a9ce2 base::RunLoop::Run()
#44 0x7f6713243b11 base::MessageLoop::Run()
#45 0x7f67086088f5 content::RendererMain()
#46 0x7f6707555bd6 content::RunZygote()
#47 0x7f6707555f09 content::RunNamedProcessTypeMain()
#48 0x7f670755a078 content::ContentMainRunnerImpl::Run()
#49 0x7f6707555425 content::ContentMain()
#50 0x7f6717921345 ChromeMain
#51 0x7f67179212f2 main
#52 0x7f670097078d __libc_start_main
#53 0x7f67179211f9 <unknown>

[scherkus (not reviewing), 2014-08-28 00:51:18]

On 2014/08/28 00:46:28, xhwang wrote:
> https://codereview.chromium.org/512973002/diff/1/media/base/pipeline.cc
> File media/base/pipeline.cc (right):
> 
>
https://codereview.chromium.org/512973002/diff/1/media/base/pipeline.cc#newco...
> media/base/pipeline.cc:293: duration_ = duration;
> On 2014/08/27 23:41:11, scherkus wrote:
> > I'm pretty sure this runs on the media thread
> > 
> > It seems silly that we both:
> >   a) store the duration and have a getter under lock
> >   b) fire a callback that the duration has changed
> > 
> > We also pass the locked version when initializing Renderer. It'd be good to
> > stick to a single idiom (polling for duration w/ lock OR callback that
> contains
> > the new duration) ... but not both.
> > 
> > Any thoughts whether cleaning that up would also fix the deadlock?
> 
> That's true for FFmepgDemuxer, but not for ChunkDemuxer:

Well that's annoying! Especially since
blink::SourceBuffer::appendBufferAsyncPart() already makes things async and
_could_ be done on the media thread, but that's a much more involved change :(

lgtm as is

[xhwang, 2014-08-28 04:07:21]

xhwang@chromium.org changed reviewers:
+ jyasskin@google.com

[xhwang, 2014-08-28 04:07:21]

jyasskin: I reenabled the disabled tests. I tested one of the tests with and
without my CL and my CL fixed the test.

[xhwang, 2014-08-28 04:18:30]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-08-28 04:19:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/512973002/20001

[Jeffrey Yasskin, 2014-08-28 05:17:48]

On 2014/08/28 04:07:21, xhwang wrote:
> jyasskin: I reenabled the disabled tests. I tested one of the tests with and
> without my CL and my CL fixed the test.

Thanks for including the re-enabling in your change.

[commit-bot: I haz the power, 2014-08-28 05:25:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-28 05:49:25]

Committed patchset #2 (id:20001) as 3b2dff86be7304ee4b6710ce2e0f1a47fbd858c1

[commit-bot: I haz the power, 2014-09-10 02:57:57]

Patchset 2 (id:??) landed as
https://crrev.com/7def98aadf7cac26952eae1fb3294f56510d67d7
Cr-Commit-Position: refs/heads/master@{#292332}