1350511 - Add a deadlock test in cubeb to reproduce the known issue and prevent it happens again

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, enhancement, P2)

Description

[C.M.Chang[:chunmin]]

Attachment: WIP

In CoreAudio, the data callback will holds a mutex shared with AudioUnit.
Thus, if the callback request another mutex M(resource) held by the another function, without releasing its holding one, then it will cause a deadlock when the another function, which holds the mutex M, request to use AudioUnit.

we illustrate a simple version of the deadlock in bug 1337805 as follows:

                   holds
  data_callback <---------- mutext_AudioUnit
       |                            ^
       |                            |
       | request                    | request
       |                            |
       v           holds            |
  mutex_cubeb ------------> get_channel_layout

In this example, the "audiounit_get_channel_layout" in f4edfb8[0,1] requests to create an AudioUnit when it holds a mutex of cubeb context[2]. Meanwhile, the data callback who holds the mutex shared with AudioUnit requests for the mutex of cubeb context. So, it causes a deadlock.
 
The problem is solve by pull 236[3]. However, to prevent it happens again, we should add the test in case who without such knowledge misuses the AudioUnit in get_channel_layout.

[0] https://github.com/kinetiknz/cubeb/blob/f4edfb8eea920887713325e44773f3a2d959860c/src/cubeb_audiounit.cpp#L2725
[1] https://hg.mozilla.org/try/rev/ebf6afd688f10c9f570f9d59bef89c5fffcf2270#l4.500
[2] https://hg.mozilla.org/try/file/ebf6afd688f10c9f570f9d59bef89c5fffcf2270/dom/media/CubebUtils.cpp#l226
[3] https://github.com/kinetiknz/cubeb/pull/236

Comment 1

[C.M.Chang[:chunmin]]

I summarized what I know from bug 1337805. 
The following figure is my understanding of the deadlock in the bug:


     request               request          holds                request
37 - - - - - > sMutex < - - - - - - - 40 <---------- mMonitor < - - - - - - 38
                 |                     ^
                 |                     |
           holds |               holds |
                 |                     |
                 v    request          |
                41 - - - - - - > mutex_AudioUnit



From the log[0, 1], the threads 37, 38, 40, 41 are stuck. The above figure is derived from their code flow below. 

Thread 37:
DecodedAudioDataSink::InitializeAudioStream => AudioStream::Init[2] => CubebUtils::GetCubebContext[3] => Request sMutex[4]

Thread 38:
AudioStream::GetPosition() => Request mMonitor[5]

Thread 40:
Call from 
AudioStream::DataCallback => Holds mMonitor[6]
AudioStream::DataCallback => CubebUtils::GetCubebContext()[7] => Request sMutex[8]

AudioStream::DataCallback is called from DataCallback_S[9], which is registered in cubeb_stream_init[10].
The DataCallback_S will be fired from audiounit_output_callback[11], that holds a mutex shared by AudioUnit inside.

Thread 41:
CubebUtils::PreferredChannelMap => CubebUtils::InitPreferredChannelLayout()[12] => Holds sMutex[13]
CubebUtils::InitPreferredChannelLayout() => cubeb_get_preferred_channel_layout[14] => audiounit_get_channel_layout => audiounit_create_unit[15, 16] => AudioComponentInstanceNew[17] => Request a mutex shared by AudioUnit inside.
(Notice that there is a CoreAudio!HALB_Mutex::Lock() for this thread in the log[1].)


[0] https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a3faba8c399138e0f90ab561ee1dc533ebbffa23&selectedJob=75944851
[1] https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-macosx64/1486642041/autoland_yosemite_r7_test-mochitest-gl-3-bm134-tests1-macosx-build158.txt.gz
[2] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/mediasink/DecodedAudioDataSink.cpp#208
[3] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/AudioStream.cpp#351
[4] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/CubebUtils.cpp#192
[5] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/AudioStream.cpp#480
[6] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/AudioStream.cpp#598
[7] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/AudioStream.cpp#604
[8] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/CubebUtils.cpp#192
[9] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/AudioStream.h#260
[10] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/AudioStream.cpp#374
[11] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/media/libcubeb/src/cubeb_audiounit.cpp#438
[12] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/CubebUtils.cpp#270
[13] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/CubebUtils.cpp#226
[14] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/dom/media/CubebUtils.cpp#234
[15] https://github.com/kinetiknz/cubeb/blob/f4edfb8eea920887713325e44773f3a2d959860c/src/cubeb_audiounit.cpp#L2725
[16] https://hg.mozilla.org/try/rev/ebf6afd688f10c9f570f9d59bef89c5fffcf2270#l4.500
[17] https://github.com/kinetiknz/cubeb/blob/f4edfb8eea920887713325e44773f3a2d959860c/src/cubeb_audiounit.cpp#L1251

Comment 2

[C.M.Chang[:chunmin]]

Per comment above, I plan to reproduce the deadlock between thread 40 and 41. The WIP patch(attachment 8851187 [details]) is uploaded.

BTW, I think it's better save the backend_id when initializing AudioStream, instead of requesting sMutex to get it every time[0]. Or it will holds a resource(mMonitor) and then request another one(sMutex). It might cause a potential deadlock.

[0] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/media/AudioStream.cpp#604

Comment 3

[C.M.Chang[:chunmin]]

Attachment: draft

to-do:
- Use RAII to unlock mutex holding by killed thread

Comment 4

[C.M.Chang[:chunmin]]

Attachment: pull on github

Comment 5

[C.M.Chang[:chunmin]]

Merged on github