1403041 - MSG reenters libcubeb from within data_callback, resulting in potential deadlocks

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Description

[Matthew Gregan [:kinetik]]

MediaStreamGraphImpl::CloseAudioInputImpl creates a new AudioCallbackDriver which calls MediaStreamGraphImpl::AudioChannelCount from it's constructor., which results in a call to cubeb_get_max_channel_count.

While this happens to work with at least some of the current backends, it's not safe to do in general and the libcubeb API is not intended to be called reentrantly from within callbacks.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bug 1403041 - Fix typo in AudioCallbackDriver member.

s/mOuputChannelCount/mOutputChannelCount/

Review commit: https://reviewboard.mozilla.org/r/183598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183598/

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1403041 - Don't re-enter libcubeb from the audio thread.

This delays initializing the channel count until actually initializing the
cubeb stream for an AudioCallbackDriver.

The constructor of an AudioCallbackDriver is usually called from the previous
driver's thread (be it a system thread or the audio callback, the exception
being when it's the first time a GraphDriver is being created), while the
AudioCallbackDriver::Init method is always called from a special thread that is
specially created to run potentially long and blocking operations, such as
creating an audio stream.

Review commit: https://reviewboard.mozilla.org/r/183600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183600/

Comment 3

[Alex Chronopoulos [:achronop]]

Attachment: Overload operator= in SpillBuffer

An optional small addition since the patches above make things more clear and well looking.

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8912229 [details]
Bug 1403041 - Fix typo in AudioCallbackDriver member.

https://reviewboard.mozilla.org/r/183598/#review188926

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8912230 [details]
Bug 1403041 - Don't re-enter libcubeb from the audio thread.

https://reviewboard.mozilla.org/r/183600/#review188976

Comment 6

[Matthew Gregan [:kinetik]]

Thanks for fixing this!

Comment 7

[Paul Adenot (:padenot)]

I'm trying to think of a strategy to prevent this from happening in the future.

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to Paul Adenot (:padenot) from comment #7)
> I'm trying to think of a strategy to prevent this from happening in the
> future.

I added asserts to the remoting to stop this issue being reintroduced.  We could do the same thing in regular libcubeb at some point.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1962479ed96
Fix typo in AudioCallbackDriver member. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/65ff7c0adadc
Don't re-enter libcubeb from the audio thread. r=kinetik

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b1962479ed96
https://hg.mozilla.org/mozilla-central/rev/65ff7c0adadc