1406988 - Assert MediaManager invariant where applicable, and check for existence everywhere else.

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P3)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This is mostly for maintenance and cleanup on shutdown. Some of the existing checks might in theory re-launch MediaManager on shutdown, which would cause a MOZ_RELEASE_ASSERT [1].

I did a cleanup pass on this when looking at bug 1397528, but turned out to be unrelated, so I'm opening this bug instead to land it.

[1] http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/MediaManager.cpp#1865

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1406988 - Assert MediaManager invariant where applicable, and check for existence everywhere else.

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

Comment 2

[Andreas Pehrson [:pehrsons] (PTO; back in Jan)]

Comment on attachment 8916683 [details]
Bug 1406988 - Assert MediaManager invariant where applicable, and check for existence everywhere else.

https://reviewboard.mozilla.org/r/187776/#review193064

::: dom/media/MediaManager.cpp:1738
(Diff revision 1)
>      if (fakeCams || fakeMics) {
>        fakeBackend = new MediaEngineDefault();
>      }
>      if ((!fakeCams && hasVideo) || (!fakeMics && hasAudio)) {
> -      RefPtr<MediaManager> manager = MediaManager_GetInstance();
> +      MediaManager* manager = MediaManager::GetIfExists();
> +      MOZ_RELEASE_ASSERT(manager); // invariant on media manager thread

What's the invariant? That MediaManager must exist while media thread is live?

Could you clarify in the comment?

::: dom/media/MediaManager.cpp:2049
(Diff revision 1)
>  int MediaManager::AddDeviceChangeCallback(DeviceChangeCallback* aCallback)
>  {
>    bool fakeDeviceChangeEventOn = mPrefs.mFakeDeviceChangeEventOn;
>    MediaManager::PostTask(NewTaskFrom([fakeDeviceChangeEventOn]() {
> -    RefPtr<MediaManager> manager = MediaManager_GetInstance();
> +    MediaManager* manager = MediaManager::GetIfExists();
> +    MOZ_RELEASE_ASSERT(manager); // invariant on media manager thread

same here

::: dom/media/MediaManager.cpp:3971
(Diff revision 1)
>    MediaManager::PostTask(NewTaskFrom([id, windowId,
>                                        audioDevice, videoDevice,
>                                        c, isChrome]() mutable {
>      MOZ_ASSERT(MediaManager::IsInMediaThread());
> -    RefPtr<MediaManager> mgr = MediaManager::GetInstance();
> +    MediaManager* mgr = MediaManager::GetIfExists();
> +    MOZ_RELEASE_ASSERT(mgr); // invariant on media manager thread

and here

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8916683 [details]
Bug 1406988 - Assert MediaManager invariant where applicable, and check for existence everywhere else.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187776/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d9d8afdea0c
Assert MediaManager invariant where applicable, and check for existence everywhere else. r=pehrsons

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/8d9d8afdea0c