Build: 1/29 Nightly Steps: 1. Go to http://mozilla.github.com/webrtc-landing/gum_test.html 2. Select video 3. Accept permissions for a webcam 4. Select stop Expected: The camera should be released - resulting in no UI showing that the camera is actively being used. Actual: The green icon UI is still shown showing the camera is still being used.

Seems basic enough that it should block.

We're generating recording-device-events event with shutdown, and also should be dispatching to the mainthread a GetUserMediaListenerRemove to remove the window ID from the active window list, so the UI *should* reflect no streams in use.... Dao?

Taking bug unless/until we know it's a UI issue.

We observe recording-device-events for the global indicator. Using this for the per-tab indicator is possible but far from ideal, since we'd need to match the list of all capturing windows with the list of all tabs in order to find the indicator we'd need to update.

Ok, what is the UI waiting for to remove the recording indicator from the tab? I don't think there's anything else we're sending... Perhaps there's a disconnect between UI and MediaManager, or something needs to be added to the API between MediaManager and the UI.

The indicator will currently be removed only when navigating away from the page. It doesn't attempt to use any other UI, since as you say there is none.

Aha. So there is in fact a disconnect; the indicator is supposed to go away when the last use is gone and the application no longer has access to the camera or mic. I and others assumed sending the recording-device-event and removing it from the window list would result in its being removed from the UI of the tab. Let me look at what I'm sending, but please feel free to suggest some additional notification needed to get this done. Thanks

How about I send a "recording-window-ended" Notification with the WindowID as the argument when the last active listener for the window goes away? I'm open as to the name. We could overload recording-device-events with an argument of "window-ended: windowId" or similar in JSON form if that works better. I'm open.

Comment on attachment 709085 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone preemptively asking for review under the assumption that this will fill the need from the backend (maybe with rewording the Notification topic)

(In reply to Randell Jesup [:jesup] from comment #8) > How about I send a "recording-window-ended" Notification with the WindowID > as the argument when the last active listener for the window goes away? I'd prefer getting the actual DOM window as the notification subject.

(In reply to Dão Gottwald [:dao] from comment #11) > (In reply to Randell Jesup [:jesup] from comment #8) > > How about I send a "recording-window-ended" Notification with the WindowID > > as the argument when the last active listener for the window goes away? > > I'd prefer getting the actual DOM window as the notification subject. Actually, I don't really mind. Getting the window via the window ID in front-end code is simple enough.

Here's how I imagine the front-end part would look like. Currently untested.

Comment on attachment 709104 [details] [diff] [review] front-end part Review of attachment 709104 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/webrtcUI.jsm @@ +212,5 @@ > } > + > +function removeBrowserSpecificIndicator(aSubject, aTopic, aData) { > + let browser = getBrowserForWindowId(aData); > + let PopupNotifications = browser.ownerDocument.defaultView.PopupNotifications; Not being a front-end person: do you need to check if the window still exists in some way?

(In reply to Randell Jesup [:jesup] from comment #14) > Not being a front-end person: do you need to check if the window still > exists in some way? Yeah, seems reasonable.

Comment on attachment 709085 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone Review of attachment 709085 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/PContent.ipdl @@ +449,5 @@ > // get nsIVolumeService to broadcast volume information > async BroadcastVolume(nsString volumeName); > > async RecordingDeviceEvents(nsString recordingStatus); > + async RecordingWindowEnded(nsString windowID); Passing the window idea here is probably the wrong thing to do. Instead, you can use the TabChild / PBrowser which knows about windows.

(In reply to Doug Turner (:dougt) from comment #16) > Comment on attachment 709085 [details] [diff] [review] > Notify UI that all gUM streams for a WindowID are gone > > Review > > > > async RecordingDeviceEvents(nsString recordingStatus); > > + async RecordingWindowEnded(nsString windowID); > > Passing the window idea here is probably the wrong thing to do. Instead, > you can use the TabChild / PBrowser which knows about windows. I can try, but I'll note that all I have identifying the window is the WindowID. If it's as easy as it appears to convert windowid's on the UI side, have me do it doesn't seem to gain anything. MediaManager keeps a map of WindowID's with an array of listeners (i.e. MediaStreams) in use by that window. I'm sending this event when the last listener for the WindowID is removed (last mediastream goes away).

-> Dao for the UI part If I need to revise the internals in order to pass back something else, please let me know (and if possible point me to an example).

Rationale for tracking-firefox 20 nomination: This breaks a basic functional flow in a privacy requirement we are required to have for the getUserMedia UI. A user must be able to understand when web content from X tabs has camera/mic access vs. not having it. That includes the case when web content decides to release access to the camera/mic through the Local Media Stream returned from a successful gUM call. The user impact of this bug is the when web content decides to release camera/mic access, we end up still indicating in the UI that the web content still access to the camera/mic actively. This effectively ends up lieing to the user, as they think web content still is making use of their camera/mic, when in fact, the camera/mic was effectively released. As a result, our gUM triage deemed this as blocking, as the device integration + privacy requirement makes this a critical flow that we have to get correct.

Comment on attachment 709085 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone I don't think I have any input to add beyond Dao/Doug's feedback on this.

this patch is not compilable and has dangling ends. It also isn't needed for FF20 or FF21!

Comment on attachment 714271 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone This isn't needed for e10s targets until at least FF22, maybe FF23, so all we need now is the straight observer service patch for desktop.

Note: that means we don't need to support e10s right now, so things are simpler. I'll break the e10s part into a separate bug

Comment on attachment 714271 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone >+ // Notify the UI that this window no longer has gUM active >+ char windowBuffer[32]; >+ PR_snprintf(windowBuffer, sizeof(windowBuffer), "%llu", aWindowID); >+ nsAutoString data; >+ data.Append(NS_ConvertUTF8toUTF16(windowBuffer)); >+ >+ nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); >+ obs->NotifyObservers(nullptr, "recording-window-ended", data.get()); If I'm not mistaken, aWindowID is the inner window id. We need the outer window id.

updated; doing test compile/etc now

Comment on attachment 714393 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone Fix silly typo

Something is still wrong here. For example, while testing on http://mozilla.github.com/webrtc-landing/gum_test.html, getUserMedia:request gives me 47 as the outer window ID and recording-window-ended gives me 49. nsIDOMWindowUtils::getOuterWindowWithId finds the window for the former but not for the latter.

(In reply to Dão Gottwald [:dao] from comment #29) > Something is still wrong here. For example, while testing on > http://mozilla.github.com/webrtc-landing/gum_test.html, getUserMedia:request > gives me 47 as the outer window ID and recording-window-ended gives me 49. > nsIDOMWindowUtils::getOuterWindowWithId finds the window for the former but > not for the latter. Scratch that, my build still had the previous patch applied.

Comment on attachment 714393 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone Looks ok to me, but I don't feel comfortable enough reviewing this code.

Comment on attachment 714393 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone Looks like we already do similar stuff here, so I'll mark r+ even though this isn't my forte.

https://hg.mozilla.org/integration/mozilla-inbound/rev/163daa0ef353 https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f4c2e87acf

(In reply to Randell Jesup [:jesup] from comment #34) > https://hg.mozilla.org/integration/mozilla-inbound/rev/163daa0ef353 Backed out and landed with me as the author: https://hg.mozilla.org/integration/mozilla-inbound/rev/02a865a0077e

https://hg.mozilla.org/mozilla-central/rev/163daa0ef353 https://hg.mozilla.org/mozilla-central/rev/f1f4c2e87acf

Needs an uplift to Aurora preferably before we go to build for Beta 1.

Comment on attachment 714393 [details] [diff] [review] Notify UI that all gUM streams for a WindowID are gone [Approval Request Comment] Bug caused by (feature/regressing bug #): Initial indicator support User impact if declined: The "camera/mic is active" indicator will remain in the URLbar until you navigate away, even after you stop accessing the camera/mic. Testing completed (on m-c, etc.): On m-c; locally tested by me. Risk to taking this patch (and alternatives if risky): very low risk, merely sends a Notification. String or UUID changes made by this patch: Notification string added, no l10n. Need to land with the other patch on this bug (front-end to receive the notification).

Comment on attachment 714441 [details] [diff] [review] front-end part [Approval Request Comment] Bug caused by (feature/regressing bug #): Initial indicator support User impact if declined: The "camera/mic is active" indicator will remain in the URLbar until you navigate away, even after you stop accessing the camera/mic. Testing completed (on m-c, etc.): On m-c; locally tested by me. Risk to taking this patch (and alternatives if risky): very low risk String or UUID changes made by this patch: Notification string added, no l10n. Need to land with the other patch on this bug (back-end to send the notification).

https://hg.mozilla.org/releases/mozilla-aurora/rev/d0a1f24eef41 https://hg.mozilla.org/releases/mozilla-aurora/rev/5ffe19b04456

Verified on Nightly on 2/18.