Multiple calls to getUsermedia return the same window / screen

RESOLVED FIXED in Firefox 55

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
18
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: Seppe Lenders, Assigned: mchiang)

Tracking

({regression})

53 Branch
mozilla56
regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54+ wontfix, firefox55+ fixed, firefox56+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
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

4 months ago
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.
Blocks: 1270572
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
Component: Untriaged → WebRTC: Audio/Video
Ever confirmed: true
Flags: needinfo?(mchiang)
Keywords: regression
Product: Firefox → Core
(Assignee)

Updated

4 months ago
Flags: needinfo?(mchiang)
(Assignee)

Comment 2

4 months ago
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?
Flags: needinfo?(jib)
Track 54+/55+/56+ as WebRTC regression.
tracking-firefox54: ? → +
tracking-firefox55: ? → +
tracking-firefox56: ? → +

Updated

4 months ago
Rank: 25
Priority: -- → P2

Updated

4 months ago
status-firefox54: affected → wontfix
status-firefox55: affected → fix-optional
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
Rank: 25 → 18
Flags: needinfo?(jib)
Priority: P2 → P1
(Assignee)

Comment 5

4 months ago
"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?
(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.
(Assignee)

Comment 7

4 months ago
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.
(Assignee)

Comment 8

4 months ago
As the discussion, I will do item 1 only.
(Assignee)

Comment 9

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7235ffc48fa14d1127823a0ba5fde37eea3e6b8
Comment hidden (mozreview-request)

Comment 11

4 months ago
mozreview-review
Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

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

Lgtm.
Attachment #8882585 - Flags: review?(jib) → review+

Comment 12

4 months ago
mozreview-review
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?
Attachment #8882585 - Flags: review?(florian) → review-
(Assignee)

Comment 13

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1327c06f8eaed7122233f8f0295d2349ad87465b
Comment hidden (mozreview-request)

Comment 15

4 months ago
mozreview-review
Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

https://reviewboard.mozilla.org/r/153682/#review159856
Attachment #8882585 - Flags: review?(florian) → review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed
Assignee: nobody → mchiang

Comment 16

4 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ba8db0fbc006
always prompt for screen sharing. r=florian,jib
Keywords: checkin-needed

Comment 17

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba8db0fbc006
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 18

3 months ago
Hi Munro, Florian, this seems like a simple and low risk fix. Should we uplift to Beta55? If yes, please nominate a patch. Thanks!
Flags: needinfo?(mchiang)
Flags: needinfo?(florian)
(Assignee)

Comment 19

3 months ago
As long as Florian has no concern, I will nominate a patch.
Flags: needinfo?(mchiang)
(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 :-).
Flags: needinfo?(florian)
(Assignee)

Comment 21

3 months ago
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
Attachment #8882585 - Flags: approval-mozilla-beta?
Comment on attachment 8882585 [details]
Bug 1374640 - always prompt for screen sharing.

fix a webrtc regression, beta55+

should be in 55.0b9
Attachment #8882585 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2f1f99506678
status-firefox55: fix-optional → fixed
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.