While page already has a live track, getUserMedia should allow un-prompted re-access to same device.

VERIFIED FIXED in Firefox 53

Status

()

defect
P2
normal
Rank:
22
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: jib, Assigned: mchiang)

Tracking

(Depends on 4 bugs)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected, firefox53 verified)

Details

()

Attachments

(2 attachments, 2 obsolete attachments)

The MediaCapture and Streams spec now mandates [1] that a browsing context that already has at least one un-stopped MediaStreamTrack in it must be allowed to request new tracks from the same device without a prompt. 

This makes sense given that streams can also be cloned. Calling getUserMedia again is in some sense just another form of clone.

[1] https://github.com/w3c/mediacapture-main/commit/bbdee2433051107e597a4fbd277b6d619113ac54
Rank: 22
Priority: -- → P2
Assignee: nobody → mchiang
Please refer to the example https://jsfiddle.net/qb0ju200/.
In current design, we allow user to access different cameras for each gUM.
If we want to support this spec, the behavior will be like Chrome -- the 2nd gUM will always access the same camera without any prompt. Is this what we want?
Flags: needinfo?(jib)
Yes, same camera on second call in that example. The prompt is a permission prompt first and device selector second. When there's no need for permission, then no prompt, e.g. user chose  "Always allow".

An extra wrinkle is that unlike our "Always allow", this implicit allowance does not extend to other devices of the same kind, so there will be cases where constraints will preclude the same camera(s) from being chosen, and in those cases the user may still see a permission prompt (unless they chose "Always allow").

Another case where the prompt may come up again would be if the site asked for more kinds of media - e.g. camera-only first, then camera+mic second - and in that case I think we should prompt for audio only (which would be consistent with how "Always allow" works, i.e. per kind).
Flags: needinfo?(jib)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> Another case where the prompt may come up again would be if the site asked
> for more kinds of media - e.g. camera-only first, then camera+mic second -
> and in that case I think we should prompt for audio only (which would be
> consistent with how "Always allow" works, i.e. per kind).

When I rethink this, I see that prompting for audio only in the same_camera + new_mic case is a bad idea, since the script may stop the camera stream in the meantime while the permission prompt is up, leaving the request in an impossible state, unless we hold the camera stream alive while the prompt is up, which would be bad I think.

Better then to treat this special case as a regular new_camera + new_mic prompt. Simpler too. Sorry for the bad advice.
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review93248

Looks good except I think the logic for requesting already_live_camera+new_mic needs to be different in hindsight (see comment 4). Misc nits and some questions on the principal stuff. Would like to see it again.

::: dom/media/MediaManager.cpp:253
(Diff revision 1)
>   * to Start() and Stop() the underlying MediaEngineSource when MediaStreams
>   * are assigned and deassigned in content.
>   */
>  class GetUserMediaCallbackMediaStreamListener : public MediaStreamListener
>  {
> +  friend MediaManager;

Why is this needed? (it may be, I'm just not seeing it)

::: dom/media/MediaManager.cpp:2093
(Diff revision 1)
> +  // Disable userContext and firstParty isolation for permissions.
> +  attrs.StripUserContextIdAndFirstPartyDomain();

Why is this needed? (not saying it isn't, just comparing to [1])

[1] https://dxr.mozilla.org/mozilla-central/rev/f8ba9c9b401f57b0047ddd6932cb830190865b38/dom/media/systemservices/CamerasParent.cpp#727

::: dom/media/MediaManager.cpp:2098
(Diff revision 1)
> +  // Disable userContext and firstParty isolation for permissions.
> +  attrs.StripUserContextIdAndFirstPartyDomain();
> +
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult rv = NS_NewURI(getter_AddRefs(uri), originNoSuffix);
> +  NS_ENSURE_SUCCESS(rv, rv);

Nit: We've been told to use

    if (NS_WARN_IF(NS_FAILED(rv))) {
      return rv;
    }

in new code.

::: dom/media/MediaManager.cpp:2466
(Diff revision 1)
> +        bool askPermission_video = IsOn(c.mVideo) ? true : false;
> +        bool askPermission_audio = IsOn(c.mAudio) ? true : false;

Nit: IsOn() already returns a boolean, so

    ? true : false

is redundant. Also, use cameCase instead of _ in variable names. e.g. askVideoPermission

::: dom/media/MediaManager.cpp:2469
(Diff revision 1)
> +        nsTArray<RefPtr<VideoDevice>> videos;
> +        nsTArray<RefPtr<AudioDevice>> audios;
> +        bool askPermission_video = IsOn(c.mVideo) ? true : false;
> +        bool askPermission_audio = IsOn(c.mAudio) ? true : false;
> +
> +        nsString rawId;

Declare rawId inside loop near it's use(s).

::: dom/media/MediaManager.cpp:2470
(Diff revision 1)
> +        nsTArray<RefPtr<AudioDevice>> audios;
> +        bool askPermission_video = IsOn(c.mVideo) ? true : false;
> +        bool askPermission_audio = IsOn(c.mAudio) ? true : false;
> +
> +        nsString rawId;
> +        auto& devices_ = **devices;

Nit: **devices is already used a few lines up, so let's hoist this reference up there if we want to keep it.

Also, no trailing _ in this file please. devs?

::: dom/media/MediaManager.cpp:2481
(Diff revision 1)
> +            }
> +            else {

Nit: } else {

Applies throughout.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

::: dom/media/MediaManager.cpp:2503
(Diff revision 1)
> +          }
> +        }
> +
> +        if (!askPermission_video && !askPermission_audio) {
> +          askPermission = false;
> +          static const char* cameraPermission = "MediaManagerVideo";

Nit: No need for this to be static (I'd rather we just inline "MediaManagerVideo").

::: dom/media/MediaManager.cpp:2506
(Diff revision 1)
> +          nsCOMPtr<nsIPermissionManager> mgr =
> +            do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);

s/mgr/permissionMgr/ (or just permissions?) to avoid confusion since we're in media mgr.

::: dom/media/MediaManager.cpp:2509
(Diff revision 1)
> +          if (NS_SUCCEEDED(rv)) {
> +            nsCOMPtr<nsIIOService> ioServ(do_GetIOService());
> +            nsCOMPtr<nsIURI> uri;
> +            rv = ioServ->NewURI(origin, nullptr, nullptr, getter_AddRefs(uri));
> +            if (NS_SUCCEEDED(rv)) {
> +              nsCOMPtr<nsIPrincipal> principal;
> +              rv = GetPrincipalFromOrigin(origin, getter_AddRefs(principal));
> +              if (NS_SUCCEEDED(rv)) {
> +                rv = mgr->AddFromPrincipal(
> +                  principal, cameraPermission, nsIPermissionManager::ALLOW_ACTION,
> +                  nsIPermissionManager::EXPIRE_SESSION, 0);
> +                if (NS_WARN_IF(NS_FAILED(rv))) {
> +                  return;
> +                }
> +              }
> +            }
> +          }

An early-bail pattern seems superior to this plough. This code will continue in light of many failures here.

::: dom/media/MediaManager.cpp:2539
(Diff revision 1)
> +        } else {
> +          devices_.Clear();
> +
> +          for (auto& video : videos) {
> +            devices_.AppendElement(video);
> +          }
> +          for (auto& audio : audios) {
> +            devices_.AppendElement(audio);

If the user is already sharing a camera, and JS requests camera+mic, this is going to prompt for both camera and mic, but limit the camera choices to the one already being streamed, is that right?

I think that's a valid idea, certainly better than mine (see comment 4), but will users seeing the prompt think it a bug that they can't choose another camera?

Maybe we should just let them pick freely in this case? I think I vote for that. Should simplify this code a bit too, since we would use the original list in that case (or maybe almost the same list, except with the already-live device moved to the head of this list, so that it'll be the default?)

Sorry for the back and forth.

::: dom/media/MediaManager.cpp:3479
(Diff revision 1)
> +  nsPIDOMWindowInner* window = nsGlobalWindow::GetInnerWindowWithId(aWindowId)->AsInner();
> +  MOZ_ASSERT(window);

window appears unused and can be removed.

::: dom/media/MediaManager.cpp:3483
(Diff revision 1)
> +{
> +  nsPIDOMWindowInner* window = nsGlobalWindow::GetInnerWindowWithId(aWindowId)->AsInner();
> +  MOZ_ASSERT(window);
> +  StreamListeners* listeners = GetWindowListeners(aWindowId);
> +
> +  if (window && listeners) {

Use early-bail instead of if-else for less overall indendation.

::: dom/media/MediaManager.cpp:3484
(Diff revision 1)
> +  nsPIDOMWindowInner* window = nsGlobalWindow::GetInnerWindowWithId(aWindowId)->AsInner();
> +  MOZ_ASSERT(window);
> +  StreamListeners* listeners = GetWindowListeners(aWindowId);
> +
> +  if (window && listeners) {
> +    nsString rawId;

Declare rawId inside loop near its use.

::: dom/media/MediaManager.cpp:3486
(Diff revision 1)
> +  StreamListeners* listeners = GetWindowListeners(aWindowId);
> +
> +  if (window && listeners) {
> +    nsString rawId;
> +    uint32_t length = listeners->Length();
> +    for (uint32_t i = 0; i < length; ++i) {

Should be able to use for ( : ) here
Attachment #8810759 - Flags: review?(jib) → review-
(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> + new_mic case is a bad idea, since the script may stop the camera stream in
> the meantime while the permission prompt is up, leaving the request in an
> impossible state, unless we hold the camera stream alive while the prompt is
> up, which would be bad I think.

May I know if there is any sample of stopping camera stream in the mean time while the permission prompt is up?
I tried below two samples and both work fine with the patch.

https://jsfiddle.net/munrochiang/64aasonz/
https://jsfiddle.net/munrochiang/p05gdzx2/
Flags: needinfo?(jib)
That's not what I meant. The problem of stopping a stream applied to my initial idea in comment 2, which was to only show a prompt requesting audio for a request for existing_camera + new_mic. Your patch works differently from my original idea, as I touch on in comment 5, and doesn't have that problem. However, I don't think limiting the choices of cameras in the prompt, if we're having a prompt, in that case is the right idea either.

That said, I'm having trouble getting your patch to work. When I call gUM({video:true}) with a stream already running, nothing happens. Will investigate.
Flags: needinfo?(jib)
It appears your patch does not work with e10s:

[Child 12058] ###!!! ASSERTION: Cannot perform action in content process!: 'Error', file /Users/Jan/moz/mozilla-central/extensions/cookie/nsPermissionManager.cpp, line 1514
[Child 12058] WARNING: 'NS_FAILED(rv)', file /Users/Jan/moz/mozilla-central/dom/media/MediaManager.cpp, line 2445

Kinda makes sense, because if it were that easy to override permissions, then this whole sandboxing permission thing would be a bit of a waste.

I'm wondering if we can partly solve this in webrtcUI.jsm, maybe tag the sources that are live, and do the final skipping of permission there?
Hmm, though that would be equally exploitable if the content process is compromised, I suppose. Perhaps CameraParent can figure out on it's own that the source is already in use, and allow it? That would seem to make the most sense, security wise. gcp WDYT?
Flags: needinfo?(gpascutto)
Doesn't sound like a good idea if we move to multiple content processes. CamerasParent would need to start tracking what origin is busy with what source (and what content process it belongs to!) and at that point it's more than duplicating nsIPermissionManager. But, we did talk about doing exactly that before, so...
Flags: needinfo?(gpascutto)
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review93248

> Why is this needed? (it may be, I'm just not seeing it)

Allow MediaManager::IsDeviceConnectedToALiveMediaStreamTrack() to access mVideoDevice.

> Should be able to use for ( : ) here

Using for ( : ) will cause build break.

error C3312: no callable 'begin' function found for type 'mozilla::StreamListners *'
error C3312: no callable 'end' function found for type 'mozilla::StreamListners *'
I will submit revision 3 for Android Firefox later.
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review94076

Works! Though I fear gcp may take issue with this approach as it adds a bit of a backdoor to his camera permission sandboxing effort. A compromised content process can get cam and mic access easily now. Not sure what else to do though. ni'ing him for ideas. Waiting with r+ until we hear what he suggests.

::: dom/media/MediaManager.cpp:3486
(Diff revision 1)
> +  StreamListeners* listeners = GetWindowListeners(aWindowId);
> +
> +  if (window && listeners) {
> +    nsString rawId;
> +    uint32_t length = listeners->Length();
> +    for (uint32_t i = 0; i < length; ++i) {

This worked for me:

    for (auto& listener : *listeners) {

::: browser/modules/ContentWebRTC.jsm:27
(Diff revision 2)
> -    Services.obs.addObserver(handleGUMRequest, "getUserMedia:request", false);
> +    Services.obs.addObserver(handlePromptedGUMRequest, "getUserMedia:Promptedrequest", false);
> +    Services.obs.addObserver(handleUnPromptedGUMRequest, "getUserMedia:UnPromptedrequest", false);

I think we should leave "getUserMedia:request" alone, or there are lots of tests [1] we need to update.

The other one should be camelCase (unprompted is one word) "getUserMedia:unpromptedRequest".

[1] https://dxr.mozilla.org/mozilla-central/search?q=%22getUserMedia%3Arequest%22&case=true

::: browser/modules/ContentWebRTC.jsm:137
(Diff revision 2)
> +
> +function handleUnPromptedGUMRequest(aSubject, aTopic, aData) {
> +  handleGUMRequest(aSubject, aTopic, aData, false);
> +}
> +
> +function handleGUMRequest(aSubject, aTopic, aData, aPrompted) {

Nit: If we default aPrompted = true then we only need one stub.

::: browser/modules/ContentWebRTC.jsm:162
(Diff revision 2)
>      },
>      aSubject.innerWindowID,
>      aSubject.callID);
>  }
>  
> -function prompt(aContentWindow, aWindowID, aCallID, aConstraints, aDevices, aSecure) {
> +function prompt(aContentWindow, aWindowID, aCallID, aConstraints, aDevices, aSecure, aPrompted) {

Nit: A little odd to have a prompted boolean argument to a function named prompt. Maybe reverse the arg and call it bypass? That's sort of what it is anyway.

::: dom/media/MediaManager.cpp:2371
(Diff revision 2)
> +        nsTArray<RefPtr<VideoDevice>> videos;
> +        nsTArray<RefPtr<AudioDevice>> audios;

There's a bit of extra code we don't need here anymore now. E.g. videos and audios don't need to be arrays anymore (can be video and audio), and the big for-loop can probably be broken in two, each using break instead of continue.

::: dom/media/MediaManager.cpp:2437
(Diff revision 2)
> +        if (askVideoPermission || askAudioPermission) {
> +          RefPtr<GetUserMediaRequest> req =
> +            new GetUserMediaRequest(window, callID, c, isHTTPS);
> +          obs->NotifyObservers(req, "getUserMedia:Promptedrequest", nullptr);
> +        } else {
> -        RefPtr<GetUserMediaRequest> req =
> +          RefPtr<GetUserMediaRequest> req =
>              new GetUserMediaRequest(window, callID, c, isHTTPS);
> -        obs->NotifyObservers(req, "getUserMedia:request", nullptr);
> +          obs->NotifyObservers(req, "getUserMedia:UnPromptedrequest", nullptr);
> +        }

Nit: move duplicated common code out of if-else
Attachment #8810759 - Flags: review?(jib) → review-
See comment 14.
Flags: needinfo?(gpascutto)
rev 3: add support for Android Firefox.
I will wait for gcp's comment and then fix the nits in rev 4.

Basically I just mimic the way we grant "always share" permission.
MediaManager --> ContentWebRTC.jsm --> webrtcUI.jsm(chrome process) decides to grant the permission directly or having a prompt.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #14)
> Works! Though I fear gcp may take issue with this approach as it adds a bit
> of a backdoor to his camera permission sandboxing effort. A compromised
> content process can get cam and mic access easily now. Not sure what else to
> do though. ni'ing him for ideas. Waiting with r+ until we hear what he
> suggests.

I'm not clear how Sandboxing can be affected given the location of these changes. CamerasParent isn't involved, and there's no permission tinkering in content that's expecting to bypass origins.

If a compromised content process is already receiving a video stream, the attacker can just pluck the camera data out of memory and he doesn't need to do a second gUM, so whether or not that gUM will request permissions is irrelevant.

My remark in comment 10 was about this case:

Chrome/Parent -> Child 1 on Origin A -> Receiving video from device 1
                 Child 2 on Origin B -> gUM request for device 1

Obviously, we'd want B to have to re-request permissions. Note that if B is compromised it could actually just *claim* that it is A, but it has to be able to know somehow that a child with "A" exists and already is receiving video. (That might be possible if nsIPermissionsManager is queriable within content. Even if that's the case, if no video is running, 2 is still blocked from doing harm. Also note that with dom.ipc.processcount=1 we're always in the latter case because origins are same-process.)
Flags: needinfo?(gpascutto)
Yes there is permission tinkering here (search for "bypass" in comment 14).

(In reply to Munro Mengjue Chiang [:mchiang] from comment #17)
> Basically I just mimic the way we grant "always share" permission.
> MediaManager --> ContentWebRTC.jsm --> webrtcUI.jsm(chrome process) decides to grant the permission
> directly or having a prompt.

The difference is a compromised content process can't do anything to obtain "Always Share" rights, whereas with this patch it just needs to s/"getUserMedia:request"/"getUserMedia:Unpromptedrequest" when notifying observers to get it.  It doesn't need to already have obtained a gUM stream to do this.
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review94186

::: mobile/android/chrome/content/WebrtcUI.js:315
(Diff revision 3)
>  
> -    NativeWindow.doorhanger.show(message, "webrtc-request", buttons, BrowserApp.selectedTab.id, options, "WEBRTC");
> +      NativeWindow.doorhanger.show(message, "webrtc-request", buttons, BrowserApp.selectedTab.id, options, "WEBRTC");
> +    } else {
> +      if (videoDevices.length) {
> +        let perms = Services.perms;
> +        perms.add(uri, "MediaManagerVideo", perms.ALLOW_ACTION, perms.EXPIRE_SESSION);

As jib observed, a compromised content process can just send a "getUserMedia:UnPromptedrequest" with an uri of it's choosing and obtain the camera, zero questions asked, regardless whether the user every gave any permissions.

That's not good.
Attachment #8810759 - Flags: review-
After discussing with gcp on irc [1], a different approach for this seems needed. Specifically, this patch breaks protection for users who never granted persistent permission to any site, from a compromised content process.

Unless the parent process somehow verifies that the content process already has a camera or mic, we see no way to securely bypass a prompt from decisions made in the content process.

webrtcUI.jsm (which is in the parent process) could track what streams have been handed out already in a realm, and already does this for persistent permissions, but this is not a persistent permission (it's not even a temporary one according to the mediacapture spec which keeps it separate from the permissions spec's definition of that).

One way to do this would be to register temporary and internal (realm-expiring) permissions (they'd need to be internal and not observable through permission.query() for spec reasons) in the parent process whenever gUM is granted to a realm, and also revoke these permissions once the final track.stop() is called (hopefully some of the existing callbacks used for indicators may be reused for this).

Open to other suggestions.

[1] http://logs.glob.uno/?c=mozilla%23media&s=18+Nov+2016&e=18+Nov+2016#c304289
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review94672
Attachment #8810759 - Flags: review?(jib)
I have made a prototype which grant the internal realm permission.
However, it's not easy to revoke it.
The callback "webrtc:UpdateBrowserIndicators" doesn't give too much information except windowID / show camera icon (boolean) / show mic icon (boolean) / show screen icon (boolean).
Even we expose more information through it, the security sandbox still rely on a non-compromised content process because a compromised one can forge the information.
(In reply to Munro Mengjue Chiang [:mchiang] from comment #23)
> Even we expose more information through it, the security sandbox still rely
> on a non-compromised content process because a compromised one can forge the
> information.

In the case where we have multiple content processes and the compromised one is not the one receiving video, the current defense is that it would have to guess a site that has camera permissions *while* the permission is active. I think jib's proposal and your implementation preserve that property. It opens a larger window during which one can do the guessing, but if that is what the spec requires, then we have no choice.
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review96914

OK for the approach. Maybe you'd want to prefix the permission you set with some identifier, though, instead of only windowid + videodeviceid. "ActiveStreamPermission_" or something like that.
Attachment #8810759 - Flags: review?(gpascutto) → review+
In the current patch it seems these permissions are only revoked when receiving a message from the content process. I think they need to be revoked also when the user stops a stream from the browser UI, ie. this code path: http://searchfox.org/mozilla-central/rev/0c055ccbcf96846044fc9a71421bd9b7978686f7/browser/base/content/browser.js#7435

Also, this needs test coverage.
(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> I think they need to be
> revoked also when the user stops a stream from the browser UI
Done in rev 6.

> Also, this needs test coverage.
Will do.
(In reply to Munro Mengjue Chiang [:mchiang] from comment #30)
> (In reply to Florian Quèze [:florian] [:flo] from comment #28)
> > I think they need to be
> > revoked also when the user stops a stream from the browser UI
> Done in rev 6.

Is MediaManager::StopSharingCallback running in the parent or content process?
(In reply to Florian Quèze [:florian] [:flo] from comment #31)
> (In reply to Munro Mengjue Chiang [:mchiang] from comment #30)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #28)
> > > I think they need to be
> > > revoked also when the user stops a stream from the browser UI
> > Done in rev 6.
> 
> Is MediaManager::StopSharingCallback running in the parent or content
> process?

It is running in content process, but it's triggered by the snippet you mentioned.

{chrome process}
http://searchfox.org/mozilla-central/rev/0c055ccbcf96846044fc9a71421bd9b7978686f7/browser/base/content/browser.js#7435

{content process}
http://searchfox.org/mozilla-central/rev/0c055ccbcf96846044fc9a71421bd9b7978686f7/browser/modules/ContentWebRTC.jsm#90

http://searchfox.org/mozilla-central/rev/0c055ccbcf96846044fc9a71421bd9b7978686f7/dom/media/MediaManager.cpp#3036

http://searchfox.org/mozilla-central/rev/0c055ccbcf96846044fc9a71421bd9b7978686f7/dom/media/MediaManager.cpp#2689
(In reply to Munro Mengjue Chiang [:mchiang] from comment #32)
> (In reply to Florian Quèze [:florian] [:flo] from comment #31)
> > (In reply to Munro Mengjue Chiang [:mchiang] from comment #30)
> > > (In reply to Florian Quèze [:florian] [:flo] from comment #28)
> > > > I think they need to be
> > > > revoked also when the user stops a stream from the browser UI
> > > Done in rev 6.
> > 
> > Is MediaManager::StopSharingCallback running in the parent or content
> > process?
> 
> It is running in content process, but it's triggered by the snippet you
> mentioned.

I'm concerned that if this isn't entirely in the chrome process, a compromised content process could work around the user's Stop Sharing action by preventing the getUserMedia:revokePermRequest notification from being sent.

Looking at this patch a bit more:
- the changes in browser/modules/webrtcUI.jsm are adding more code duplication than I would like.
- I wonder why you decided to use the permission manager to set ActiveStreamPermission_* permissions, and if it does something that you wouldn't get by just setting properties at the same time as browser._devicePermissionURIs is modified. This latter solution would let you easily cleanup from the chrome process when the user stops sharing.
(In reply to Florian Quèze [:florian] [:flo] from comment #33)
> I'm concerned that if this isn't entirely in the chrome process, a
> compromised content process could work around the user's Stop Sharing action
> by preventing the getUserMedia:revokePermRequest notification from being
> sent.
We had the same concern. Please refer to comment 23 and comment 24.
Currently webrtcui/chrome process can control the permission granting because the flow must go through it and user will be aware of this via the prompt. So a compromised content process cannot forge a fake request and get the permission silently.

However, for the revoke part, webrtcui rely on mediamanager's notification to know if a stream is stopped. In this case, a compromised content can forge the information no matter how we design the revoke mechanism. In the long run, we may need to put more critical component into chrome process to solve this problem.

> 
> Looking at this patch a bit more:
> - the changes in browser/modules/webrtcUI.jsm are adding more code
> duplication than I would like.
We may be able to reuse the snippet for persistent permission (always share).
Here I use a isolated block and some duplicated code to prevent impacting the behavior of always share.
I can have an extension bug to refactor it afterward.

> - I wonder why you decided to use the permission manager to set
> ActiveStreamPermission_* permissions, and if it does something that you
> wouldn't get by just setting properties at the same time as
> browser._devicePermissionURIs is modified. This latter solution would let
> you easily cleanup from the chrome process when the user stops sharing.
We need ActiveStreamPermission_* permissions. Please refer to comment 21. We need to register a temporary and internal (realm-expiring) permission because the permission is only for this window. If we put it in _devicePermissionURIs, then another tab would also have the permission before we revoke it.
(In reply to Munro Mengjue Chiang [:mchiang] from comment #34)

> However, for the revoke part, webrtcui rely on mediamanager's notification
> to know if a stream is stopped. In this case, a compromised content can
> forge the information no matter how we design the revoke mechanism.

To know when a stream has stopped because the webpage stopped it, we need the notification from the media manager service. But when the user revoked permission from the browser's UI, we don't need any notification.

> We may be able to reuse the snippet for persistent permission (always share).

Yes, please :-).

> Here I use a isolated block and some duplicated code to prevent impacting
> the behavior of always share.

The always share behavior has reasonable test coverage, so I'm not worried about its code being touched.

> > - I wonder why you decided to use the permission manager to set
> > ActiveStreamPermission_* permissions, and if it does something that you
> > wouldn't get by just setting properties at the same time as
> > browser._devicePermissionURIs is modified. This latter solution would let
> > you easily cleanup from the chrome process when the user stops sharing.
> We need ActiveStreamPermission_* permissions. Please refer to comment 21.

Comment 21 was just one suggestion. I'm suggesting something else.

> the permission is only for this window. If we put it in
> _devicePermissionURIs, then another tab would also have the permission
> before we revoke it.

In the references to browser._devicePermissionURIs, 'browser' is a different object for each tab. So no, another tab wouldn't get permissions. Another page loaded in the same tab may get permission if for some reason the notification from the content process never arrives, so we may want to clear the permission whenever a new page is loaded, just to be extra safe.
(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> In the references to browser._devicePermissionURIs, 'browser' is a different
> object for each tab. So no, another tab wouldn't get permissions. Another
> page loaded in the same tab may get permission if for some reason the
> notification from the content process never arrives, so we may want to clear
> the permission whenever a new page is loaded, just to be extra safe.
This realm-expiring permission is granted to a {uri, device raw ID}.
Is there any way to put this in _devicePermissionURIs?

> Yes, please :-).
> 
> The always share behavior has reasonable test coverage, so I'm not worried
> about its code being touched.
OK, I will address in the next revision.

> To know when a stream has stopped because the webpage stopped it, we need
> the notification from the media manager service. But when the user revoked
> permission from the browser's UI, we don't need any notification.
For this case (user revoked permission from the browser's UI), I can try to revoke the permission in webrtcUI without going through content manager. However, does webrtcUI keep a list of camera / microphone device rawId for all active streams?
> revoke the permission in webrtcUI without going through content manager.
typo, content process mediamanager
(In reply to Munro Mengjue Chiang [:mchiang] from comment #36)
> This realm-expiring permission is granted to a {uri, device raw ID}.
> Is there any way to put this in _devicePermissionURIs?
> 
> For this case (user revoked permission from the browser's UI), I can try to
> revoke the permission in webrtcUI without going through content manager.
> However, does webrtcUI keep a list of camera / microphone device rawId for
> all active streams?

If _devicePermissionURIs cannot be used, and we don't want to use permission manager, what about creating a new vector like webrtcUI._activeStreamTracks to store these data {uri, windowID, deviceRawID}?

When the user revoked permission from the browser's UI, we just need to remove all items with specified uri & windowID. Then we can do this in chrome process only.
In comment 33 I said "setting properties at the same time as browser._devicePermissionURIs is modified", not "put this in _devicePermissionURIs". I meant something like browser._activeStreamTracks.

The reason for putting this in browser._something rather than webrtcUI._something was that we need to keep the information associated with the tab. Another (possibly better) solution would be a WeakMap[1] in webrtcUI, where the key would be the browser, and the value would be an object like {windowID, deviceRawID}

webrtcUI already has a _streams arrays, but I don't think it's very helpful for what we want to do here, so we could either modify it, or create something else, eg. webrtcUI._activeDevicePermissions.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap
(In reply to Florian Quèze [:florian] [:flo] from comment #39)
> In comment 33 I said "setting properties at the same time as
> browser._devicePermissionURIs is modified", not "put this in
> _devicePermissionURIs". I meant something like browser._activeStreamTracks.
> 
> The reason for putting this in browser._something rather than
> webrtcUI._something was that we need to keep the information associated with
> the tab. Another (possibly better) solution would be a WeakMap[1] in
> webrtcUI, where the key would be the browser, and the value would be an
> object like {windowID, deviceRawID}
> 
> webrtcUI already has a _streams arrays, but I don't think it's very helpful
> for what we want to do here, so we could either modify it, or create
> something else, eg. webrtcUI._activeDevicePermissions.
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/WeakMap

I will deliver 3 changes in the next revision. Please let me know if there is any other concern.
1) use browser._activeStreamTracks instead of permission manager
2) reuse the snippet for persistent permission
3) revoke permission in chrome process without going through mediamanager
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review98098

I see florian has already given good feedback so I don't have too much to add. He's also a good person to review UI changes anyway. Removing r? from me for now.

::: browser/modules/ContentWebRTC.jsm:28
(Diff revisions 3 - 6)
>      if (this._initialized)
>        return;
>  
>      this._initialized = true;
> -    Services.obs.addObserver(handlePromptedGUMRequest, "getUserMedia:request", false);
> -    Services.obs.addObserver(handleUnPromptedGUMRequest, "getUserMedia:UnPromptedrequest", false);
> +    Services.obs.addObserver(handleGUMRequest, "getUserMedia:request", false);
> +    Services.obs.addObserver(handleGUMRevokePermRequest, "getUserMedia:revokePermRequest", false);

I had trouble initially understanding what 'revokePermRequest' meant. The word "request" as a noun usually refers to the permission prompt (e.g. "webrtc:CancelRequest").

We're not revoking regular "permission" here, nor removing a prompt - rather we're removing the implicit permission used internally for sharing.

How about handleGUMStop and "getUserMedia:stopSharing" instead?

::: dom/media/MediaManager.cpp:2269
(Diff revisions 3 - 6)
>      }
>  
>      uint32_t videoPerm = nsIPermissionManager::UNKNOWN_ACTION;
>      if (IsOn(c.mVideo)) {
>        rv = permManager->TestExactPermissionFromPrincipal(
> -        principal, "camera", &videoPerm);
> +        principal, videoType == MediaSourceEnum::Camera ? "camera" : "screen",

Have you tested this with screensharing?

We might want to test for the right MediaSourceEnums here explicitly rather than default to "screen", just to be sure we're not bleeding permission from some future type to screensharing, just because screensharing is such a sensitive to share and can be exploited.

::: dom/media/MediaManager.cpp:2653
(Diff revisions 3 - 6)
> -                    StreamListeners *aListeners,
> +                                  StreamListeners *aListeners,
> -                    void *aData)
> +                                  void *aData)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    if (aListeners) {
> +    nsString rawId;

Move declaration of rawId down near its use(s).

::: dom/media/MediaManager.cpp:2656
(Diff revisions 3 - 6)
> +    RefPtr<nsPIDOMWindowInner> window = globalWindow ? globalWindow->AsInner()
> +                                                     : nullptr;

Do we need to refcount here?

::: browser/modules/ContentWebRTC.jsm:129
(Diff revision 6)
>      secure: isSecure,
>    };
>    mm.sendAsyncMessage("rtcpeer:Request", request);
>  }
>  
> +function handleGUMRevokePermRequest(aSubject, aTopic, aData) {

Note you could use destructuring here if you want:

    function handleGUMRevokePermRequest({ windowID, callID }, aTopic, aData) {
Attachment #8810759 - Flags: review?(jib)
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review98098

> Have you tested this with screensharing?
> 
> We might want to test for the right MediaSourceEnums here explicitly rather than default to "screen", just to be sure we're not bleeding permission from some future type to screensharing, just because screensharing is such a sensitive to share and can be exploited.

I forgot to mention there is a rebase between rev 4 and 5.
This change is not made by me.
revision 9 fixes the timeout.
browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js also breaks. Checking.
revision 10 fixes browser_devices_get_user_media_in_frame.js break.
Attachment #8813603 - Attachment is obsolete: true
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review100298

This isn't a full review, I have already left enough comments that I'll need to look at the whole patch again next time.

::: browser/modules/ContentWebRTC.jsm:28
(Diff revision 12)
>      if (this._initialized)
>        return;
>  
>      this._initialized = true;
>      Services.obs.addObserver(handleGUMRequest, "getUserMedia:request", false);
> +    Services.obs.addObserver(handleGUMStop, "getUserMedia:stopSharing", false);

This notification name is misleading. I think you want something like recording-device-stopped

Or could we extend the existing recording-device-events notififcation so that it provides the information we need?

::: browser/modules/ContentWebRTC.jsm:134
(Diff revision 12)
> +function handleGUMStop(aSubject, aTopic, aData) {
> +  let contentWindow = Services.wm.getOuterWindowWithId(aSubject.windowID);
> +
> +  let request = {
> +    windowID: aSubject.windowID,
> +    rawID: aSubject.callID,

I don't understand how you can transform a callID to a raw device id.

::: browser/modules/webrtcUI.jsm:142
(Diff revision 12)
>  
>    forgetStreamsFromBrowser: function(aBrowser) {
>      this._streams = this._streams.filter(stream => stream.browser != aBrowser);
>    },
>  
> +  stopSharing: function(aBrowser) {

This name is very misleading, everywhere else "stopSharing" means "the user has revoked this page's permissions to use the device, stop the active streams now".

I think you want to call this "forgetActivePermissionsFromBrowser" to be consistent with "forgetStreamsFromBrowser" above.

::: browser/modules/webrtcUI.jsm:666
(Diff revision 12)
>              // (it's really one-shot, not for the entire session)
>              perms.add(uri, "MediaManagerVideo", perms.ALLOW_ACTION,
>                        perms.EXPIRE_SESSION);
> +            if (!webrtcUI._activeDevicePermissions.has(aBrowser)) {
> +              let s = new Set();
> +              webrtcUI._activeDevicePermissions.set(aBrowser, s);

you can inline "new Set()" here instead of using the temporary 's' variable. This applies throughout the patch.

::: browser/modules/webrtcUI.jsm:668
(Diff revision 12)
>                        perms.EXPIRE_SESSION);
> +            if (!webrtcUI._activeDevicePermissions.has(aBrowser)) {
> +              let s = new Set();
> +              webrtcUI._activeDevicePermissions.set(aBrowser, s);
> +            }
> +            let s = webrtcUI._activeDevicePermissions.get(aBrowser);

Could you please use a more explicit variable name? Myabe "activePerms"? This applies throughout the patch.

::: browser/modules/webrtcUI.jsm:669
(Diff revision 12)
> +            if (!webrtcUI._activeDevicePermissions.has(aBrowser)) {
> +              let s = new Set();
> +              webrtcUI._activeDevicePermissions.set(aBrowser, s);
> +            }
> +            let s = webrtcUI._activeDevicePermissions.get(aBrowser);
> +            s.add(aRequest.windowID + videoDevices[videoDeviceIndex].id);

You need a separator between the window id and the device id. Otherwise if the windowId is 1 and the device id is "12", how will you know that it's not windowId = 11 and deviceId = 2?

You probably also want an audio: and video: prefix, unless you are sure the audio and video device ids will always be different.

::: browser/modules/webrtcUI.jsm:687
(Diff revision 12)
> +              if (!webrtcUI._activeDevicePermissions.has(aBrowser)) {
> +                let s = new Set();
> +                webrtcUI._activeDevicePermissions.set(aBrowser, s);
> +              }
> +              let s = webrtcUI._activeDevicePermissions.get(aBrowser);
> +              s.add(aRequest.windowID + audioDevices[audioDeviceIndex - videoDevices.length].id);

I can't figure out what "- videoDevices.length" is meant to do here.

::: dom/media/MediaManager.cpp:2784
(Diff revision 12)
> +    auto* globalWindow = nsGlobalWindow::GetInnerWindowWithId(aWindowID);
> +    RefPtr<nsPIDOMWindowInner> window = globalWindow ? globalWindow->AsInner()
> +                                                     : nullptr;
> +    RefPtr<GetUserMediaRequest> req =
> +      new GetUserMediaRequest(window, NullString());
> +    obs->NotifyObservers(req, "getUserMedia:stopSharing", nullptr);

Which case is this? If it's when the window has already been closed, we have likely cleaned up on the webrtcUI side when the browser tab got closed or when the page was navigated away, so I'm not sure we need a notification here.
Attachment #8810759 - Flags: review?(florian) → review-
Comment on attachment 8818829 [details]
Bug 1270572 - write tests for the umprompted gUM request;

https://reviewboard.mozilla.org/r/98768/#review100310

You will want a lot more tests. I think you'll want to have your test in a new file in the test/webrtc/ folder.

Eg. check that
if there's an active camera stream
  gUM(audio) causes a prompt
  gUM(audio+camera) causes a prompt
  gUM(screen sharing) causes a prompt
  gUM(camera) returns a stream without prompting

if there's an active audio stream
  gUM(audio) returns a stream without prompting
  gUM(audio+camera) prompts
  gUM(camera) prompts

if there's an active camera+audio stream
  gUM(screensharing+audio) prompts

if there's an active stream in frame a, gUM() in frame b should prompt.
if there's an active stream in frame a, gUM() in the top level window should prompt.

if a stream is started, then the tab is detached to another window, check that we don't prompt when requesting the same device.

::: browser/base/content/test/webrtc/browser_devices_get_user_media.js:79
(Diff revision 4)
> +    Assert.deepEqual((yield getMediaCaptureState()), {audio: true, video: true},
> +                     "expected camera and microphone to be shared");
> +
> +    yield checkSharingUI({audio: true, video: true});
> +    yield closeStream(false, 0, true);
> +    yield closeStream(false, 0, false);

You want to check that after closing all streams, another request will show a prompt.

::: browser/base/content/test/webrtc/get_user_media.html:28
(Diff revision 4)
>  }
>  
>  var gStream;
> +var gSecondStream;
>  
> -function requestDevice(aAudio, aVideo, aShare) {
> +function requestDevice(aAudio, aVideo, aShare, aSecondStream) {

Could you make gStream an array (rename to gStreams), and instead of the aSecondStream parameter, make it "aStreamId = 0"?

::: browser/base/content/test/webrtc/get_user_media.html:63
(Diff revision 4)
> -    return;
> +      return;
> -  gStream.getTracks().forEach(t => t.stop());
> +    gStream.getTracks().forEach(t => t.stop());
> -  gStream = null;
> +    gStream = null;
> -  message("closed");
> +    message("closed");
> +  } else {
> +    if (!gSecondStream)

This check doesn't seem useful.
Attachment #8818829 - Flags: review?(florian) → review-
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review100298

Sure, thanks. I would like to lock down the issues before making the next move.

> I don't understand how you can transform a callID to a raw device id.

For the stop recording case, since we don't need the callID, I reuse it to store the device raw ID.
To prevent confusion, I will add below attribute to GetUserMediaRequest.webidl.
readonly attribute DOMString rawID;

> This name is very misleading, everywhere else "stopSharing" means "the user has revoked this page's permissions to use the device, stop the active streams now".
> 
> I think you want to call this "forgetActivePermissionsFromBrowser" to be consistent with "forgetStreamsFromBrowser" above.

I will change the name to forgetActivePermissionsFromBrowser.

> you can inline "new Set()" here instead of using the temporary 's' variable. This applies throughout the patch.

will do.

> Could you please use a more explicit variable name? Myabe "activePerms"? This applies throughout the patch.

will do.

> You need a separator between the window id and the device id. Otherwise if the windowId is 1 and the device id is "12", how will you know that it's not windowId = 11 and deviceId = 2?
> 
> You probably also want an audio: and video: prefix, unless you are sure the audio and video device ids will always be different.

device RawID is very long and uniqle string like 'CC24367K0R0F6VVDE'.
So this should be fine. I will still add a separator though.

> I can't figure out what "- videoDevices.length" is meant to do here.

mediaManager put both video devices and audio devices in the same vector.
Video devices first then audio devices.
So the first audio device index is videoDevices.length.
In my first draft I use audioDevices[audioDeviceIndex].id and always got wrong id.
Then I figured out this.

> Which case is this? If it's when the window has already been closed, we have likely cleaned up on the webrtcUI side when the browser tab got closed or when the page was navigated away, so I'm not sure we need a notification here.

This is for the case javascript call MediaStreamTrack.stop(). The window is not closed.
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review100298

> This is for the case javascript call MediaStreamTrack.stop(). The window is not closed.

Sorry, this comment was wrong.
This case is when we reload a frame.
For example, press Run in https://jsfiddle.net/ddwrjdmv/
(In reply to Munro Mengjue Chiang [:mchiang] from comment #61)

> device RawID is very long and uniqle string like 'CC24367K0R0F6VVDE'.
> So this should be fine. I will still add a separator though.

So for cameras the rawID I get is a string similar to your example.

For microphones, the rawID is identical to the name, ie. I get "default: Internal Microphone" and "Internal Microphone" as ids. (I wonder if this is a bug)

For a screen I get an id that's a large number, eg. 458600034

For windows, I get ids that are numbers, eg 86 120 492 33253 47365

For applications, I also get ids that are numbers, eg. 248 1498 72071

I see no way to distinguish these without a prefix. Also, be careful when choosing (or parsing) the separator, as it seems any printable character can be included in the audio device id.

> > I can't figure out what "- videoDevices.length" is meant to do here.
> 
> mediaManager put both video devices and audio devices in the same vector.
> Video devices first then audio devices.
> So the first audio device index is videoDevices.length.
> In my first draft I use audioDevices[audioDeviceIndex].id and always got
> wrong id.
> Then I figured out this.

Took me a while to understand this. The deviceIndex is assigned in ContentWebRTC.jsm. That it happens to match the order returned by MediaManager is just an implementation detail that we shouldn't rely on.

For screensharing devices, we actually save the id on the menuitem: http://searchfox.org/mozilla-central/rev/cc2a84852bd4e6f6d8d4d5b17b8382bb5d005749/browser/modules/webrtcUI.jsm#489
We can do something similar for cameras and microphones, and then you can reliably get back to the id.
Attachment #8818829 - Attachment is obsolete: true
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review102530

::: browser/modules/ContentWebRTC.jsm:139
(Diff revision 14)
> +    rawID: aSubject.rawID,
> +    isVideo: aSubject.isVideo,
> +  };
> +
> +  let mm = getMessageManagerForWindow(contentWindow);
> +  mm.sendAsyncMessage("webrtc:StopSharing", request);

There's already a "webrtc:StopSharing" message being sent from the chrome process (in browser.js) to the content process. Having 2 different messages with the exact same name is very confusing, please find a better name here.

::: browser/modules/webrtcUI.jsm:142
(Diff revision 14)
>  
>    forgetStreamsFromBrowser: function(aBrowser) {
>      this._streams = this._streams.filter(stream => stream.browser != aBrowser);
>    },
>  
> +  forgetActivePermissionsFromBrowser: function(aBrowser) {

You'll need to rebase the patch on top of bug 1325464.

::: browser/modules/webrtcUI.jsm:295
(Diff revision 14)
>      }
>    }
>    return host;
>  }
>  
> +function stopSharing(aBrowser, aRequest) {

This name should be changed too.

::: browser/modules/webrtcUI.jsm:301
(Diff revision 14)
> +  if (webrtcUI.activePerms.has(aBrowser.outerWindowID)) {
> +    if (!aRequest.rawID) {
> +      webrtcUI.activePerms.delete(aBrowser.outerWindowID);
> +    } else if (aRequest.isVideo) {
> +      if (webrtcUI.activePerms.get(aBrowser.outerWindowID).
> +        has(aRequest.windowID + "_VIDEO_" + aRequest.rawID)) {

When using hardcoded strings like this, they should be in constants, preferably at the top of the file. But using _VIDEO_ or _AUDIO_ isn't enough to avoid collisions between window ids and application ids, as shown in comment 63.

Please use instead the strings from device.mediaSource (eg. "screen", "application", "camera", "microphone", ...).

::: browser/modules/webrtcUI.jsm:307
(Diff revision 14)
> +        webrtcUI.activePerms.get(aBrowser.outerWindowID).
> +          delete(aRequest.windowID + "_VIDEO_" + aRequest.rawID);
> +      }
> +    } else {
> +      if (webrtcUI.activePerms.get(aBrowser.outerWindowID).
> +        has(aRequest.windowID + "_AUDIO_" + aRequest.rawID)) {

This indentation isn't correct. You'll want to put webrtcUI.activePerms.get(aBrowser.outerWindowID) in a temporary variable.

::: browser/modules/webrtcUI.jsm:461
(Diff revision 14)
>          // We don't check that permissions are set to ALLOW_ACTION in this
>          // test; only that they are set. This is because if audio is allowed
>          // and video is denied persistently, we don't want to show the prompt,
>          // and will grant audio access immediately.
> +        if (((!audioDevices.length || micPerm) && (!videoDevices.length || camPerm)) ||
> +            ((!audioDevices.length || tmpMicPerm) && (!videoDevices.length || tmpCamPerm))) {

You'll need to update the comment to explain what the test is now doing. Part of this test is duplicated 3 lines later, please avoid that using a temporary variable. Also, this test is really hard to understand currently, is there a way to make it more readable? (I'm not sure I really understood what it does)

::: browser/modules/webrtcUI.jsm:475
(Diff revision 14)
> -          }
> +            }
> -          if (audioDevices.length && micPerm == perms.ALLOW_ACTION)
> +            if (audioDevices.length && micPerm == perms.ALLOW_ACTION)
> -            allowedDevices.push(audioDevices[0].deviceIndex);
> +              allowedDevices.push(audioDevices[0].deviceIndex);
> +          } else {
> +            allowedDevices = allowedTmpDevices;
> +            Services.perms.add(uri, "MediaManagerVideo",

It seems this is adding video permissions even if we are only sharing an audio device.

::: dom/webidl/GetUserMediaRequest.webidl:17
(Diff revision 14)
>    readonly attribute unsigned long long innerWindowID;
>    readonly attribute DOMString callID;
> +  readonly attribute DOMString rawID;
>    MediaStreamConstraints getConstraints();
>    readonly attribute boolean isSecure;
> +  readonly attribute boolean isVideo;

You'll need to request review on the changes in this file from someone else (likely jib), but this way to extend the existing GetUserMediaRequest interface to make it do something different doesn't seem great to me. Creating a new separate interface for the new notification may be better. It seems all we need in this interface is a window id, a device raw id and the device's mediaSource.
Attachment #8810759 - Flags: review?(florian) → review-
(In reply to Florian Quèze [:florian] [:flo] from comment #69)
> ::: dom/webidl/GetUserMediaRequest.webidl:17
> (Diff revision 14)
> >    readonly attribute unsigned long long innerWindowID;
> >    readonly attribute DOMString callID;
> > +  readonly attribute DOMString rawID;
> >    MediaStreamConstraints getConstraints();
> >    readonly attribute boolean isSecure;
> > +  readonly attribute boolean isVideo;
> 
> You'll need to request review on the changes in this file from someone else
> (likely jib), but this way to extend the existing GetUserMediaRequest
> interface to make it do something different doesn't seem great to me.
> Creating a new separate interface for the new notification may be better. It
> seems all we need in this interface is a window id, a device raw id and the
> device's mediaSource.

jib, could you help provide your opinion? Thanks!
Flags: needinfo?(jib)
(In reply to Munro Mengjue Chiang [:mchiang] from comment #70)

Yeah I looked at that in comment 41. Florian is right it would be cleaner with a separate interface, but I wasn't going to make you do it. Up to you.

Either way, note you'll need a DOM reviewer for the webidl file, to avoid the dumb autoland bouncer.

Comment 41 is also where I suggested "stopSharing". Sorry about that. Open to a better name that is not taken.
Flags: needinfo?(jib)
(In reply to Munro Mengjue Chiang [:mchiang] from comment #73)
> Comment on attachment 8822119 [details]
> Bug 1270572 - write tests for the umprompted gUM request;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/101124/diff/3-4/

Version 4 here seems to revert the changes you did previously to address comment 59.
revision 5: backout to revision 3.
revision 16 & 6: rebase
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review103462

::: browser/base/content/browser.js:4914
(Diff revision 16)
>        let mm = gBrowser.selectedBrowser.messageManager;
>        mm.sendAsyncMessage("Reader:PushState", {isArticle: gBrowser.selectedBrowser.isArticle});
>        return;
>      }
>  
> +    webrtcUI.forgetActivePermissionsFromBrowser(aBrowser);

This line should be moved...

::: browser/base/content/browser.js:4928
(Diff revision 16)
>        PopupNotifications.locationChange(aBrowser);
>  
>      let tab = gBrowser.getTabForBrowser(aBrowser);
>      if (tab && tab._sharingState) {
>        gBrowser.setBrowserSharing(aBrowser, {});
>        webrtcUI.forgetStreamsFromBrowser(aBrowser);

... here.

And actually, I see no reason why forgetStreamsFromBrowser couldn't call forgetActivePermissionsFromBrowser directly.

::: browser/base/content/browser.js:7493
(Diff revision 16)
>              }
>            }
>          }
>          let mm = gBrowser.selectedBrowser.messageManager;
>          mm.sendAsyncMessage("webrtc:StopSharing", windowId);
> +        webrtcUI.forgetActivePermissionsFromBrowser(gBrowser.selectedBrowser);

I would prefer if we moved these 3 lines to webrtcUI.jsm and just called webrtcUI.stopSharing(gBrowser.selectedBrowser, windowId); here, but I'm not requiring this change, it would just be cleaner.

::: browser/modules/webrtcUI.jsm:151
(Diff revision 16)
>    forgetStreamsFromBrowser(aBrowser) {
>      this._streams = this._streams.filter(stream => stream.browser != aBrowser);
>    },
>  
> +  forgetActivePermissionsFromBrowser(aBrowser) {
> +    if (webrtcUI.activePerms.has(aBrowser.outerWindowID)) {

You don't need this test, Map.delete returns false if the element didn't exist, but won't throw an error.

::: browser/modules/webrtcUI.jsm:348
(Diff revision 16)
>    }
>    return host;
>  }
>  
> +function stopRecording(aBrowser, aRequest) {
> +  if (webrtcUI.activePerms.has(aBrowser.outerWindowID)) {

Please put outerWindowID in a temporary variable, it's used 4 times in this function.

I would prefer an early return after this test, to decrease the indent level.

::: browser/modules/webrtcUI.jsm:353
(Diff revision 16)
> +  if (webrtcUI.activePerms.has(aBrowser.outerWindowID)) {
> +    if (!aRequest.rawID) {
> +      webrtcUI.activePerms.delete(aBrowser.outerWindowID);
> +    } else {
> +      let set = webrtcUI.activePerms.get(aBrowser.outerWindowID);
> +      if (set.has(aRequest.windowID + aRequest.mediaSource + aRequest.rawID)) {

You don't need the has() test here. If you did, 'aRequest.windowID + aRequest.mediaSource + aRequest.rawID' should have been put in a temporary variable.

::: browser/modules/webrtcUI.jsm:354
(Diff revision 16)
> +    if (!aRequest.rawID) {
> +      webrtcUI.activePerms.delete(aBrowser.outerWindowID);
> +    } else {
> +      let set = webrtcUI.activePerms.get(aBrowser.outerWindowID);
> +      if (set.has(aRequest.windowID + aRequest.mediaSource + aRequest.rawID)) {
> +        webrtcUI.activePerms.get(aBrowser.outerWindowID).

This is the same thing as the content of the 'set' variable you already have.

::: browser/modules/webrtcUI.jsm:479
(Diff revision 16)
>            camPerm = perms.UNKNOWN_ACTION;
>  
> +        let tmpCamPerm = perms.UNKNOWN_ACTION;
> +        let tmpMicPerm = perms.UNKNOWN_ACTION;
> +        let allowedTmpDevices = [];
> +

Suggestion: instead of these 3 variables, have only these 2 variables:
let activeCamera;
let activeMic;

::: browser/modules/webrtcUI.jsm:482
(Diff revision 16)
> +        let tmpMicPerm = perms.UNKNOWN_ACTION;
> +        let allowedTmpDevices = [];
> +
> +        for (let device of videoDevices) {
> +          if (webrtcUI.activePerms.has(aBrowser.outerWindowID)) {
> +            if (webrtcUI.activePerms.get(aBrowser.outerWindowID).

let set = webrtcUI.activePerms.get(aBrowser.outerWindowID);
if (set && set.has(...

(same comment applies for the audioDevices block of code below)

::: browser/modules/webrtcUI.jsm:507
(Diff revision 16)
>          // We don't check that permissions are set to ALLOW_ACTION in this
>          // test; only that they are set. This is because if audio is allowed
>          // and video is denied persistently, we don't want to show the prompt,
>          // and will grant audio access immediately.
> -        if ((!audioDevices.length || micPerm) && (!videoDevices.length || camPerm)) {
> -          // All permissions we were about to request are already persistently set.
> +        let isAlwaysShare = (!audioDevices.length || micPerm) && (!videoDevices.length || camPerm);
> +        let isTemporalShare = (!audioDevices.length || tmpMicPerm) && (!videoDevices.length || tmpCamPerm);

I think you meant 'temporary' rather than 'temporal' here, but it's a long word so it's fine with me if you just name this 'isTempShare'.

::: browser/modules/webrtcUI.jsm:509
(Diff revision 16)
>          // and video is denied persistently, we don't want to show the prompt,
>          // and will grant audio access immediately.
> -        if ((!audioDevices.length || micPerm) && (!videoDevices.length || camPerm)) {
> -          // All permissions we were about to request are already persistently set.
> +        let isAlwaysShare = (!audioDevices.length || micPerm) && (!videoDevices.length || camPerm);
> +        let isTemporalShare = (!audioDevices.length || tmpMicPerm) && (!videoDevices.length || tmpCamPerm);
> +
> +        if ( isAlwaysShare || isTemporalShare ) {

nit: no space after ( and before )

I don't think the logic is correct here: if a page requesting camera+microphone already has an active camera stream and persistent microphone permission, I think this code will cause a prompt to appear. Please include this case in your tests.

I think this test should become:

if ((!audioDevices.length || micPerm == perms.ALLOW_ACTION || activeMic) && ...

Note: the comment at lines 450-453 (starting with "We don't check that permissions are set to ALLOW"...) is obsolete since bug 802326. You can remove or update it.

::: browser/modules/webrtcUI.jsm:514
(Diff revision 16)
> +        if ( isAlwaysShare || isTemporalShare ) {
>            let allowedDevices = [];
> +          if (isAlwaysShare) {
> +            // All permissions we were about to request are already persistently set.
> -          if (videoDevices.length && camPerm == perms.ALLOW_ACTION) {
> +            if (videoDevices.length && camPerm == perms.ALLOW_ACTION) {
> -            allowedDevices.push(videoDevices[0].deviceIndex);
> +              allowedDevices.push(videoDevices[0].deviceIndex);

Based on my previous suggestions, this can become:

if (videoDevices.length) {
  allowedDevices.push((activeCamera || videoDevices[0]).deviceIndex);
  ...

::: browser/modules/webrtcUI.jsm:521
(Diff revision 16)
> -                               Services.perms.ALLOW_ACTION,
> +                                 Services.perms.ALLOW_ACTION,
> -                               Services.perms.EXPIRE_SESSION);
> +                                 Services.perms.EXPIRE_SESSION);
> -          }
> +            }
> -          if (audioDevices.length && micPerm == perms.ALLOW_ACTION)
> +            if (audioDevices.length && micPerm == perms.ALLOW_ACTION)
> -            allowedDevices.push(audioDevices[0].deviceIndex);
> +              allowedDevices.push(audioDevices[0].deviceIndex);
> +          } else { // isTemporalShare == true

And we don't need this block of code anymore.

::: dom/webidl/GetUserMediaRequest.webidl:10
(Diff revision 16)
>   *
>   * This is an internal IDL file
>   */
>  
>  [NoInterfaceObject]
>  interface GetUserMediaRequest {

This needs a comment saying that when an object implementing this interface is passed for the recording-device-stopped notification, only windowID, rawID and mediaSource will be set. And that rawID and mediaSource won't be set when this interface is passed for the getUserMedia:request notification.
Attachment #8810759 - Flags: review?(florian) → review-
Comment on attachment 8822119 [details]
Bug 1270572 - write tests for the umprompted gUM request;

https://reviewboard.mozilla.org/r/101124/#review103504

This looks reasonable, but I haven't done a thorough review yet, I'll focus more on reviewing the tests once the code itself is ready. Some of the cases I mentioned in comment 59 aren't covered yet, and when thinking about this more I'll likely find more cases that need testing (feel free to also add test coverages for cases I haven't explicitly listed but that you think will be valuable to have).

I think we'll want shorter test descriptions (that can fit on one line) and helper functions to reduce code duplication.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access.js:41
(Diff revision 6)
> +                     "expected camera and microphone to be shared");
> +    yield indicator;
> +    yield checkSharingUI({audio: true, video: true});
> +
> +	// If there's an active audio+camera stream,
> +	// gUM(audio+camera) returns a stream without prompting;

the indent is incorrect here (please avoid using the tab character for indentation).

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_in_frame.js:9
(Diff revision 6)
> +
> +registerCleanupFunction(function() {
> +  gBrowser.removeCurrentTab();
> +});
> +
> +function promiseReloadFrame(aFrameId) {

Instead of copying this from browser_devices_get_user_media_in_frame.js, you can move it to head.js

::: browser/base/content/test/webrtc/head.js:385
(Diff revision 6)
>        global = global.document.getElementById(args.aFrameId).contentWindow;
>      global.requestDevice(args.aRequestAudio, args.aRequestVideo, args.aType);
>    });
>  }
>  
> -function* closeStream(aAlreadyClosed, aFrameId) {
> +function* closeStream(aAlreadyClosed, aFrameId, aNumOfStreams = 1) {

nit: 'aStreamCount' instead of 'aNumOfStreams'
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review103462

> Based on my previous suggestions, this can become:
> 
> if (videoDevices.length) {
>   allowedDevices.push((activeCamera || videoDevices[0]).deviceIndex);
>   ...

In case of camPerm == perms.DENY_ACTION, this snippet will push videoDevices[0]).deviceIndex into allowedDevices.
Here I have two versions of change. I prefer version 1.

1)
          if (videoDevices.length && ((camPerm == perms.ALLOW_ACTION) || activeCamera)) {
            if (camPerm == perms.ALLOW_ACTION) {
              allowedDevices.push(videoDevices[0].deviceIndex);
            } else {
              allowedDevices.push(activeCamera);
            }
            Services.perms.add(uri, "MediaManagerVideo",
                               Services.perms.ALLOW_ACTION,
                               Services.perms.EXPIRE_SESSION);
          }
2)
          if (videoDevices.length) {
            if (camPerm == perms.ALLOW_ACTION) {
              allowedDevices.push(videoDevices[0].deviceIndex);
              Services.perms.add(uri, "MediaManagerVideo",
                                 Services.perms.ALLOW_ACTION,
                                 Services.perms.EXPIRE_SESSION);
            } else if (activeCamera) {
              allowedDevices.push(activeCamera);
              Services.perms.add(uri, "MediaManagerVideo",
                                 Services.perms.ALLOW_ACTION,
                                 Services.perms.EXPIRE_SESSION);
            }
          }
(In reply to Munro Mengjue Chiang [:mchiang] from comment #82)
> Comment on attachment 8810759 [details]
> Bug 1270572 - allow un-prompted gUM access if the page has a live track
> connected to the same device;
> 
> https://reviewboard.mozilla.org/r/93086/#review103462
> 
> > Based on my previous suggestions, this can become:
> > 
> > if (videoDevices.length) {
> >   allowedDevices.push((activeCamera || videoDevices[0]).deviceIndex);
> >   ...
> 
> In case of camPerm == perms.DENY_ACTION, this snippet will push
> videoDevices[0]).deviceIndex into allowedDevices.

This case does not (or at the very least should not) happen since bug 802326, because when there's one permission set to DENY, MediaManager will deny the request immediately without even firing the getUserMedia:request notification.

This is covered by tests since https://hg.mozilla.org/integration/mozilla-inbound/rev/cef717a46cf0#l1.261

> 1)
>           if (videoDevices.length && ((camPerm == perms.ALLOW_ACTION) ||
> activeCamera)) {
>             if (camPerm == perms.ALLOW_ACTION) {
>               allowedDevices.push(videoDevices[0].deviceIndex);
>             } else {
>               allowedDevices.push(activeCamera);
>             }

It's really a matter of when there's both ALLOW persistent permission, and an activeCamera, whether we want to return the first device (videoDevices[0]), or the device that the user may have previously selected (activeCamera). I think the latter (which is the behavior my snippet produced) is better; but I don't know if this behavior is specified anywhere.
(In reply to Florian Quèze [:florian] [:flo] from comment #85)
> This case does not (or at the very least should not) happen since bug
> 802326, because when there's one permission set to DENY, MediaManager will
> deny the request immediately without even firing the getUserMedia:request
> notification.
Not sure if I understand this correctly, but according to the previous discussion about security sandbox, we are trying to avoid relying on MediaManager as much as we can in case that content process is compromised.
What about keeping the test camPerm == perms.ALLOW_ACTION here?

if (videoDevices.length && ((camPerm == perms.ALLOW_ACTION) || activeCamera)) {
  allowedDevices.push((activeCamera || videoDevices[0]).deviceIndex);
  Services.perms.add(uri, "MediaManagerVideo",
                     Services.perms.ALLOW_ACTION,
                     Services.perms.EXPIRE_SESSION);
}
revision 10: add the test for "if a stream is started, then the tab is detached to another window, check that we don't prompt when requesting the same device."

revision 11: add the test for "if there's an active stream in frame a, gUM() in the top level window should prompt."

All test cases mentioned in comment 59 should be covered now.
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review104188

::: browser/modules/webrtcUI.jsm:482
(Diff revisions 16 - 18)
> -        let allowedTmpDevices = [];
>  
>          for (let device of videoDevices) {
> -          if (webrtcUI.activePerms.has(aBrowser.outerWindowID)) {
> -            if (webrtcUI.activePerms.get(aBrowser.outerWindowID).
> -              has(aRequest.windowID + device.mediaSource + device.id)) {
> +          let set = webrtcUI.activePerms.get(aBrowser.outerWindowID);
> +          if (set && set.has(aRequest.windowID + device.mediaSource + device.id)) {
> +            if (!activeCamera) {

activeCamera = device;

::: browser/modules/webrtcUI.jsm:493
(Diff revisions 16 - 18)
>          }
>  
>          for (let device of audioDevices) {
> -          if (webrtcUI.activePerms.has(aBrowser.outerWindowID)) {
> -            if (webrtcUI.activePerms.get(aBrowser.outerWindowID).
> -              has(aRequest.windowID + device.mediaSource + device.id)) {
> +          let set = webrtcUI.activePerms.get(aBrowser.outerWindowID);
> +          if (set && set.has(aRequest.windowID + device.mediaSource + device.id)) {
> +            if (!activeMic) {

activeMic = device;

::: browser/modules/webrtcUI.jsm:501
(Diff revisions 16 - 18)
> +            activeMic.push(device.deviceIndex);
> +            break;
>            }
>          }
>  
> -        // We don't check that permissions are set to ALLOW_ACTION in this
> +        if ((!audioDevices.length || micPerm || activeMic) && (!videoDevices.length || camPerm || activeCamera)) {

If you really want to do '== perms.ALLOW_ACTION' check, they should be on this line.

This line is too long, wrapp it after '&&'.

::: browser/modules/webrtcUI.jsm:503
(Diff revisions 16 - 18)
> -        let isAlwaysShare = (!audioDevices.length || micPerm) && (!videoDevices.length || camPerm);
> -        let isTemporalShare = (!audioDevices.length || tmpMicPerm) && (!videoDevices.length || tmpCamPerm);
> -
> -        if ( isAlwaysShare || isTemporalShare ) {
>            let allowedDevices = [];
> -          if (isAlwaysShare) {
> +          if (videoDevices.length && ((camPerm == perms.ALLOW_ACTION) || activeCamera)) {

if (videoDevices.length) is enough, the other checks are duplicated with the check you already have 3 lines above.

::: browser/modules/webrtcUI.jsm:504
(Diff revisions 16 - 18)
> -        let isTemporalShare = (!audioDevices.length || tmpMicPerm) && (!videoDevices.length || tmpCamPerm);
> -
> -        if ( isAlwaysShare || isTemporalShare ) {
>            let allowedDevices = [];
> -          if (isAlwaysShare) {
> -            // All permissions we were about to request are already persistently set.
> +          if (videoDevices.length && ((camPerm == perms.ALLOW_ACTION) || activeCamera)) {
> +            allowedDevices.push(activeCamera || videoDevices[0].deviceIndex);

allowedDevices.push(activeCamera || videoDevices[0]).deviceIndex;

::: browser/modules/webrtcUI.jsm:711
(Diff revision 18)
> +            }
> +
> +            for (let device of videoDevices) {
> +              if (device.deviceIndex == videoDeviceIndex) {
> +                webrtcUI.activePerms.get(aBrowser.outerWindowID).
> +                  add(aRequest.windowID + device.mediaSource + device.id);

nit: the line break should be like this:
  webrtcUI.activePerms.get(...)
          .add(...);

Same comment applies to the audioDevices block.

::: dom/webidl/GetUserMediaRequest.webidl:10
(Diff revision 18)
>   *
>   * This is an internal IDL file
>   */
>  
> +// for gUM request start (getUserMedia:request) notification, rawID and mediaSource won't be set
> +// for gUM request stop (recording-device-stopped) notification, only windowID, rawID and mediaSource will be set

When looking at the code, it seems there's a case where only the windowID will be specified, and rawID & mediaSource will be empty strings. Can we document this here? When will it happen?

When reading your comment, I would think the recording-device-stopped notification is fired once when the streams obtained for the gUM request are stopped. But when looking at the code this notification is fired multiple time (once for each device).
Attachment #8810759 - Flags: review?(florian) → review-
(In reply to Munro Mengjue Chiang [:mchiang] from comment #86)
> (In reply to Florian Quèze [:florian] [:flo] from comment #85)
> > This case does not (or at the very least should not) happen since bug
> > 802326, because when there's one permission set to DENY, MediaManager will
> > deny the request immediately without even firing the getUserMedia:request
> > notification.
> Not sure if I understand this correctly, but according to the previous
> discussion about security sandbox, we are trying to avoid relying on
> MediaManager as much as we can in case that content process is compromised.
> What about keeping the test camPerm == perms.ALLOW_ACTION here?

That's a good point. Ideally this check should be done earlier in the 'prompt' function, before we call PopupNotifications.show.

See how the patch in bug 1206232 handles denying requests.
How are we looking? This turned out more complicated than I think we originally thought. Do we feel we're close? If not, we could perhaps look at cloning an existing stream, something that didn't occur to me at the outset. Of course there's no guarantee there won't be skeletons there as well, just wanted to mention it so we have all options to consider. If we're close then great!
Flags: needinfo?(mchiang)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #95)
> How are we looking? This turned out more complicated than I think we
> originally thought. Do we feel we're close?

Yes, comment 93 is only details, and the patch now looks good otherwise.
Comment on attachment 8822119 [details]
Bug 1270572 - write tests for the umprompted gUM request;

https://reviewboard.mozilla.org/r/101124/#review104242

This looks reasonable :-).

Next time you push this to try (which I assume will be for the next version of this test patch), please use a try syntax that only runs the tests you care about. Something like this:
try: -b do -p linux64,linux,macosx64,win32 -u mochitest-bc,mochitest-e10s-bc -t none --try-test-paths browser-chrome:browser/base/content/test/webrtc

I think we should verify with the tests that the temporary permissions get correctly removed, which means:
- after the page closes the stream, the next request prompts
- after the user clicks "stop sharing", the next request prompts
- after the user reloads the page, the next request prompts.

You can handle the last 2 cases by extending the "getUserMedia audio+video: stop sharing" and "getUserMedia audio+video: reloading the page removes all gUM UI" tests in browser_devices_get_user_media.js. The first one can go in browser_devices_get_user_media_unprompted_access.js.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_in_frame.js:52
(Diff revision 11)
> +
> +    yield expectObserverCalled("getUserMedia:response:deny");
> +    yield expectObserverCalled("recording-window-ended");
> +
> +    // If there's an active audio+camera stream in frame 1,
> +    // gUM(audio+camera) in the top level window causes a prompt.

We should add the opposite checks too:

- If there's an active stream at the top level window, a request in a frame should cause a prompt.
- we should check that umprompted access in frames does work: if there's a stream in frame1, requesting a stream for the same device in frame1 should work.
- moving promiseReloadFrame to head.js is nice, but I don't see this test using it. We should check that reloading a frame removes the temporary permission and that requesting a stream in that frame will prompt after the reload.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_in_frame.js:53
(Diff revision 11)
> +    yield expectObserverCalled("getUserMedia:response:deny");
> +    yield expectObserverCalled("recording-window-ended");
> +
> +    // If there's an active audio+camera stream in frame 1,
> +    // gUM(audio+camera) in the top level window causes a prompt.
> +

nit: remove this empty line after the comment.

::: browser/base/content/test/webrtc/head.js:373
(Diff revision 11)
>  
>    if (!aShouldKeepSharing)
>      yield* checkNotSharing();
>  }
>  
> -function promiseRequestDevice(aRequestAudio, aRequestVideo, aFrameId, aType) {
> +function promiseRequestDevice(aRequestAudio, aRequestVideo, aFrameId, aType, aBrowser = gBrowser.selectedBrowser) {

nit: wrap this long line:
function promiseRequestDevice(aRequestAudio, aRequestVideo, aFrameId, aType,
                              aBrowser = gBrowser.selectedBrowser) {
Attachment #8822119 - Flags: review?(florian) → review-
(In reply to Jan-Ivar Bruaroey [:jib] from comment #95)
> How are we looking? This turned out more complicated than I think we
> originally thought. Do we feel we're close? If not, we could perhaps look at
> cloning an existing stream, something that didn't occur to me at the outset.
> Of course there's no guarantee there won't be skeletons there as well, just
> wanted to mention it so we have all options to consider. If we're close then
> great!

I think we are getting close.
I will keep fixing the remaining nits and adding more tests.
Flags: needinfo?(mchiang)
Comment on attachment 8822119 [details]
Bug 1270572 - write tests for the umprompted gUM request;

https://reviewboard.mozilla.org/r/101124/#review104242

- after the page closes the stream, the next request prompts
^ This has been tested in "getUserMedia audio+camera" of browser_devices_get_user_media_unprompted_access.js

Will add 2 tests for "stop sharing" and reload.
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review104568

Looks good to me, thanks for the updated version.

You'll need an additional review from a dom peer for the dom/webidl/GetUserMediaRequest.webidl change.

Not sure if jib wants to have another look at the dom/media changes.
Attachment #8810759 - Flags: review?(florian) → review+
Comment on attachment 8822119 [details]
Bug 1270572 - write tests for the umprompted gUM request;

https://reviewboard.mozilla.org/r/101124/#review104574

I think we have good coverage now. The following comments are details. The main thing that remains to do here is to get this to be green on Linux. The problem may not be specific to linux, as it failed once on "Windows 7 VM opt" too.

You may need to disable a subset of browser_devices_get_user_media_unprompted_access_in_frame.js on Linux, like I had to do at http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js#190 for the screen sharing test.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_in_frame.js:81
(Diff revisions 11 - 13)
> +    // reload frame 1
> +    let promises = [promiseObserverCalled("recording-device-events"),
> +                    promiseObserverCalled("recording-device-events"),
> +                    promiseObserverCalled("recording-window-ended")];
> +    yield promiseReloadFrame("frame1");
> +    yield Promise.all(promises);

I think we should add here:
  yield checkNotSharing();

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_in_frame.js:98
(Diff revisions 11 - 13)
> +    });
>  
> +    yield expectObserverCalled("getUserMedia:response:deny");
> +    yield expectObserverCalled("recording-window-ended");
> +
> +    // create an active audio+camera stream at the top level window

This seems to be the beginning of a separate test, I would finish the previous one with a yield checkNotSharing();, and start a new test for the following part that is about checking the behavior in frames while a stream exists at the top level.

By "new test" I mean:
{
  desc: ...
  run: ...
not a new test file.

::: browser/base/content/test/webrtc/browser_devices_get_user_media.js:152
(Diff revision 13)
>  
>      // the stream is already closed, but this will do some cleanup anyway
>      yield closeStream(true);
> +
> +    // After stop sharing,
> +    // gUM(audio+camera) causes a prompt.

nit: this comment can fit on a single line.

::: browser/base/content/test/webrtc/browser_devices_get_user_media.js:193
(Diff revision 13)
>      yield checkSharingUI({video: true, audio: true});
>  
>      yield reloadAndAssertClosedStreams();
> +
> +    // After the reload,
> +    // gUM(audio+camera) causes a prompt.

same here

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access.js:5
(Diff revision 13)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +requestLongerTimeout(2);

Do we really need this?

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access.js:68
(Diff revision 13)
> +    });
> +
> +    yield expectObserverCalled("getUserMedia:response:deny");
> +
> +    // After closing all streams,
> +    // gUM(audio+camera) causes a prompt.

nit: this comment can fit on a single line.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_in_frame.js:15
(Diff revision 13)
> +    "by the user agent or the platform in the current context.";
> +
> +var gTests = [
> +
> +{
> +  desc: "getUserMedia audio+camera in frame 1",

This test timeouts on Linux on your try push. Before we can land, we'll need to figure out why.

Test logs will be more readable if most of the comments here are changed to info() calls.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_in_frame.js:37
(Diff revision 13)
> +    yield indicator;
> +    yield checkSharingUI({video: true, audio: true});
> +    yield expectNoObserverCalled();
> +
> +    // If there's an active audio+camera stream in frame 1,
> +    // gUM(audio+camera) in frame 2 causes a prompt;

make this:
info("gUM(audio+camera) in frame 2 should prompt");

You can omit the first line of the existing comment, as the description of the test will already be printed in the test log.
Attachment #8822119 - Flags: review?(florian) → review-
Added smaug for DOM review (one .webidl file).
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review105044

r+ to the .webidl
Attachment #8810759 - Flags: review?(bugs) → review+
revision 20 & 14: rebase
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review106150

::: browser/base/content/browser.js:4921
(Diff revisions 20 - 21)
>      let tab = gBrowser.getTabForBrowser(aBrowser);
>      if (tab && tab._sharingState) {
>        gBrowser.setBrowserSharing(aBrowser, {});
>        webrtcUI.forgetStreamsFromBrowser(aBrowser);
>      }
> +    webrtcUI.forgetActivePermissionsFromBrowser(aBrowser);

On Linux, sometimes (tab && tab._sharingState) == false during reloading, which lead to forgetStreamFromBrowser not being called. Then the next gUM will show no prompt and cause test timeout.

Not sure if this is a bug on Linux or not.

Since it doesn't hurt to always call forgetActivePermissionFromBrowser, I move it to here to assure we clear the realm permission every time we reload.
Comment on attachment 8822119 [details]
Bug 1270572 - write tests for the umprompted gUM request;

https://reviewboard.mozilla.org/r/101124/#review106154

::: browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js:103
(Diff revisions 14 - 15)
>  
>      yield indicator;
>      yield checkSharingUI({video: true, audio: true});
>  
>      info("reloading the frame");
> -    promise = promiseObserverCalled("recording-device-events");
> +    promise = promiseObserverCalled("recording-device-stopped");

Recording-device-stopped come later than recording-device-events. It causes a timing issue on Linux that when this case ends, recording-device-stopped hasn't been fired and realm permission hasn't been cleared. This will cause next gUM show no prompt and cause a timeout.

So hear we use a promise to wait for recording-device-stopped. Make sure the realm permission is cleared and then finish the test.
Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

https://reviewboard.mozilla.org/r/93086/#review106358

::: browser/base/content/browser.js:4921
(Diff revisions 20 - 21)
>      let tab = gBrowser.getTabForBrowser(aBrowser);
>      if (tab && tab._sharingState) {
>        gBrowser.setBrowserSharing(aBrowser, {});
>        webrtcUI.forgetStreamsFromBrowser(aBrowser);
>      }
> +    webrtcUI.forgetActivePermissionsFromBrowser(aBrowser);

Do you know in which case tab._sharingState isn't set? Is it just that we are not waiting for the correct events before continuing the test? If it's the indicator that isn't displayed when it should be, it's scary! Could be related to bug 1325261.

"it doesn't hurt to always call" isn't exactly true. The current code was written the way it is so that webrtcUI wouldn't be imported unless the user has been on a page using webrtc at least once. But given that nsBrowserGlue.js uses webrtcUI as early as _finalUIStartup, it probably doesn't matter much.

Instead of calling forgetActivePermissionsFromBrowser from here, you could just call forgetStreamsFromBrowser unconditonally (ie. move that line outside of the if block)
Comment on attachment 8822119 [details]
Bug 1270572 - write tests for the umprompted gUM request;

https://reviewboard.mozilla.org/r/101124/#review106362

Thanks for debugging the test failures!
Attachment #8822119 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/bb87f60ef239
https://hg.mozilla.org/mozilla-central/rev/8516638f3e90
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1332608
Jib, Munro -- I've set qawanted because I think manual testing on this makes sense before this goes to Beta.
Flags: needinfo?(mchiang)
Flags: needinfo?(jib)
Keywords: qawanted
(In reply to Maire Reavy [:mreavy] Please needinfo me from comment #124)
> Jib, Munro -- I've set qawanted because I think manual testing on this makes
> sense before this goes to Beta.

Hi Maire, 
the QA teams are now using the qe-verify+ flag for bugs that need verification.
Flags: qe-verify+
Keywords: qawanted
Yes, manual testing is needed. Also we added quite a few mochitests to cover different scenarios.
Flags: needinfo?(mchiang)
Depends on: 1335794
Agree. Even with automated testing, some manual testing would be good to make sure we didn't add any ways to circumvent the permission prompt, and also check that real devices don't respond oddly in these new situations.
Flags: needinfo?(jib)
I am willing to do manual testing for this. Could someone please tell me exactly what to test and I will do it. Thank you.
Thanks for the help.
Please notice that currently you may encounter video freeze issue due to bug 1321714.

Here are the test steps:

1. Browse the website https://jsfiddle.net/ddwrjdmv/
2. Press the button gum_av; grant the permission
3. Press the button gum_v; check if there is no prompt
4. Press the button stop_av & stop_v & gum_v; check if it causes a prompt; grant the permission
5. Press the button Run; press the button gum_v; check if it causes a prompt; grant the permission
6. Press the button gum_a; check if it causes a prompt; grant the permission
7. Open a new tab and browse to the same website
8. Press the button gum_av; check if it causes a prompt; grant the permission
9. Tear off the 2nd tab to a new Window; press the button gum_v; check if there is no prompt
Depends on: 1340174
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

I have reproduced the issue using Firefox Nightly 53.0a1 (id: 20170118030214). 
This is VERIFIED FIXED in Firefox 53.0b1 (id: 20170307064827) on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04 LTS x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1374640
Depends on: 1443294
You need to log in before you can comment on or make changes to this bug.