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

RESOLVED FIXED in Firefox 43

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Anthony Minessale, Assigned: jib)

Tracking

39 Branch
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → WebRTC: Audio/Video
Ever confirmed: true
Product: Firefox → Core
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.
(Reporter)

Comment 2

3 years ago
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.
(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.
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.
backlog: --- → webrtc/webaudio+
Rank: 25
Flags: needinfo?(jib)
Priority: -- → P2
Yes I'm looking into this now, since it's happening in 40 for me. Marking as P1.
Flags: needinfo?(jib)
Priority: P2 → P1
Thanks, Jib.  I'm assigning this to you until we know more.
Assignee: nobody → jib
Rank: 25 → 15
Created attachment 8656882 [details]
MozReview Request: Bug 1201197 - add dedicated listener to enumerateDevices.

Bug 1201197 - add dedicated listener to enumerateDevices.
Attachment #8656882 - Flags: review?(rjesup)
.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 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.
Attachment #8656882 - Flags: review?(rjesup) → review+
Comment on attachment 8656882 [details]
MozReview Request: Bug 1201197 - add dedicated listener to enumerateDevices.

Bug 1201197 - add dedicated listener to enumerateDevices.
Keywords: checkin-needed
Version: 43 Branch → 39 Branch
jib: is there a try run for this change ?
Flags: needinfo?(jib)
Keywords: checkin-needed
Yes, in the mozReview link. https://reviewboard.mozilla.org/r/18259/
Flags: needinfo?(jib)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4cec3b1e2863
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.