835953 - Releasing webcam access in gUM still leaves the green icon UI showing the camera is active shown

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P1)

Description

[Jason Smith [:jsmith]]

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.

Comment 1

[Jason Smith [:jsmith]]

Seems basic enough that it should block.

Comment 2

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

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?

Comment 3

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

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

Comment 4

[Dão Gottwald [:dao]]

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.

Comment 5

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

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.

Comment 6

[Dão Gottwald [:dao]]

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.

Comment 7

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

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

Comment 8

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

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 9

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

Attachment: Notify UI that all gUM streams for a WindowID are gone

Comment 10

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

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)

Comment 11

[Dão Gottwald [:dao]]

(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.

Comment 12

[Dão Gottwald [:dao]]

(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.

Comment 13

[Dão Gottwald [:dao]]

Attachment: front-end part

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

Comment 14

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

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?

Comment 15

[Dão Gottwald [:dao]]

(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 16

[Doug Turner (:dougt)]

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.

Comment 17

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

(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).

Comment 18

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

-> 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).

Comment 19

[Jason Smith [:jsmith]]

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 20

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 21

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

Attachment: Notify UI that all gUM streams for a WindowID are gone

Comment 22

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

Attachment: Notify UI in e10s that gUM recording for window is done WIP

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

Comment 23

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

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.

Comment 24

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

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 25

[Dão Gottwald [:dao]]

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.

Comment 26

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

Attachment: Notify UI that all gUM streams for a WindowID are gone

updated; doing test compile/etc now

Comment 27

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

Attachment: Notify UI that all gUM streams for a WindowID are gone

Comment 28

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

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

Fix silly typo

Comment 29

[Dão Gottwald [:dao]]

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.

Comment 30

[Dão Gottwald [:dao]]

(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 31

[Dão Gottwald [:dao]]

Attachment: front-end part

Comment 32

[Dão Gottwald [:dao]]

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 33

[Justin Dolske [:Dolske]]

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.

Comment 34

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

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

Comment 35

[Dão Gottwald [:dao]]

(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

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

Comment 37

[Jason Smith [:jsmith]]

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

Comment 38

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

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 39

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

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).

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02a865a0077e

Comment 41

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

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

Comment 42

[Jason Smith [:jsmith]]

Verified on Nightly on 2/18.