Closed
Bug 1140363
Opened 10 years ago
Closed 10 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)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: standard8, Assigned: jib)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jesup
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•10 years ago
|
||
This is the patch based on latest fx-team and is basically the part 1 patch for bug 1106941
Reporter | ||
Updated•10 years ago
|
Version: 33 Branch → Trunk
Reporter | ||
Comment 2•10 years ago
|
||
The correct one this time.
Attachment #8573888 -
Attachment is obsolete: true
Comment 3•10 years ago
|
||
(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)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Mark, please let me know if this fixes it. Thanks.
Flags: needinfo?(standard8)
Attachment #8574069 -
Flags: review?(rjesup)
Updated•10 years ago
|
Attachment #8574069 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 6•10 years ago
|
||
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
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e2d5d99e94b7
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 10•10 years ago
|
||
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?
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Updated•10 years ago
|
status-firefox37:
--- → affected
status-firefox38:
--- → affected
You need to log in
before you can comment on or make changes to this bug.
Description
•