+++ This bug was initially created as a clone of Bug #802656 +++ Spun off from bug 802656 comment 12. Could someone please sum up the privacy/UX concerns?

Blocking on a decision from privacy here. Monica - Can you provide input?

I'm sorry for the delay, I missed this when I was out. From Maire's notes after our meeting: Hi all -- Here is the summary of our decisions and action items coming out of our gUM UI meeting last Thursday (10/18 @ 12 pm Pacific). We want to enable the gUM (getUserMedia) feature by default -- ideally in time for Firefox 19 uplift. We asked ourselves: what is the minimum set of UI design changes necessary to do that? We decided the following are necessary: 1) Adding a "Never share with this site" option to the choices offered to users -> if we do this, we'll need to have a way for the user to undo this. 2) Adding an easy way within Firefox to shut down camera/microphone access after access has been granted 3) Adding a "glow" or color change to the tab when the page in the tab has access to camera/microphone data 4) Making sure that user documentation for this feature is readily available We decided the following are nice-to-have: 1) Adding a pop up and warn or a growl (e.g. a pop up saying, "site X now has access to your camera") 2) Adding persistent permissions (e.g. an option saying "always share with this site, don't ask me again") . Even if we have the time to implement persistent permissions, we probably should not try to get them into v1 since adding this feature means considering, handling, and debugging lots of edge cases. ACTION ITEM: Maire will reach out to the telemetry and test pilot folks and ask how we can gather gUM users statistics/metrics from Aurora/Beta users in the field. If I missed anything or if you disagree with anything in this summary, please chime in. Thanks. -Maire

The original privacy review request: https://bugzilla.mozilla.org/show_bug.cgi?id=795024

(In reply to Monica Chew from comment #3) > The original privacy review request: > https://bugzilla.mozilla.org/show_bug.cgi?id=795024 I skimmed that bug, didn't see anything that would be relevant for this bug.

OK -- I am missing some context and I can't tell from Anant's comment in the other bug which of the privacy concerns we talked about in the meeting are relevant here. To sum up, the UX changes agreed on were to ensure that the user always knew which tab was accessing which device and could revoke access at any time. Having multiple tabs access the same device is not something that explicitly came up during the meeting.

It was discussed (I think in that meeting), but briefly. The basic privacy/security concern is that if two tabs are both getting input from the same camera, they can at time figure out what other application has the page open. (certain aspects of resolution and other settings might be a give-away you're using Hangouts, for example, or Skype). This might be combined with an app that has persistent permission to "sample" the camera state to see if it's in-use and if so what's using it. It can also allow purposeful leaks of information from one tab to another in a low-bandwidth fashion, though there are other ways to do that. I believe these aspects are relatively minor. I think we can decide to either: a) allow camera sharing and if the privacy concerns seem real, deal with them in some way in the future, or disable the feature later, or b) don't allow multiple tabs to access the same camera at once. The downside of this is that Chrome and Opera do allow this, and some demos will break (though probably not many real apps, if any). I recommend b), which also means we can defer any work on this until later.

(In reply to Randell Jesup [:jesup] from comment #6) > The basic privacy/security concern is that if two tabs are both getting > input from the same camera, they can at time figure out what other > application has the page open. How would they? Would Firefox behave differently because some other page has access to the same source? Or are you assuming that applications would communicate behind the scene? As mentioned in bug 802656, the list of devices being unstable is a problem for the UI, so I recommend a).

(In reply to Dão Gottwald [:dao] from comment #7) > (In reply to Randell Jesup [:jesup] from comment #6) > > The basic privacy/security concern is that if two tabs are both getting > > input from the same camera, they can at time figure out what other > > application has the page open. > > How would they? Would Firefox behave differently because some other page has > access to the same source? Or are you assuming that applications would > communicate behind the scene? The hardware has settings that can't be independent from each other; it's a little like the "use the accelerometers to spy on typing" trick, but with significantly lower risk. For example, an app could use the resolution or frame-rate settings to infer the capturing app (if it does something unusual), or white-balance settings, etc. The real risk is low (perhaps extremely low) as most apps will use "standard" settings, but the fancier apps like Skype and Hangouts might be detectable, and so an spy tab could determine when/if/how-long you skyped with someone (but not who). > As mentioned in bug 802656, the list of devices being unstable is a problem > for the UI, so I recommend a). This is a problem in the base code; it should use a unique identifier in all cases. I think I opened some issues upstream on this. I'm not sure how a) makes that any better.

(In reply to Randell Jesup [:jesup] from comment #8) > > As mentioned in bug 802656, the list of devices being unstable is a problem > > for the UI, so I recommend a). > > This is a problem in the base code; it should use a unique identifier in all > cases. I think I opened some issues upstream on this. I'm not sure how a) > makes that any better. We seem to be talking past each other. The problematic scenario is the user getting a list of devices to approve access for, then the list of devices changing because some other site got or lost access, and finally the user acting on the outdated list. I don't see what unique identifiers have to do with this.

See bug 815469 for the index issue (which had been discussed in several bugs including this one). I don't see how that bug affects the decision over a or b.

(In reply to Randell Jesup [:jesup] from comment #10) > See bug 815469 for the index issue (which had been discussed in several bugs > including this one). I don't see how that bug affects the decision over a > or b. I didn't say it would. You brought up unique identifiers... but they won't change the fact that the user may pick a device that isn't available anymore (because another site picked it) or miss a choice when a device becomes available again (because another site freed it).

UI issues aside, sharing the same picture or sound with multiple applications/conferences/whatever seems like it could be a very real use case, so given that other browsers support it, I actually feel pretty strongly that we should too. Cross-browser consistency is important. If we think the privacy threat is significant (which isn't my reading of this bug so far), we should take it to the spec and reach consensus there.

After discussion with Dao and ekr, while there are some real (if probably very minor) issues here, there are certainly no short-term practical concerns with information leakage. So we're going to go with option (a) - allow multiple tabs access to the camera, unless we don't have time to make the mods to do so for FF20. Per Anant on IRC, see https://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#484

WIP patch, video modifed for sharing; compiles and runs (audio still needs the start/stop count mod), doesn't work, loose ends to wrap up

Comment on attachment 696599 [details] [diff] [review] Allow the user to explicitly share devices between tabs Try against Alder (for gum/peerconnection Mochitests): https://tbpl.mozilla.org/?tree=Try&rev=8514e7c0fdae

Comment on attachment 696599 [details] [diff] [review] Allow the user to explicitly share devices between tabs Review of attachment 696599 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +47,5 @@ > } > > nsresult > MediaEngineWebRTCAudioSource::Allocate() > { Would be good to log here if Allocate() is called when mSources is empty vs. the first time. Same with Deallocate. @@ +78,5 @@ > > nsresult > MediaEngineWebRTCAudioSource::Start(SourceMediaStream* aStream, TrackID aID) > { > + if (!mInitDone || !aStream) { Why remove the (mState != kAllocated) check? @@ +243,5 @@ > return; > } > > if (mState == kStarted) { > + while (!mSources.IsEmpty()) Should probably document in MediaEngine.h that Stop() will remove the element it stopped. Nit: braces. @@ +290,5 @@ > + AudioSegment segment; > + segment.Init(CHANNELS); > + segment.AppendFrames( > + buffer.forget(), length, 0, length, AUDIO_FORMAT_S16 > + ); Nit: indentation. @@ +293,5 @@ > + buffer.forget(), length, 0, length, AUDIO_FORMAT_S16 > + ); > + SourceMediaStream *source = mSources[i]; > + if (source) > + source->AppendToTrack(mTrackID, &segment); Nit: braces. ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp @@ +208,5 @@ > } > > nsresult > MediaEngineWebRTCVideoSource::Deallocate() > { Probably should log here (like in AudioSource), for both branches. @@ +233,5 @@ > nsresult > MediaEngineWebRTCVideoSource::Start(SourceMediaStream* aStream, TrackID aID) > { > int error = 0; > + if (!mInitDone || !aStream) { Same comment as AudioSource, why remove the allocated check? @@ +433,5 @@ > } > > if (mState == kStarted) { > + while (!mSources.IsEmpty()) > + Stop(mSources[0], kVideoTrack); // XXX change to support multiple tracks Nit: braces.

(In reply to Anant Narayanan [:anant] from comment #19) > @@ +78,5 @@ > > > > nsresult > > MediaEngineWebRTCAudioSource::Start(SourceMediaStream* aStream, TrackID aID) > > { > > + if (!mInitDone || !aStream) { > > Why remove the (mState != kAllocated) check? Because another tab may have already moved it past kAllocated to kStarted (see later in the function with the patch applied) > > @@ +243,5 @@ > > return; > > } > > > > if (mState == kStarted) { > > + while (!mSources.IsEmpty()) > > Should probably document in MediaEngine.h that Stop() will remove the > element it stopped. Nit: braces. Nothing outside of this knows about mSources; we use it to make sure if you call Stop twice we don't mess up our counts.

Verified on 1/29 through the test case.