1406830 - race setting AsyncCubebTask::sThreadPool

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

The first AsyncCubebTask dispatch from AudioCallbackDriver::Start() may either
be from MediaStreamGraphImpl::RunInStableState() on the main thread or
ThreadedDriver::RunThread() on a threaded driver thread.  These could
potentially occur concurrently when there are multiple MediaStreamGraphs.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: bug 1406830 remove unused RemoveMixerCallback

Unused since
https://hg.mozilla.org/mozilla-central/rev/ef761a32a969178df81a61bc2d0a376dde2eec03#l1.144

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

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: bug 1406830 remove unused GraphDriver::Resume()

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

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: bug 1406830 reimplement Shutdown() as virtual separately on each driver

moving Stop() to AudioCallbackDriver only.

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

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: bug 1406830 store the AsyncCubeTask SharedThreadPool reference on the AudioCallbackDriver

The first AsyncCubebTask dispatch from AudioCallbackDriver::Start() may either
be from MediaStreamGraphImpl::RunInStableState() on the main thread or
ThreadedDriver::RunThread() on a threaded driver thread.
These could potentially occur concurrently when there are multiple
MediaStreamGraphs.

This change removes the race around setting sThreadPool.
SharedThreadPool::Get() would have returned the same pointer, and so
that race was probably mostly benign apart from the potential to add an
extra reference and so hang on shutdown in SharedThreadPool::SpinUntilEmpty().

Storing the reference to the SharedThreadPool on the object using it is the
typical way to use SharedThreadPool.  It lets the thread pool be released when
not in use, and lets SharedThreadPool deal with multi-thread access and
shutdown.

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

Comment 5

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d388956e7dcf1f14e34b5ddaf0aa4182223a988e

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8916496 [details]
bug 1406830 remove unused RemoveMixerCallback

https://reviewboard.mozilla.org/r/187622/#review192710

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8916499 [details]
bug 1406830 store the AsyncCubeTask SharedThreadPool reference on the AudioCallbackDriver

https://reviewboard.mozilla.org/r/187628/#review192712

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8916496 [details]
bug 1406830 remove unused RemoveMixerCallback

https://reviewboard.mozilla.org/r/187622/#review192714

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8916497 [details]
bug 1406830 remove unused GraphDriver::Resume()

https://reviewboard.mozilla.org/r/187624/#review192716

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8916498 [details]
bug 1406830 reimplement Shutdown() as virtual separately on each driver

https://reviewboard.mozilla.org/r/187626/#review192718

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8916499 [details]
bug 1406830 store the AsyncCubeTask SharedThreadPool reference on the AudioCallbackDriver

https://reviewboard.mozilla.org/r/187628/#review192720

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93727ab4586d
remove unused RemoveMixerCallback r=padenot
https://hg.mozilla.org/integration/autoland/rev/5496bf169434
remove unused GraphDriver::Resume() r=padenot
https://hg.mozilla.org/integration/autoland/rev/a19b9ada34d2
reimplement Shutdown() as virtual separately on each driver r=padenot
https://hg.mozilla.org/integration/autoland/rev/426e40fa5d9c
store the AsyncCubeTask SharedThreadPool reference on the AudioCallbackDriver r=jesup,padenot

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/93727ab4586d
https://hg.mozilla.org/mozilla-central/rev/5496bf169434
https://hg.mozilla.org/mozilla-central/rev/a19b9ada34d2
https://hg.mozilla.org/mozilla-central/rev/426e40fa5d9c