1698513 - Revoking cam+mic permission takes three times the clicks now in iframes (regression)

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P1)

Description

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

Attachment: image.png

STRs:

1. Open https://jan-ivar.github.io/dummy/iframe_gum.html and Allow cam + mic.
2. Click red camera icon in URL bar to open the permissions dropdown, and click the first ✖ in

Open Pop-up Windows                   [Allow][▼]
🔴 Use the Camera          Allowed Temporarily ✖
🔴 Use the Microphone      Allowed Temporarily ✖

Expected results:

Open Pop-up Windows                   [Allow][▼]

Actual results (image):

⚫ Use the Camera          Allowed Temporarily ✖
⚫ Use the Microphone      Allowed Temporarily ✖
Open Pop-up Windows                   [Allow][▼]

The result is I have to click click ✖ 3 times instead of 1 to clear cam+mic permissions. Also the ✖'s move around (get reordered) making them more difficult to hit, and making it less obvious that they weren't cleared the first time like they are in release.

I see basically three bugs here

1. A single click should clear grace and non-grace permissions
2. The items should not reorder on the user
3. A single click should always clear both camera and mic together, because it's a safer, as users may forget to clear both otherwise (this was by design).

Comment 1

[Paul Zühlcke [:pbz]]

clearPermissionsAndStopSharing correctly clears the permissions. However, once it sends the webrtc:StopSharing message here: https://searchfox.org/mozilla-central/rev/aa9a7136835deb0eeba00c62bb50a4a0e2cdea2d/browser/modules/webrtcUI.jsm#654 the WebRTCParent adds the temporary grant permissions again.

I've discussed two possible solutions with :jib:

1. Pass a flag into webrtc:StopSharing which signals the track-stop handler in WebRTCParent to not add new grace periods (because it's an explicit clear/stop, for example revoking via the permission panel).
2. Refactor the permission model for the WebRTC one-off grants. That is, remove webrtcUI.activePerms and fully store permissions in SitePermissions. Upon permissions grant (the user accepts the permission dialog), we store a temporary, but non-expiring double-keyed permission (e.g. camera^myCameraId). It will have the lifetime of the browser. Once the device stops deactivateDevicePerm checks if such a permission still exists and updates it to expire after a grace period (effectively it overwrites the permission with a new permission with expiry timer).
   This model allows us to clear permissions and stop active devices without starting new grace periods.

Comment 2

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

I think 2 is the best solution, even short-term. To sell it, I wouldn't even gate this on removing webrtcUI.activePerms. Merely:

1. Add an ALLOW, SCOPE_TEMPORARY in activateDevicePerm without grace period, using the same key we use for grace periods
2. Modify deactivateDevicePerm to change it to an expiring one. If not found, then no grace period.

Adding a temporary grant permission at the point of grant, and then converting it to a grace period on stop, accomplishes two things:

• Fix this bug, by giving clearPermissionsAndStopSharing something to clear, leaving nothing to be converted to grace periods
• Perhaps map more closely to the user-observable model

Comment 3

[Johann Hofmann [:johannh]]

Yeah, that seems good to me!

Comment 4

[Paul Zühlcke [:pbz]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #2)

I think 2 is the best solution, even short-term. To sell it, I wouldn't even gate this on removing webrtcUI.activePerms. Merely:

1. Add an ALLOW, SCOPE_TEMPORARY in activateDevicePerm without grace period, using the same key we use for grace periods
2. Modify deactivateDevicePerm to change it to an expiring one. If not found, then no grace period.

Adding a temporary grant permission at the point of grant, and then converting it to a grace period on stop, accomplishes two things:

• Fix this bug, by giving clearPermissionsAndStopSharing something to clear, leaving nothing to be converted to grace periods
• Perhaps map more closely to the user-observable model

If we don't want to change the current permission behavior with iframes we need to key by device type, device ID and outer window ID, as activePerms does. I've refactored webRTCParent so it uses SitePermissions instead of the activePerms map, and sets permissions like camera^myCameraID^12 where 12 is the outer window ID. While I'm not a big fan of adding yet another key, SitePermissions and the permission panel can handle it already.
This runs mostly fine on tests. However, I've discovered that webrt:StopRecording isn't called for the error case. Specifically in this test: https://searchfox.org/mozilla-central/rev/1a47a74bd5ba89f2474aa27c40bd478e853f3276/browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js#122,145
This means deactivateDevicePerm isn't called for the error case and we end up with a lingering permission.

Comment 5

[Paul Zühlcke [:pbz]]

I've found another, much simpler way:
Before sending the webrtc:StopSharing messages here: https://searchfox.org/mozilla-central/rev/b7b156e53643f0237f3e98a76e5fc7fa9e3b4e71/browser/modules/webrtcUI.jsm#654 we can remove all outer window ids, matching the browser of the given tab, from webrtcUI.activePerms. Prior to adding the grace period we check webrtcUI.activePerms and if it doesn't have an entry for the given outer window id we skip adding the grace period.
webrtcUI.activePerms also contains window IDs of subframes, so we need to iterate over all BrowsingContexts which are children of the browser.

Comment 6

[Paul Zühlcke [:pbz]]

Attachment: Bug 1698513 - Do not start WebRTC permission grace-period on permission revoke. r=johannh!

Comment 7

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

(In reply to Paul Zühlcke [:pbz] from comment #4)

If we don't want to change the current permission behavior with iframes we need to key by device type, device ID and outer window ID, as activePerms does.

I think changing the current permission behavior with iframes would be an improvement actually. It's too restrictive today. Example:

1. Open https://jan-ivar.github.io/dummy/iframe_iframe_gum_cross.html

This will prompt on pageload and again when pressing the Camera button in the iframe. In both cases the user is trusting jan-ivar.github.io, so the second prompt is redundant from a regular user's perspective, as it is indistinguishable from the first.

So I think it would ok to not track the outer window ID in sitePermissions, and instead follow what we already do for grace periods. Permission should always be tied to the top-level origin here.

As to activePerms, I'd leave things alone there (including the outer window ID) because that array is used to track live tracks per document, to satisfy a very specific spec requirement.

Comment 8

[Paul Zühlcke [:pbz]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #7)

(In reply to Paul Zühlcke [:pbz] from comment #4)

If we don't want to change the current permission behavior with iframes we need to key by device type, device ID and outer window ID, as activePerms does.

I think changing the current permission behavior with iframes would be an improvement actually. It's too restrictive today. Example:

1. Open https://jan-ivar.github.io/dummy/iframe_iframe_gum_cross.html

This will prompt on pageload and again when pressing the Camera button in the iframe. In both cases the user is trusting jan-ivar.github.io, so the second prompt is redundant from a regular user's perspective, as it is indistinguishable from the first.

So I think it would ok to not track the outer window ID in sitePermissions, and instead follow what we already do for grace periods. Permission should always be tied to the top-level origin here.

As to activePerms, I'd leave things alone there (including the outer window ID) because that array is used to track live tracks per document, to satisfy a very specific spec requirement.

I'm open to changing this behavior in the future. However, to quickly address this bug I'd rather go for the small fix I submitted.

For future reference: The bug only affects revoking active devices from iframes, because webrtcUI#forgetActivePermissionsFromBrowser only clears the entry for the top level window, while there may be entries for frames as well.
https://searchfox.org/mozilla-central/rev/f07a609a76136ef779c65185165ff5ac513cc172/browser/modules/webrtcUI.jsm#686

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24ff7bb331e0
Do not start WebRTC permission grace-period on permission revoke. r=johannh

Comment 10

[Alexandru Michis [:malexandru]]

Backed out changeset 24ff7bb331e0 (bug 1698513) for causing bc failures in browser_devices_get_user_media_in_frame.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/79541989faf46ee8c4bdaaba834c3f15651ac2db
Push with failures, failure log.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7e19e5b81de
Do not start WebRTC permission grace-period on permission revoke. r=johannh

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 13

[Paul Zühlcke [:pbz]]

Comment on attachment 9209845 [details]
Bug 1698513 - Do not start WebRTC permission grace-period on permission revoke. r=johannh!

Beta/Release Uplift Approval Request

• User impact if declined: Users have to clear permissions / stop active WebRTC devices multiple times before device permissions are fully cleared. This makes for a bad UX and could be a privacy issue, since users expect permissions to be revoked on the first click. The bug only affects devices shared with iframes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See STR in comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Medium risk since the patch changes how we revoke / stop WebRTC device permissions. However, we have good WebRTC test coverage and the code change is relatively small.
• String changes made/needed:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9209845 [details]
Bug 1698513 - Do not start WebRTC permission grace-period on permission revoke. r=johannh!

Approved for 88.0b3.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/efa2ec4421b5

Comment 16

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on affected Firefox Beta 88.0b2 and can confirm as verified-fixed on latest Firefox Nightly 89.0a1 (2021-03-25) (64-bit) and Firefox Beta 88.0b3 on Windows 10 and MacOS 10.15. The grace period is not applied anymore after revoking permissions.