Fixing deadlock by removing unnesessary lock in AudioRendererMixer

Fixing deadlock by removing unnesessary lock in AudioRendererMixer

Deadlock is like this:

On main or media thread:
ARMM::GetOutputDeviceInfo [takes |lock_|]
-> AOD::GetOutputDeviceInfo -> waits for device authorization, which can be signalled on IO thread only

On IO thread:
AOD::CreateStreamOnIOThread/OnStateChanged*
-> ARMM::OnRenderError [takes |lock_|],
so OnDeviceAuthorized/OnIPCClosed will never be called and device authorization will never be signalled.

*AOD::OnDeviceAuthorized itself does not deadlock it always signals event before calling OnRenderError()

That lock in ARMM::GetOutputDeviceInfo is never needed, after all

BUG=615589
Committed: https://crrev.com/c017b5d078463e7265b5bef1d34a25a57859293c
Cr-Commit-Position: refs/heads/master@{#400735}

[o1ka, 2016-06-20 17:07:15]

Description was changed from

==========
Fixing deadlock by removing unnesessary lock in AudioRendererMixer

Deadlock is like this:

On main or media thread:
ARMM::GetOutputDeviceInfo [takes |lock_|]
-> AOD::GetOutputDeviceInfo -> waits for device authorization, which can be
signalled on IO thread only

On IO thread:
AOD::CreateStreamOnIOThread/OnStateChanged*
-> ARMM::OnRenderError [takes |lock_|],
so OnDeviceAuthorized/OnIPCClosed will never be called and device authorization
will never be signalled.

*AOD::OnDeviceAuthorized itself does not deadlock it always signals event before
calling OnRenderError()

That lock in ARMM::GetOutputDeviceInfo is never needed, after all

BUG=615589
==========

to

==========
Fixing deadlock by removing unnesessary lock in AudioRendererMixer

Deadlock is like this:

On main or media thread:
ARMM::GetOutputDeviceInfo [takes |lock_|]
-> AOD::GetOutputDeviceInfo -> waits for device authorization, which can be
signalled on IO thread only

On IO thread:
AOD::CreateStreamOnIOThread/OnStateChanged*
-> ARMM::OnRenderError [takes |lock_|],
so OnDeviceAuthorized/OnIPCClosed will never be called and device authorization
will never be signalled.

*AOD::OnDeviceAuthorized itself does not deadlock it always signals event before
calling OnRenderError()

That lock in ARMM::GetOutputDeviceInfo is never needed, after all

BUG=615589
==========

[o1ka, 2016-06-20 17:07:15]

olka@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[o1ka, 2016-06-20 17:08:23]

Dale, PTAL.

[DaleCurtis, 2016-06-20 17:15:38]

lgtm

[DaleCurtis, 2016-06-20 17:15:45]

Nice find!

[DaleCurtis, 2016-06-20 17:16:17]

(Sorry, should have caught this during review, we should almost never call into
sinks under lock)

[o1ka, 2016-06-20 17:18:10]

The CQ bit was checked by olka@chromium.org

[commit-bot: I haz the power, 2016-06-20 17:18:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2085603002/1

[o1ka, 2016-06-20 17:22:26]

On 2016/06/20 17:16:17, DaleCurtis wrote:
> (Sorry, should have caught this during review, we should almost never call
into
> sinks under lock)

Well, I'm kind of an author of this deadlock, so it's my fault.

Actually, we do call Start/Play under lock pretty often (and here as well), but
ARM is the only case where the lock is held in OnRenderError() => acquired on IO
thread, so it slipped through.

[DaleCurtis, 2016-06-20 17:40:28]

On 2016/06/20 at 17:22:26, olka wrote:
> On 2016/06/20 17:16:17, DaleCurtis wrote:
> > (Sorry, should have caught this during review, we should almost never call
into
> > sinks under lock)
> 
> Well, I'm kind of an author of this deadlock, so it's my fault.
> 
> Actually, we do call Start/Play under lock pretty often (and here as well),
but ARM is the only case where the lock is held in OnRenderError() => acquired
on IO thread, so it slipped through.

We don't call Start/Play under lock in this class, nor any of the other classes
under media that I'm aware of -- for exactly this reason, we can hit deadlock.

[o1ka, 2016-06-20 17:46:10]

On 2016/06/20 17:40:28, DaleCurtis wrote:
> On 2016/06/20 at 17:22:26, olka wrote:
> > On 2016/06/20 17:16:17, DaleCurtis wrote:
> > > (Sorry, should have caught this during review, we should almost never call
> into
> > > sinks under lock)
> > 
> > Well, I'm kind of an author of this deadlock, so it's my fault.
> > 
> > Actually, we do call Start/Play under lock pretty often (and here as well),
> but ARM is the only case where the lock is held in OnRenderError() => acquired
> on IO thread, so it slipped through.
> 
> We don't call Start/Play under lock in this class, nor any of the other
classes
> under media that I'm aware of -- for exactly this reason, we can hit deadlock.

Here, for example
https://cs.chromium.org/chromium/src/media/base/audio_renderer_mixer.cc?q=Aud....
We don't call Stop() under lock, because it deadlocks with Render() call if it
holds the same lock. Start/Play are asynchronous, so they should be safe.

[DaleCurtis, 2016-06-20 17:55:48]

On 2016/06/20 at 17:46:10, olka wrote:
> On 2016/06/20 17:40:28, DaleCurtis wrote:
> > On 2016/06/20 at 17:22:26, olka wrote:
> > > On 2016/06/20 17:16:17, DaleCurtis wrote:
> > > > (Sorry, should have caught this during review, we should almost never
call
> > into
> > > > sinks under lock)
> > > 
> > > Well, I'm kind of an author of this deadlock, so it's my fault.
> > > 
> > > Actually, we do call Start/Play under lock pretty often (and here as
well),
> > but ARM is the only case where the lock is held in OnRenderError() =>
acquired
> > on IO thread, so it slipped through.
> > 
> > We don't call Start/Play under lock in this class, nor any of the other
classes
> > under media that I'm aware of -- for exactly this reason, we can hit
deadlock.
> 
> Here, for example
https://cs.chromium.org/chromium/src/media/base/audio_renderer_mixer.cc?q=Aud....
> We don't call Stop() under lock, because it deadlocks with Render() call if it
holds the same lock. Start/Play are asynchronous, so they should be safe.

Ah, yes you're correct. I think previously we have assumed even Play/Pause
aren't safe, but they are. Will send you a patch for that.

[commit-bot: I haz the power, 2016-06-20 18:34:44]

Description was changed from

==========
Fixing deadlock by removing unnesessary lock in AudioRendererMixer

Deadlock is like this:

On main or media thread:
ARMM::GetOutputDeviceInfo [takes |lock_|]
-> AOD::GetOutputDeviceInfo -> waits for device authorization, which can be
signalled on IO thread only

On IO thread:
AOD::CreateStreamOnIOThread/OnStateChanged*
-> ARMM::OnRenderError [takes |lock_|],
so OnDeviceAuthorized/OnIPCClosed will never be called and device authorization
will never be signalled.

*AOD::OnDeviceAuthorized itself does not deadlock it always signals event before
calling OnRenderError()

That lock in ARMM::GetOutputDeviceInfo is never needed, after all

BUG=615589
==========

to

==========
Fixing deadlock by removing unnesessary lock in AudioRendererMixer

Deadlock is like this:

On main or media thread:
ARMM::GetOutputDeviceInfo [takes |lock_|]
-> AOD::GetOutputDeviceInfo -> waits for device authorization, which can be
signalled on IO thread only

On IO thread:
AOD::CreateStreamOnIOThread/OnStateChanged*
-> ARMM::OnRenderError [takes |lock_|],
so OnDeviceAuthorized/OnIPCClosed will never be called and device authorization
will never be signalled.

*AOD::OnDeviceAuthorized itself does not deadlock it always signals event before
calling OnRenderError()

That lock in ARMM::GetOutputDeviceInfo is never needed, after all

BUG=615589
==========

[commit-bot: I haz the power, 2016-06-20 18:34:45]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-20 18:39:53]

Description was changed from

==========
Fixing deadlock by removing unnesessary lock in AudioRendererMixer

Deadlock is like this:

On main or media thread:
ARMM::GetOutputDeviceInfo [takes |lock_|]
-> AOD::GetOutputDeviceInfo -> waits for device authorization, which can be
signalled on IO thread only

On IO thread:
AOD::CreateStreamOnIOThread/OnStateChanged*
-> ARMM::OnRenderError [takes |lock_|],
so OnDeviceAuthorized/OnIPCClosed will never be called and device authorization
will never be signalled.

*AOD::OnDeviceAuthorized itself does not deadlock it always signals event before
calling OnRenderError()

That lock in ARMM::GetOutputDeviceInfo is never needed, after all

BUG=615589
==========

to

==========
Fixing deadlock by removing unnesessary lock in AudioRendererMixer

Deadlock is like this:

On main or media thread:
ARMM::GetOutputDeviceInfo [takes |lock_|]
-> AOD::GetOutputDeviceInfo -> waits for device authorization, which can be
signalled on IO thread only

On IO thread:
AOD::CreateStreamOnIOThread/OnStateChanged*
-> ARMM::OnRenderError [takes |lock_|],
so OnDeviceAuthorized/OnIPCClosed will never be called and device authorization
will never be signalled.

*AOD::OnDeviceAuthorized itself does not deadlock it always signals event before
calling OnRenderError()

That lock in ARMM::GetOutputDeviceInfo is never needed, after all

BUG=615589

Committed: https://crrev.com/c017b5d078463e7265b5bef1d34a25a57859293c
Cr-Commit-Position: refs/heads/master@{#400735}
==========

[commit-bot: I haz the power, 2016-06-20 18:39:55]

Patchset 1 (id:??) landed as
https://crrev.com/c017b5d078463e7265b5bef1d34a25a57859293c
Cr-Commit-Position: refs/heads/master@{#400735}