1374640 - Multiple calls to getUsermedia return the same window / screen

Status: RESOLVED FIXED

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

Description

[Seppe Lenders]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

I have created a minimal demo that illustrates my problem: https://jsfiddle.net/hpz7xwLj/2/. You can simply run the demo with FF >= 52.


Actual results:

The goal of the demo is to start 2 screensharing streams. In FF 52 both calls to getUserMedia trigger showing the popup where you can choose a screen to share. You can select a different screen each time, and the webpage will show both streams simultaneously.
Starting with FF 53 (and tested up to nightly), the popup is only shown once. The second getUserMedia call is resolved immediately with the stream that was chosen during the first getUserMedia call. The webpage shows the same stream twice.

Is this intentional? If yes, what is the rationale behind this? Is there a different way to replicate the FF<=52 behaviour?

Comment 1

[Loic]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6ff304ff85180a803ed57c560f8ff1b72c7af657&tochange=8516638f3e905ba0542fe6432e2946c365460cfa

Munro Mengjue Chiang — Bug 1270572 - allow un-prompted gUM access if the page has a live track connected to the same device; r=florian,gcp,smaug


Florian added to CC too, as you reviewed the patch.

Comment 2

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

This is intentional. As mentioned in bug 1270572, 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. 

[1] https://github.com/w3c/mediacapture-main/commit/bbdee2433051107e597a4fbd277b6d619113ac54

jib, this spec change will make sharing two different screens & cameras impossible (unless using two different constraints to access them). Was this impact considered when the spec change was discussed?

Comment 3

[Gerry Chang [:gchang]]

Track 54+/55+/56+ as WebRTC regression.

Comment 4

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

I don't believe this impact was considered. In hindsight I think we should make an exception here and always prompt for screen sharing. I'm up-ing priority for this regression in case this is trivial and upliftable.

I don't think we need to change the spec for this. The mediacapture spec only talks about "permission" here [1], not "selection":

  "6. For the origin identified by originIdentifier, request permission for use of the devices, while considering all devices attached to a live MediaStreamTrack in the current browsing context to have permission status "granted", resulting in a set of provided media."

About "selection" it goes on to say:

  "The decision of which devices to choose from the finalSet is completely up to the User Agent and may be determined by asking the user."

The screensharing spec [2] (not the one we follow yet, that's bug 1321221, but still the one that counts I think) implies that selection is mandatory:

  "The getDisplayMedia function does not permit the use of constraints for selection of a source as described in the getUserMedia() algorithm."

We pretty much honor this inasmuch as we don't expose deviceIds or allow selection by them.

So I don't see anything that says we MUST return the same screen. For cameras and mics that just happens to be the net result as long as constraints (or the lack of them) end up selecting the same device as before.

[1] https://rawgit.com/w3c/mediacapture-main/master/getusermedia.html#dom-mediadevices-getusermedia()
[2] https://w3c.github.io/mediacapture-screen-share/#constraints

Comment 5

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

"while considering all devices attached to a live MediaStreamTrack in the current browsing context to have permission status "granted", resulting in a set of provided media."
I suppose that means only the devices attached to a live MediaStreamTrack are treated as "granted"?
If we allow users selecting a source which is not "granted" in the 2nd gUM request, what's the point to grant the permission?

Comment 6

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

(In reply to Munro Mengjue Chiang [:mchiang] from comment #5)
> I suppose that means only the devices attached to a live MediaStreamTrack are treated as "granted"?

Yes. We appear to do this correctly already. E.g. with two cameras, https://jsfiddle.net/jib1/t6hxzqdd/ asks for permission for each one. But if I s/cams[1]/cams[0]/, I get a single prompt instead.

> If we allow users selecting a source which is not "granted" in the 2nd gUM request, what's the point to grant the permission?

My interpretation is the spec ONLY disallows permission prompts for the SAME device. E.g. this MUST NOT prompt twice:

    await navigator.mediaDevices.getUserMedia({video: {deviceId: {exact: id}});
    await navigator.mediaDevices.getUserMedia({video: {deviceId: {exact: id}});

The spec's interest here is reducing friction and maintain internal consistency by mandating away unnecessary permission prompts in the comparable cases of re-obtaining the same source using getUserMedia() vs stream.clone(). It also dovetails nicely with the language of when temporary permissions go away (when the light goes out, access cannot come back).

However, if you use:

    await navigator.mediaDevices.getUserMedia({video: true});

then you're at the mercy of the browser to choose a device for you which "may be determined by asking the user." This is the case that applies to screensharing, since screensharing does not support constraints.

Note the spec is written to be browser agnostic, and Firefox is the only browser that offers selection inside the cam/mic permission prompt. For screensharing however, both Chrome and Firefox implement source pickers with implicit permission.

Comment 7

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

So here is my plan:
Two steps:
1) For screen sharing, always prompt.
2) For camera/microphone sharing, only suppress the prompt for the SAME device.

Comment 8

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

As the discussion, I will do item 1 only.

Comment 9

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

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

Comment 10

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

Attachment: Bug 1374640 - always prompt for screen sharing.

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

Comment 11

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

Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

https://reviewboard.mozilla.org/r/153682/#review158984

Lgtm.

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

https://reviewboard.mozilla.org/r/153682/#review159306

::: browser/modules/webrtcUI.jsm:483
(Diff revision 1)
>  
>          let activeCamera;
>          let activeMic;
>  
> +        // Always prompt for screen sharing
> +        if (!(requestTypes.includes("Screen") ||

Could this just use the sharingScreen variable instead of having a full list of screen sharing types? Or maybe this block wants to be an else for the if block 7 lines above?

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1327c06f8eaed7122233f8f0295d2349ad87465b

Comment 14

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

Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

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

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

https://reviewboard.mozilla.org/r/153682/#review159856

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba8db0fbc006
always prompt for screen sharing. r=florian,jib

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba8db0fbc006

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Munro, Florian, this seems like a simple and low risk fix. Should we uplift to Beta55? If yes, please nominate a patch. Thanks!

Comment 19

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

As long as Florian has no concern, I will nominate a patch.

Comment 20

[Florian Quèze [:florian]]

(In reply to Munro Mengjue Chiang [:mchiang] from comment #19)
> As long as Florian has no concern, I will nominate a patch.

Uplifting this patch seems reasonable to me :-).

Comment 21

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

Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1270572
[User impact if declined]: Multiple calls to getUsermedia return the same window / screen
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a small change which always shows a prompt for screen sharing.
[String changes made/needed]: No

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

fix a webrtc regression, beta55+

should be in 55.0b9

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f1f99506678

Comment 24

[Andrei Vaida [:avaida]]

(In reply to Munro Mengjue Chiang [:mchiang] from comment #21)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Munro Mengjue Chiang's assessment on manual testing needs and the fact that this fix has automated coverage.