1201197 - Enumeration of Devices silently fails when called adjacent to stopping a WebRTC stream

Status: RESOLVED FIXED

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

Description

[Anthony Minessale]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2499.0 Safari/537.36

Steps to reproduce:

Call getUserMedia and then enumerate devices in the success callback right after stopping the resulting stream.

https://jsfiddle.net/catr8ca3/10/



Actual results:

When you delay the call to enumerate devices, it works but if you call it right away, nothing happens and the callback to enumerate the devices never fires.


Expected results:

Probably in any case the callback should still fire to enumerate the devices but that is a philosophical question left for the developers.

Comment 1

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

This seems timing related to me. I'm only able to reproduce it around 40% of the time and mostly on the first try after opening or refreshing (or hitting Run in) the fiddle. I have also seen it in 40, which I think means gcp is off the hook.

Comment 2

[Anthony Minessale]

Interesting.  For me it fails every time.
Maybe also related to cpu speed or count/platform?
Even 100 ms delay makes it work every time so it seems like the stream.stop and enumerate end up triggering async functions in varying order.

Comment 3

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

(In reply to Anthony Minessale from comment #2)
> Even 100 ms delay makes it work every time so it seems like the stream.stop
> and enumerate end up triggering async functions in varying order.

Yes, I'm not able to reproduce without the stream.stop, or with a delay, so those definitely seem involved. Didn't mean to suggest otherwise.

Comment 4

[Maire Reavy [:mreavy]]

Jib -- I've made this a P2/25.  Do you think it should be higher?  Have you had a chance to verify the likely impact?  Thanks.

Comment 5

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

Yes I'm looking into this now, since it's happening in 40 for me. Marking as P1.

Comment 6

[Maire Reavy [:mreavy]]

Thanks, Jib.  I'm assigning this to you until we know more.

Comment 7

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

Attachment: MozReview Request: Bug 1201197 - add dedicated listener to enumerateDevices.

Bug 1201197 - add dedicated listener to enumerateDevices.

Comment 8

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

.stop() is a synchronous API, but its effect has a lag, which overlaps slightly here with enumerateDevices()'s setup.

enumerateDevices marks the content window "active" in MediaManager early, but it wasn't holding any reference to anyting like a listener to make sure it stayed active for the duration of its request, so basically stop() takes the window off the "active" list, since there are no other listeners, leaving enumerateDevices with no active window when it finishes, which it silently fails on on the assumption that the user navigated away.

enumerateDevices now holds on to a placeholder listener just like gUM does, to make sure the window is held active for the duration of the request.

Comment 9

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

Comment on attachment 8656882 [details]
MozReview Request: Bug 1201197 - add dedicated listener to enumerateDevices.

https://reviewboard.mozilla.org/r/18261/#review16403

Failure to get the maanger would seem to be impossible, given the impl, and other uses assume it's infallible (such as MediaManager::EnumerateDevicesImpl()).  So being consistent would make sense, and you can add a comment to MediaManager::Get() to that effect.

Comment 10

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

Comment on attachment 8656882 [details]
MozReview Request: Bug 1201197 - add dedicated listener to enumerateDevices.

Bug 1201197 - add dedicated listener to enumerateDevices.

Comment 11

[Carsten Book [:Tomcat]]

jib: is there a try run for this change ?

Comment 12

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

Yes, in the mozReview link. https://reviewboard.mozilla.org/r/18259/

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cec3b1e2863

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cec3b1e2863