Closed Bug 1140363 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: standard8, Assigned: jib)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Flags: needinfo?(jib)
Flags: needinfo?(florian)
Attached patch Patch for reproduction (obsolete) — Splinter Review
This is the patch based on latest fx-team and is basically the part 1 patch for bug 1106941
Version: 33 Branch → Trunk
The correct one this time.
Attachment #8573888 - Attachment is obsolete: true
(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.
Flags: needinfo?(florian)
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
Assignee: nobody → jib
Flags: needinfo?(jib)
Mark, please let me know if this fixes it. Thanks.
Flags: needinfo?(standard8)
Attachment #8574069 - Flags: review?(rjesup)
Attachment #8574069 - Flags: review?(rjesup) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2d5d99e94b7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Thanks Jib, it works fine!
Flags: needinfo?(standard8)
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
Attachment #8574069 - Flags: approval-mozilla-aurora?
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)
Attachment #8574069 - Flags: approval-mozilla-beta?
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.
Attachment #8574069 - Flags: approval-mozilla-beta?
Attachment #8574069 - Flags: approval-mozilla-beta+
Attachment #8574069 - Flags: approval-mozilla-aurora?
Attachment #8574069 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.