1443525 - Add a test to ensure we're not crashing if cubeb_init fails

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P3)

Description

[Paul Adenot (:padenot)]

It's probably enough to have a Gecko pref, that, when set, forces `GetCubebContextUnlocked` to return `nullptr`.

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1443525 - Add hidden pref to force CubebUtils to return null context.

Add a hidden pref, media.cubeb.force_null_context, that will force CubebUtils
to return a nullptr when asked for the cubeb context. This is to enable testing
of components, simulating the case cubeb were to fail.

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

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1443525 - Add mochitests to test gUM when a cubeb context cannot be obtained.

Add two mochitests to provide coverage for getUserMedia when getting a cubeb
context fails.
- The first test checks that gUM fails with no audio devices returned. In normal
  circumstances, without cubeb we do not expect devices to be available.
- The second test checks that if fake streams are enabled, gUM will still return
  a stream and that this stream can be used.

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

Comment 3

[Paul Adenot (:padenot)]

Comment on attachment 8960836 [details]
Bug 1443525 - Add hidden pref to force CubebUtils to return null context.

https://reviewboard.mozilla.org/r/229588/#review235418

Very cool. Hopefully with that and your new logging we'll catch things early next time.

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8960837 [details]
Bug 1443525 - Add mochitests to test gUM when a cubeb context cannot be obtained.

https://reviewboard.mozilla.org/r/229590/#review235420

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Was seeing some unexpected failures for these and believe what's happening is that if we nullify cubeb after a device enumeration has already taken place, then we can end up using the previous list when we do a new device enumeration. This is because the device list is maintained and we don't unmap the IDs if we failed to grab cubeb during the next enumeration[0].

In the above push[1] I've changed the order so that devices are unmapped before checking if cubeb exists. Which fixes the tests. It would also mean if cubeb died on us in the wild, we wouldn't maintain a list with potentially stale IDs.

I know there is a larger effort towards reworking device enumeration with bug 1404977. :padenot, would that bandaid fix here[1] be worth doing in the meantime? Or is there something else in the works, or something more I could push along which would be worth doing?

[0]: https://searchfox.org/mozilla-central/source/dom/media/webrtc/MediaEngineWebRTC.cpp#48
[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0b4d9e6cb167258aefcf0d6cc4b05e02a31edf9

Comment 7

[Paul Adenot (:padenot)]

I've stopped working on the larger cleanup of this (it was mostly ready, but we have other issues), so I think it's good if you land this one. Good find in any case!

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 12

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8960836 [details]
Bug 1443525 - Add hidden pref to force CubebUtils to return null context.

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

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8960837 [details]
Bug 1443525 - Add mochitests to test gUM when a cubeb context cannot be obtained.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a659624435e3
Add hidden pref to force CubebUtils to return null context. r=padenot
https://hg.mozilla.org/integration/autoland/rev/1dd761da1344
Add mochitests to test gUM when a cubeb context cannot be obtained. r=padenot

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a659624435e3
https://hg.mozilla.org/mozilla-central/rev/1dd761da1344