1400889 - Crash in mozilla::AudioOutputObserver::InsertFarEnd

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-d0891d65-75b5-4f36-a6d5-c3e960170912.
=============================================================

Seen while looking at Mac crash stats: http://bit.ly/2xc08mk. Appears to have started using 20170901151028.

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=04b6be50a2526c7a26a63715f441c47e1aa1f9be&tochange=a3585c77e2b1bc5f5fea907e97762f7b47a12033

Comments:
This one might actually have to do with having some breakpoints set rather than the APIs I was hit - https://crash-stats.mozilla.com/report/index/69151572-d2c0-4889-a5d4-2c26d0170904
Testing outbound calls with the twilio javascript SDK on localhost. Crashed when I initiated an outbound - https://crash-stats.mozilla.com/report/index/610525a8-cd2c-4f31-ad14-bc05f0170904

URLs:
http://swimminglessonsformodernlife.com/aframe-speech-component/example/
https://hangouts.google.com/call/

Comment 1

[Nils Ohlmeier [:drno]]

The crash signature show up as early as April/May, e.g. https://crash-stats.mozilla.com/report/index/664474e5-75d7-4230-a346-ebf8f0170511

Comment 2

[Michael Froman [:mjf]]

Does MSG and P2/19 seem about right for this?

Comment 3

[Paul Adenot (:padenot)]

I'm rewriting all this, but I won't land it in 57. Alex, can you quickly fix this one? You need to simply up/downmix properly instead of MOZ_CRASH-ing.

Comment 4

[Alex Chronopoulos [:achronop]]

Sure, do you know how to repro?

Comment 5

[Alex Chronopoulos [:achronop]]

One way to repro this is the following:

You need 2 output device one stereo and one mono
1. Set the mono output device as default
2. Make a gUM (audio) request to a stereo microphone. 
3. From the sounds settings change the default output to the stereo device
4. Use the track.applyConstraints()method to change the input track to mono 

You can repro easily using the following fiddle: https://jsfiddle.net/achronop/2jgob190/

On step 2 use the default button on the top left, on step 4 use the mono radio button on the bottom left.

Analysis:
On step 2 AudioOutputObserver::mPlayoutChannels is set to `1` here: http://searchfox.org/mozilla-central/source/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#118
On step 3 we switch to a stereo capable device. At this point the new device is configure to provide mono output.
On step 4 we create a new driver in order to change the input number of channels. The number of output channels of the driver are set to max available which is 2 in our case. But AudioOutputObserver::mPlayoutChannels is 1 and we cause an assert crash.

In order to solve it we can change the AudioOutputObserver in order to be able to set the mPlayoutChannels from outside (or resetting it since if it is zero will be updated correctly on next callback). A second solution is to update AudioCallbackDriver the set the desired output channels on the constructor (instead of using the max available by default).

Please note I do not have a mono device and I repro by hardcoding values on debugger.

Paul can you tell me your thoughts?

Comment 6

[Paul Adenot (:padenot)]

I'd prefer the following:

> In order to solve it we can change the AudioOutputObserver in order to be able to set the mPlayoutChannels from outside (or 
> resetting it since if it is zero will be updated correctly on next callback)

Comment 7

[Alex Chronopoulos [:achronop]]

Attachment: Reset AudioOutputObjerver

This  patch tries to reset the AudioOutputObserver from MediaEngineWebRTCMicrophoneSource class when the UpdateSingleSource method requested a new GraphDriver. This is not ideal because the new driver will apply at the end of the next iteration. The  AudioOutputObserver::mPlayoutChannels will be reseted at the end of AudioOutputObserver::InsertFarEnd. Later on in the same iteration the MediaEngineWebRTCMicrophoneSource::Process will ask for the PlayoutChannels and will get the new value which will not corresponds to the given data. 

The ideal will be to reset the observer at the same point we switch the driver at the end of AudioCallbackDriver::DataCallback() but this requires to create a new Reset method in AudioDataListenerInterface.

Comment 8

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1400889 - Keep constant output channels in MediaStreamGraph.

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

Comment 9

[Alex Chronopoulos [:achronop]]

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

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8911155 [details]
Bug 1400889 - Keep constant output channels in MediaStreamGraph.

https://reviewboard.mozilla.org/r/182662/#review187974

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:112
(Diff revision 1)
>    int channels = aChannels;
>    if (aChannels > MAX_CHANNELS) {
>      channels = MAX_CHANNELS;
>    }
>  
> -  if (mPlayoutChannels != 0) {
> +  if (mPlayoutChannels != 0 || mPlayoutChannels != static_cast<uint32_t>(channels)) {

Please update the rest of the code with this new assumption.

Comment 11

[Alex Chronopoulos [:achronop]]

Comment on attachment 8911155 [details]
Bug 1400889 - Keep constant output channels in MediaStreamGraph.

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

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8911155 [details]
Bug 1400889 - Keep constant output channels in MediaStreamGraph.

https://reviewboard.mozilla.org/r/182662/#review190072

::: dom/media/GraphDriver.h:460
(Diff revision 2)
>    {
>      MOZ_ASSERT(mOutputChannels != 0 && mOutputChannels <= 8);
>      return mOutputChannels;
>    }
>  
> +  bool setOutputChannelCount(uint32_t aChannels);

Always start with a capital letter for Public Methods.

::: dom/media/GraphDriver.cpp:667
(Diff revision 2)
>      output.format = CUBEB_SAMPLE_FLOAT32NE;
>    }
>  
>    // Query and set the number of channels this AudioCallbackDriver will use.
> +  if (!mOutputChannels) {
> -  mOutputChannels = std::max<uint32_t>(1, mGraphImpl->AudioChannelCount());
> +    mOutputChannels = std::max<uint32_t>(1, mGraphImpl->AudioChannelCount());

How about fixing the number of channel count on the MediaStreamGraph (only asking cubeb for the channel count on the first call, memorizing the value), and then always using this value when creating drivers ?

::: dom/media/MediaStreamGraph.cpp:3191
(Diff revision 2)
>  bool
>  SourceMediaStream::OpenNewAudioCallbackDriver(AudioDataListener * aListener)
>  {
>    MOZ_ASSERT(GraphImpl()->mLifecycleState ==
>        MediaStreamGraphImpl::LifecycleState::LIFECYCLE_RUNNING);
> -  AudioCallbackDriver* nextDriver = new AudioCallbackDriver(GraphImpl());
> +  UniquePtr<AudioCallbackDriver> nextDriver = MakeUnique<AudioCallbackDriver>(GraphImpl());

It would be simpler to have a parameter for the output channel count, right ?

Comment 13

[Alex Chronopoulos [:achronop]]

Comment on attachment 8911155 [details]
Bug 1400889 - Keep constant output channels in MediaStreamGraph.

https://reviewboard.mozilla.org/r/182662/#review190624

::: dom/media/GraphDriver.cpp:667
(Diff revision 2)
>      output.format = CUBEB_SAMPLE_FLOAT32NE;
>    }
>  
>    // Query and set the number of channels this AudioCallbackDriver will use.
> +  if (!mOutputChannels) {
> -  mOutputChannels = std::max<uint32_t>(1, mGraphImpl->AudioChannelCount());
> +    mOutputChannels = std::max<uint32_t>(1, mGraphImpl->AudioChannelCount());

Yeah, since you prefer that path I will change it.

::: dom/media/MediaStreamGraph.cpp:3191
(Diff revision 2)
>  bool
>  SourceMediaStream::OpenNewAudioCallbackDriver(AudioDataListener * aListener)
>  {
>    MOZ_ASSERT(GraphImpl()->mLifecycleState ==
>        MediaStreamGraphImpl::LifecycleState::LIFECYCLE_RUNNING);
> -  AudioCallbackDriver* nextDriver = new AudioCallbackDriver(GraphImpl());
> +  UniquePtr<AudioCallbackDriver> nextDriver = MakeUnique<AudioCallbackDriver>(GraphImpl());

In this case we cannot check if it's a valid one (and return an error). This will show up later during the driver initialization by failing. Is that ok?

Comment 14

[Alex Chronopoulos [:achronop]]

Comment on attachment 8911155 [details]
Bug 1400889 - Keep constant output channels in MediaStreamGraph.

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

Comment 15

[Paul Adenot (:padenot)]

Comment on attachment 8911155 [details]
Bug 1400889 - Keep constant output channels in MediaStreamGraph.

https://reviewboard.mozilla.org/r/182662/#review191852

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d798bb059d77
Keep constant output channels in MediaStreamGraph. r=padenot

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/d798bb059d77

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. Crash volume on ESR52 doesn't seem high enough to warrant backport consideration, though.

Comment 19

[Alex Chronopoulos [:achronop]]

Attachment: Uplift to beta

Approval Request Comment
[Feature/Bug causing the regression]:Bug 1378070
[User impact if declined]: Crash when many output device are used with different channel count.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes
steps in comment 5
[List of other uplifts needed for the feature/fix]:N/A
[Is the change risky?]:No
[Why is the change risky/not risky?]: Make number of out channels unchangeable during a call.
[String changes made/needed]: N/A

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916607 [details] [diff] [review]
Uplift to beta

This is not a new crash, neither is it high volume in release56/beta57. I'd prefer we let this one ride the 58 train.

Comment 21

[xpeng]

will the modification be merged into firefox 56/57?  the crash issue is serious.

Comment 22

[Nils Ohlmeier [:drno]]

(In reply to xpeng from comment #21)
> will the modification be merged into firefox 56/57?  the crash issue is
> serious.

Mozilla's policies would allow this to get into Firefox 56 Release only if it would be a serious security issue.

And in comment #20 Firefox Release Engineering made the decision to no include this patch Firefox 57 Beta, because the crash rate is not very high and the risk of unknown side effects is too high.

So no this is only going to be fixed in Firefox 58.

Comment 23

[Marcia Knous [:marcia]]

Someone reported a crash in Bug 1416855 that has the same signature as this one, and it is happening in 58 beta.

Comment 24

[Alexandru Simonca [:asimonca], Release Desktop QA]

I've experienced a tab crash with this signature while testing WebRTC using macOS 10.13 with Nightly 59.0a1 (id: 20171221100108)
The steps to reproduce are:

0. Be sure to have a bluetooth headset connected to the Apple machine you are testing with
1. Open Nightly.
2. Go to a WebRTC service. (We reproduced with appear.in and web.ciscospark.com)
3. Enter a room and start a video call.
4. Go to "Settings" -> "Audio" in the OS and change the input to the default microphone. The stream stops.
5. Refresh the page in Nightly.

Actual result:
The tab crashes after accepting  the permissions doorhanger. 

Expected result:
The tab shouldn't crash.

https://crash-stats.mozilla.com/report/index/c77d52b0-4f74-4071-bfba-3d4150171221

Comment 25

[Alex Chronopoulos [:achronop]]

This is in my radar. The description is promising and I have the equipment to repro. I will try it soon.

Comment 26

[xpeng]

firefox crashes when plugin or plugout the device : Yamaha PJP-20UR
there are several crashes 
Firefox 57.0.4 Crash Report [@ mozilla::AudioOutputObserver::InsertFarEnd ]
https://crash-stats.mozilla.com/report/index/2cd206db-c71a-4821-ae64-c44400180106

I will fire a new bug for the issue:
Firefox 57.0.4 Crash Report [@ CTopologyCache::OnDisposalTimer ] 
https://crash-stats.mozilla.com/report/index/c8a243ed-8173-44cb-a684-8a95e0180106

there is no crash report when plugin the device : Yamaha PJP-20UR

Comment 27

[Stefan Fleiter (:sfleiter)]

Here another crash report from Mac 57.0.4:
https://crash-stats.mozilla.com/report/index/595a3b77-6cac-4984-9fcb-f769c0180115#tab-details

Did *not* manage to reproduce on Mac Nightly 59.0a1 (2018-01-14) (64-Bit)
built from https://hg.mozilla.org/mozilla-central/rev/e157471df46e457a0f9c1eb785dc8cb5746067a4

Comment 28

[Stefan Fleiter (:sfleiter)]

Happens also on Windows
https://crash-stats.mozilla.com/report/index/fd111192-e807-445b-99a4-8c4b80180115
but also only on Firefox 57 and not on Nightly.

Comment 29

[Alex Chronopoulos [:achronop]]

This must have been corrected indirectly, in Nightly, by bug 1428392. I am closing for now, if you still get the crash feel free to reopen this bug.