1524648 - MediaEngineWebRTCMicrophoneSource::Deallocat asserts if another browser already has gUM on audio and video

Status: RESOLVED FIXED

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

Description

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

I'm able to hit the MOZ_ASSERT(mState == kStopped); in MediaEngineWebRTCMicrophoneSource::Deallocate by trying use gUM if I've already grabbed it in another browser:

STR:

• Open copy 1 of Firefox, go to https://mozdevs.github.io/MediaRecorder-examples/record-video-and-audio.html, accept the gum prompt and leave that browser there.
• Open copy 2 of Firefox (debug to hit the assert), go to https://mozdevs.github.io/MediaRecorder-examples/record-video-and-audio.html, accept the gUM prompt.
• Copy 2 of Firefox asserts.

This appears to require both mic and cam on my laptop. Mic only examples seem fine.

Comment 1

[Paul Adenot (:padenot)]

I can't seem to repro this one on Linux, I'll try on Windows.

Comment 2

[Paul Adenot (:padenot)]

Can't repro either.

Comment 3

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

Will try and take another look later today and see if I can find out if there's something more to the repro -- if nothing else I should be able to provide a stack trace to better contextualise what's happening.

Comment 4

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

Always asserts on my setup, though I'm still unclear as to what the confounding factor is. Let me know if it would be useful for me to try with any alterations.

Asserting stack:

>	xul.dll!mozilla::MediaEngineWebRTCMicrophoneSource::Deallocate(const RefPtr<const mozilla::AllocationHandle> &) Line 446	C++
 	xul.dll!mozilla::MediaDevice::Deallocate() Line 1017	C++
 	xul.dll!mozilla::GetUserMediaTask::Run() Line 1657	C++
 	xul.dll!MessageLoop::RunTask(already_AddRefed<nsIRunnable> aTask) Line 443	C++
 	xul.dll!MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask && pending_task) Line 450	C++
 	xul.dll!MessageLoop::DoWork() Line 523	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainUIThreads::DoRunLoop() Line 377	C++
 	xul.dll!base::MessagePumpWin::Run(base::MessagePump::Delegate * delegate) Line 79	C++
 	xul.dll!MessageLoop::RunHandler() Line 309	C++
 	xul.dll!base::Thread::ThreadMain() Line 192	C++
 	xul.dll!`anonymous namespace'::ThreadFunc(void * closure) Line 31	C++
 	[External Code]	
 	mozglue.dll!patched_BaseThreadInitThunk(int aIsInitialThread, void * aStartAddress, void * aThreadParam) Line 735	C++
 	[External Code]	

Looks like it's hitting failing to allocate the video device in MediaManager. Once this happens it goes to deallocate the audio device. We then call through to our asserting funciton. Looks like since we've just allocated the audio device, we're not in a stopped state when we call deallocate?

It looks like we could also hit this assert if we called Shutdown on a freshly allocated MicSource, as that will also deallocate without changing state.

Comment 5

[Paul Adenot (:padenot)]

Yep I think you're right. In terms of state, it's probably legal to deallocate when just allocated.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1524648 - Relax assertion in MediaEngineWebRTCMicrophoneSource::Deallocate to take into account the fact that starting the device can have failed. r?pehrsons

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b350508a425c
Relax assertion in MediaEngineWebRTCMicrophoneSource::Deallocate to take into account the fact that starting the device can have failed. r=pehrsons

Comment 8

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on MediaEngineWebRTCAudio.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/31b3a7b0f085fff80bdd7462b83bc595be62cd83
Push link: https://hg.mozilla.org/integration/autoland/rev/b350508a425c588684557eb62686cf1a8b4f8e07
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227575095&repo=autoland&lineNumber=20596

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/678688278ecc
Relax assertion in MediaEngineWebRTCMicrophoneSource::Deallocate to take into account the fact that starting the device can have failed. r=pehrsons

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/678688278ecc