1140363 - If getting media fails after gUM prompt (e.g. privileged code), then devices aren't properly/fully released

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

I've hit this whilst trying to fix bug 1106941 - the Loop code there is privileged and skips the gUM prompt.

Note that the Global indicator does go away - this is just the one on the conversation widow.

STR

1) No camera installed, (or in GetUserMediaTask::SelectDevice negate the first if (!sources.Length()) and testing Loop
2) Open the conversation window

=> At this point the conversation window does two gUM requests:

- The first is requesting both audio and video, and Fail is called with NotFoundError.
- The second is requesting just audio (via constraints), and is granted appropriately.

3) Have another browser join the conversation
4) Leave the conversation so that you get the feedback view

=> At this point a microphone indication remains.

I've done some debugging, and what I've discovered is that if I do a request for audio-only then the indicator goes away fine.

However, with the two-step audio+video and then audio-only, the indicator remains.

The only thing I've noticed is that "recording-window-ended" doesn't get triggered until the window actually closes, but I can't see anything that would be holding onto a reference for some strange reason.

jib/Florian, any chance you could help me look at this? I can attach a patch that will show this behavior.

Comment 1

[Mark Banner (:standard8)]

Attachment: Patch for reproduction

This is the patch based on latest fx-team and is basically the part 1 patch for bug 1106941

Comment 2

[Mark Banner (:standard8)]

Attachment: Patch for reproduction v2

The correct one this time.

Comment 3

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #0)

> The only thing I've noticed is that "recording-window-ended" doesn't get
> triggered until the window actually closes, but I can't see anything that
> would be holding onto a reference for some strange reason.

That's the notification we rely on to remove the browser-specific indicator, so this makes me think this bug is a platform issue.

Comment 4

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

I think I see what's going on. It seems we remove the "activeWindowId" listener that fires "recording-window-ended" on Denied [1], but not on Fail. I can put a patch up in a few hours.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?rev=8f725bbaff1c&mark=1160-1161#1131

Comment 5

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

Attachment: fire recording-window-ended on gUM failures, like deny, to remove indicator

Mark, please let me know if this fixes it. Thanks.

Comment 6

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

Green try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a163c035f5c

Lets check this in, since it's clearly an improvement, even if for some reason Mark's problem turns out to be something else.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d5d99e94b7

Comment 8

[Phil Ringnalda (:philor)]

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

Comment 9

[Mark Banner (:standard8)]

Thanks Jib, it works fine!

Comment 10

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

Comment on attachment 8574069 [details] [diff] [review]
fire recording-window-ended on gUM failures, like deny, to remove indicator

Approval Request Comment
[Feature/regressing bug #]:
Bug 826576 probably (long time)
[User impact if declined]:
Users may see a camera or microphone indicator left on in the Hello conversation window after a call to getUserMedia fails for reasons other than the user denying permission, such as a user not having a camera or microphone on their system.
[Describe test coverage new/current, TreeHerder]:
Fix of symptom verified by Standard8. Landed on m-c.
[Risks and why]: 
Low, because it's plugging a resource leak in an unusual code-path, caused by missing cleanup code that is present in another more common path. The patch pushes removal of GetUserMediaCallbackMediaStreamListeners from the "Denied" codepath down to a common "Denied or Error" codepath.
[String/UUID change made/needed]: None

Comment 11

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

Comment on attachment 8574069 [details] [diff] [review]
fire recording-window-ended on gUM failures, like deny, to remove indicator

Approval Request Comment
[Feature/regressing bug #]:
Bug 826576 probably (long time)
[User impact if declined]:
Users may see a camera or microphone indicator left on in the Hello conversation window after a call to getUserMedia fails for reasons other than the user denying permission, such as a user not having a camera or microphone on their system.
[Describe test coverage new/current, TreeHerder]:
Fix of symptom verified by Standard8. Landed on m-c.
[Risks and why]: 
Low, because it's plugging a resource leak in an unusual code-path, caused by missing cleanup code that is present in another more common path. The patch pushes removal of GetUserMediaCallbackMediaStreamListeners from the "Denied" codepath down to a common "Denied or Error" codepath.
[String/UUID change made/needed]: None

(Patch applies with offset)

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8574069 [details] [diff] [review]
fire recording-window-ended on gUM failures, like deny, to remove indicator

Looks like a good fix to get in to help with user experience with Hello, low risk.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c51acd8dcf49

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0cd4e38e00b1