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

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 1

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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?

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 3

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review commit: https://reviewboard.mozilla.org/r/93086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93086/

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 6

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 10

[Gian-Carlo Pascutto [:gcp]]

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

Comment 11

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 *'

Comment 12

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/1-2/

Comment 13

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

I will submit revision 3 for Android Firefox later.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

See comment 14.

Comment 16

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/2-3/

Comment 17

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

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

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 20

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 23

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug-1270572-grant-internal-realm-permission.patch

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.

Comment 24

[Gian-Carlo Pascutto [:gcp]]

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

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/3-4/

Comment 26

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 27

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/4-5/

Comment 28

[Florian Quèze [:florian]]

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.

Comment 29

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/5-6/

Comment 30

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 31

[Florian Quèze [:florian]]

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

Comment 32

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(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

Comment 33

[Florian Quèze [:florian]]

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

Comment 34

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 35

[Florian Quèze [:florian]]

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

Comment 36

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 37

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

> revoke the permission in webrtcUI without going through content manager.
typo, content process mediamanager

Comment 38

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 39

[Florian Quèze [:florian]]

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

Comment 40

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 42

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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.

Comment 43

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/6-7/

Comment 44

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/7-8/

Comment 45

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

The latest draft will cause timeout while running https://searchfox.org/mozilla-central/source/browser/base/content/test/webrtc/browser_devices_get_user_media.js#185

I am debugging it now.

Comment 46

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/8-9/

Comment 47

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

revision 9 fixes the timeout.

Comment 48

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1270572 - write tests for the umprompted gUM request;

Review commit: https://reviewboard.mozilla.org/r/98768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98768/

Comment 49

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js also breaks. Checking.

Comment 50

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/9-10/

Comment 51

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8818829 [details]
Bug 1270572 - write tests for the umprompted gUM request;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98768/diff/1-2/

Comment 52

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

revision 10 fixes browser_devices_get_user_media_in_frame.js break.

Comment 53

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/10-11/

Comment 54

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8818829 [details]
Bug 1270572 - write tests for the umprompted gUM request;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98768/diff/2-3/

Comment 55

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

revison 11: rebase

Comment 56

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/11-12/

Comment 57

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8818829 [details]
Bug 1270572 - write tests for the umprompted gUM request;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98768/diff/3-4/

Comment 58

[Florian Quèze [:florian]]

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.

Comment 59

[Florian Quèze [:florian]]

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.

Comment 60

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13388b3b1a4d

Comment 61

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 62

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/

Comment 63

[Florian Quèze [:florian]]

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

Comment 64

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/12-13/

Comment 65

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1270572 - write tests for the umprompted gUM request;

Review commit: https://reviewboard.mozilla.org/r/101124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101124/

Comment 66

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/1-2/

Comment 67

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/13-14/

Comment 68

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/2-3/

Comment 69

[Florian Quèze [:florian]]

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.

Comment 70

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 71

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 72

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/14-15/

Comment 73

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/

Comment 74

[Florian Quèze [:florian]]

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

Comment 75

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/4-5/

Comment 76

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

revision 5: backout to revision 3.

Comment 77

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/15-16/

Comment 78

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/5-6/

Comment 79

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

revision 16 & 6: rebase

Comment 80

[Florian Quèze [:florian]]

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.

Comment 81

[Florian Quèze [:florian]]

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 82

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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);
            }
          }

Comment 83

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/16-17/

Comment 84

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/6-7/

Comment 85

[Florian Quèze [:florian]]

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

Comment 86

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(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);
}

Comment 87

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/17-18/

Comment 88

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/7-8/

Comment 89

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/8-9/

Comment 90

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/9-10/

Comment 91

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/10-11/

Comment 92

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 93

[Florian Quèze [:florian]]

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

Comment 94

[Florian Quèze [:florian]]

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

Comment 95

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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!

Comment 96

[Florian Quèze [:florian]]

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

[Florian Quèze [:florian]]

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

Comment 98

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 99

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 100

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/18-19/

Comment 101

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/11-12/

Comment 102

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/12-13/

Comment 103

[Florian Quèze [:florian]]

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.

Comment 104

[Florian Quèze [:florian]]

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.

Comment 105

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Added smaug for DOM review (one .webidl file).

Comment 106

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

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

Comment 107

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/19-20/

Comment 108

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/13-14/

Comment 109

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

revision 20 & 14: rebase

Comment 110

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/20-21/

Comment 111

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/14-15/

Comment 112

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 113

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 114

[Florian Quèze [:florian]]

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 115

[Florian Quèze [:florian]]

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!

Comment 116

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/21-22/

Comment 117

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/15-16/

Comment 118

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

We need a rebase

Comment 119

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8810759 [details]
Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93086/diff/22-23/

Comment 120

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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/16-17/

Comment 121

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ba4a6839a65894effd035b1413c34549ed6d47e

Comment 122

[Ryan VanderMeulen [:RyanVM]]

Tomcat autolanded this, but pulsebot bug marking is currently broken.

https://hg.mozilla.org/integration/autoland/rev/bb87f60ef239
https://hg.mozilla.org/integration/autoland/rev/8516638f3e90

Comment 123

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bb87f60ef239
https://hg.mozilla.org/mozilla-central/rev/8516638f3e90

Comment 124

[Maire Reavy [:mreavy]]

Jib, Munro -- I've set qawanted because I think manual testing on this makes sense before this goes to Beta.

Comment 125

[Brindusa Tot, Desktop QA]

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

Comment 126

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Yes, manual testing is needed. Also we added quite a few mochitests to cover different scenarios.

Comment 127

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 128

[Vuyisile Ndlovu [:terrameijar]]

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.

Comment 129

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 130

[Alexandru Simonca [:asimonca], Release Desktop QA]

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.