Closed Bug 1693677 Opened 3 years ago Closed 3 years ago

Grant a temporary grace period for re-requesting microphone or camera devices after the track stops

Categories

(Firefox :: Site Permissions, enhancement, P1)

Product:
Firefox
Component:
Site Permissions
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox88 --- verified

People

(Reporter: mconley, Assigned: jib)

References

(Depends on 1 open bug, Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [proton-door-hangers])

Attachments

(4 files, 1 obsolete file)

Bug 1693677 - Add a 50s grace period for re-requesting a camera or microphone device in a tab.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1693677 - Fix bug where nsIMediaDevice active devices list would omit camera from joint gUM requests.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1693677 - Test permission grace periods.
48 bytes, text/x-phabricator-request
Details | Review
testrun.rtf
42.23 KB, text/rtf
Details

I still need to hammer out the exact rules here, but so far, I believe it goes like this:

  1. Suppose the user has granted temporary permission for a site to use one or more camera and/or microphone tracks.
  2. If those microphone or camera tracks ends, either by page navigation, or by the site stopping the track via track.stop, we should create a "temporary grant" for those same devices for 50 seconds*. That way, if a site re-requests those same devices, and the temporary grant exists, we share them without re-requesting permission.
  3. If the user has set a permanent or temporary block for the device type for the top-level frame, then the temporary grants should not apply.
  4. This needs to build off of the work in bug 1693621 for the temporary grants, so this is blocked on that bug.
  5. Ideally, the user should be able to see in the permissions panel that the grace periods exist, in the event that they want to revoke them. I don't believe special styling is required for them - the UI that we currently use for temporary camera and microphone grants is sufficient.

Once bug 1693621 is done, there are three big parts for this work:

  1. Finding the best place to call into SitePermissions to register the grace periods
  2. Checking for those grace periods on device access requests
  3. Writing and updating our automated tests, since this is subtle but significant behaviour change that our tests aren't currently prepared for.
  • This value should be stored in a pref so we can tune it, but let's start with 50 seconds for now.
Whiteboard: [proton-door-hangers]
Severity: -- → N/A
Priority: -- → P2
Assignee: nobody → jib
Status: NEW → ASSIGNED
Priority: P2 → P1
Attachment #9204707 - Attachment is obsolete: true
Attachment #9204707 - Attachment is obsolete: false
Attachment #9204707 - Attachment description: Bug 1693677 - [WIP] Add a 50s grace period for re-requesting a camera or microphone device in a tab. → Bug 1693677 - Add a 50s grace period for re-requesting a camera or microphone device in a tab.
Attachment #9205602 - Attachment is obsolete: true

This seems to work well now, and is ready for testing.

Depends on: 1695615

Fix a bug where camera was being omitted from accounting in cases where
camera and mic where obtained jointly with a single getUserMedia call.

The permission grace period implementation relies on more accurate
accounting of when individual active devices are deactivated, and this bug
caused an existing test to fail and permission to slip through by failing
to account for cameras needing clearing from webrtcUI.activePerms during a
session.

Also remove a few redundant lines from said test (behavior neutral).

Depends on D106043

Rebased on top of bug 1695615.

Attached file testrun.rtf —

Hi Paul, I have the tests succeeding at testing what I want them to test so far, but the second test is failing over side-effects I don't understand, likely from me using BrowserTestUtils.withNewTab wrong.

I based this off similar tests in SitePermissions, so I was hoping you could help me out with some pointers. Do you know what's going on here?

Also, do you know any way to get around this annoying lint error?

★ ~/moz/mozilla-central $ hg amend
/Users/Jan/moz/mozilla-central/browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js
  12:43  error  listen for events instead of setTimeout() with arbitrary delay  mozilla/no-arbitrary-setTimeout (eslint)

✖ 1 problem (1 error, 0 warnings, 15 fixed)

I need to use setTimeout to test the grace period (or is there a better way?)

Flags: needinfo?(pbz)

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

Also, do you know any way to get around this annoying lint error?

/* eslint-disable mozilla/no-arbitrary-setTimeout */
e.g. https://searchfox.org/mozilla-central/rev/ca910762568921c0faa34838d6a4efac2471dff1/toolkit/components/glean/xpcshell/test_Glean.js#65

If you need setTimeout (there is probably no alternative here) you can use requestFlakyTimeout: https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/testing/mochitest/tests/SimpleTest/SimpleTest.js#924
If you still get linting errors when using that, you can additionally add the eslint exception :karlt mentioned.

I don't think you actually need to open another tab in the individual test cases, since the WebRTC test wrapper already does that for you. It should pass you the browser of the test tab, for example run: async function checkAudioVideoGracePastStop(browser) {.

Concerning the test failure: I see an unexpected permission prompt in checkAudioVideoGracePastStop. Is that the issue you're describing?
If I add log statements to SitePermissions.getForPrincipal and SitePermissions.setForPrincipal I can see that there is a temporary permission set for microphone^someId, but camera^someOtherId is not set. I assume the lack of the camera permission is triggering the prompt?

Note that I've tested this on an artifact build.

 0:04.09 INFO closing the stream
 0:04.10 GECKO(23660) console.debug: "setForPrincipal" ({principal:"https://example.com", permissionID:"microphone^B7CBD7C1-53EF-42F9-8353-73F61C70C092", state:1, scope:"{SitePermissions.SCOPE_TEMPORARY}", browser:"https://example.com/browser/browser/base/content/test/webrtc/get_user_media.html", browserURI:"https://example.com/browser/browser/base/content/test/webrtc/get_user_media.html"})
 0:04.10 PASS WebRTC indicator hidden - 
 0:04.10 PASS camera global indicator as expected - 
 0:04.10 PASS microphone global indicator as expected - 
 0:04.10 PASS screen global indicator as expected - 
 0:04.10 PASS WebRTC menu should be hidden - 
 0:04.10 PASS popup WebRTC indicator hidden - 
 0:04.10 INFO Reload through the page
 0:04.12 GECKO(23660) TEST DEVICES: Using media devices:
 0:04.13 GECKO(23660) audio:
 0:04.13 GECKO(23660) video:
 0:04.13 INFO After page reload, gUM(camera+mic) returns a stream without prompting within grace period.
 0:04.13 INFO requesting devices
 0:04.22 GECKO(23660) console.log: "Failed: Observer topic getUserMedia:request not expected in content process"
 0:04.22 GECKO(23660) console.debug: "getForPrincipal" ({principal:"https://example.com", permissionID:"microphone", browser:"https://example.com/browser/browser/base/content/test/webrtc/get_user_media.html"})
 0:04.22 GECKO(23660) console.debug: "result" ({state:0, scope:"{SitePermissions.SCOPE_PERSISTENT}"})
 0:04.22 GECKO(23660) console.debug: "getForPrincipal" ({principal:"https://example.com", permissionID:"camera", browser:"https://example.com/browser/browser/base/content/test/webrtc/get_user_media.html"})
 0:04.22 GECKO(23660) console.debug: "result" ({state:0, scope:"{SitePermissions.SCOPE_PERSISTENT}"})
 0:04.22 GECKO(23660) console.debug: "getForPrincipal" ({principal:"https://example.com", permissionID:"microphone", browser:(void 0)})
 0:04.22 GECKO(23660) console.debug: "result" ({state:0, scope:"{SitePermissions.SCOPE_PERSISTENT}"})
 0:04.22 GECKO(23660) console.debug: "getForPrincipal" ({principal:"https://example.com", permissionID:"camera", browser:(void 0)})
 0:04.22 GECKO(23660) console.debug: "result" ({state:0, scope:"{SitePermissions.SCOPE_PERSISTENT}"})
 0:04.22 GECKO(23660) console.debug: "getForPrincipal" ({principal:"https://example.com", permissionID:"camera^1041FCBD-3F12-4F7B-9E9B-1EC556DD5676", browser:"https://example.com/browser/browser/base/content/test/webrtc/get_user_media.html"})
 0:04.22 GECKO(23660) console.debug: "result" ({state:0, scope:"{SitePermissions.SCOPE_PERSISTENT}"})
 0:04.22 GECKO(23660) console.debug: "getForPrincipal" ({principal:"https://example.com", permissionID:"microphone^B7CBD7C1-53EF-42F9-8353-73F61C70C092", browser:"https://example.com/browser/browser/base/content/test/webrtc/get_user_media.html"})
 0:04.22 GECKO(23660) console.debug: "result" ({state:1, scope:"{SitePermissions.SCOPE_TEMPORARY}"})
Flags: needinfo?(pbz)

Thanks for the tips all! I'll retry without the extra tab to see if that helps.

Concerning the test failure: I see an unexpected permission prompt in checkAudioVideoGracePastStop. Is that the issue you're describing?

No, that should be fixed by D107769. Do you have it in your patch set?

Instead, I thought I was seeing is all my explicit test assertions PASSing, but interleaved with the same noise about "not expected in content process". But it was late, so I'll retest this morning.

It was pilot error on my part. Works better now. Thanks again!

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

Thanks for the tips all! I'll retry without the extra tab to see if that helps.

Concerning the test failure: I see an unexpected permission prompt in checkAudioVideoGracePastStop. Is that the issue you're describing?

No, that should be fixed by D107769. Do you have it in your patch set?

Instead, I thought I was seeing is all my explicit test assertions PASSing, but interleaved with the same noise about "not expected in content process". But it was late, so I'll retest this morning.

I didn't get that error because I was building in artifact mode. Now that I've done a full compile I can see the same issue.

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

It was pilot error on my part. Works better now. Thanks again!

Glad you managed to resolve the issue! I've seen that other tests use await disableObserverVerification(); before reload. Maybe that's related?

Depends on: 1698000
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9895c7f1b26
Add a 50s grace period for re-requesting a camera or microphone device in a tab. r=mconley,johannh
https://hg.mozilla.org/integration/autoland/rev/816d2b917939
Fix bug where nsIMediaDevice active devices list would omit camera from joint gUM requests. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/df230c366ab3
Test permission grace periods. r=mconley,pbz

Backed out 3 changesets (bug 1693677) for browser_devices_get_user_media_grace.js failures.

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=FlqL-HPCRwKAEtDPr8M5NA.0&fromchange=5dc83a74daabeecc37cb2902a41c781d178f167e&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64%2Fdebug-mochitest-browser-chrome-fis-e10s%2Cbc14&tochange=2291da90d977ef992be5c132fd5706b5e65085a6

Backout link: https://hg.mozilla.org/integration/autoland/rev/2291da90d977ef992be5c132fd5706b5e65085a6

Failure log: https://treeherder.mozilla.org/logviewer?job_id=333066779&repo=autoland&lineNumber=19040

...
[task 2021-03-12T22:38:56.289Z] 22:38:56     INFO - Navigate to a different-origin page
[task 2021-03-12T22:38:56.289Z] 22:38:56     INFO - After navigating to a different-origin page, gUM(camera+mic) causes a prompt.
[task 2021-03-12T22:38:56.289Z] 22:38:56     INFO - requesting devices
[task 2021-03-12T22:38:56.289Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | webRTC-shareDevices notification shown - 
[task 2021-03-12T22:38:56.289Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | notification panel open - 
[task 2021-03-12T22:38:56.290Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | notification panel populated - 
[task 2021-03-12T22:38:56.290Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | microphone selector visible - 
[task 2021-03-12T22:38:56.293Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | Selector list should be hidden. - 
[task 2021-03-12T22:38:56.293Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | Selector label should not be hidden. - 
[task 2021-03-12T22:38:56.293Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | Label should be showing the lone device label. - 
[task 2021-03-12T22:38:56.293Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | camera selector visible - 
[task 2021-03-12T22:38:56.294Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | Selector list should be hidden. - 
[task 2021-03-12T22:38:56.294Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | Selector label should not be hidden. - 
[task 2021-03-12T22:38:56.295Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | screen selector hidden - 
[task 2021-03-12T22:38:56.297Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | expected nothing to be shared - {} deepEqual {} - 
[task 2021-03-12T22:38:56.297Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | no sharing indicator on the control center icon - 
[task 2021-03-12T22:38:56.297Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | WebRTC indicator hidden - 
[task 2021-03-12T22:38:56.298Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | camera global indicator as expected - 
[task 2021-03-12T22:38:56.298Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | microphone global indicator as expected - 
[task 2021-03-12T22:38:56.299Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | screen global indicator as expected - 
[task 2021-03-12T22:38:56.300Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | WebRTC menu should be hidden - 
[task 2021-03-12T22:38:56.300Z] 22:38:56     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | popup WebRTC indicator hidden - 
[task 2021-03-12T22:38:56.301Z] 22:38:56     INFO - Navigate back to the first page
[task 2021-03-12T22:38:56.301Z] 22:38:56     INFO - Buffered messages logged at 22:37:39
[task 2021-03-12T22:38:56.302Z] 22:38:56     INFO - After navigating back to the first page, gUM(camera+mic) returns a stream without prompting within grace period.
[task 2021-03-12T22:38:56.305Z] 22:38:56     INFO - requesting devices
[task 2021-03-12T22:38:56.309Z] 22:38:56     INFO - Buffered messages finished
[task 2021-03-12T22:38:56.310Z] 22:38:56     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | Test timed out - 
[task 2021-03-12T22:38:56.310Z] 22:38:56     INFO - GECKO(8321) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2021-03-12T22:38:56.310Z] 22:38:56     INFO - GECKO(8321) | MEMORY STAT | vsize 3167MB | residentFast 347MB | heapAllocated 101MB
[task 2021-03-12T22:38:56.311Z] 22:38:56     INFO - TEST-OK | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | took 90049ms
[task 2021-03-12T22:38:56.311Z] 22:38:56     INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-03-12T22:38:56.311Z] 22:38:56     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_grace.js | Found a tab after previous test timed out: https://example.com/browser/browser/base/content/test/webrtc/get_user_media.html - 
[task 2021-03-12T22:38:56.312Z] 22:38:56     INFO - GECKO(8321) | [Child 8560, Main Thread] WARNING: NS_ENSURE_TRUE(!mHasOrHasHadOwnerWindow || mOwnerWindow) failed: file /builds/worker/checkouts/gecko/dom/events/DOMEventTargetHelper.cpp:327
[task 2021-03-12T22:38:56.312Z] 22:38:56     INFO - GECKO(8321) | [Child 8380: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 7ff86a3c4800 == 1 [pid = 8380] [id = 1]
[task 2021-03-12T22:38:56.315Z] 22:38:56     INFO - GECKO(8321) | [Child 8380: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (7ff886d63ac0) [pid = 8380] [serial = 5] [outer = 0]
[task 2021-03-12T22:38:56.316Z] 22:38:56     INFO - GECKO(8321) | [Child 8380: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (7ff86a3c6400) [pid = 8380] [serial = 6] [outer = 7ff886d63ac0]
[task 2021-03-12T22:38:56.316Z] 22:38:56     INFO - checking window state
...
Flags: needinfo?(jib)
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15139da9aa2a
Add a 50s grace period for re-requesting a camera or microphone device in a tab. r=mconley,johannh
https://hg.mozilla.org/integration/autoland/rev/e9d57d671fa5
Fix bug where nsIMediaDevice active devices list would omit camera from joint gUM requests. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/10a2a44e4e25
Test permission grace periods. r=mconley,pbz
Flags: needinfo?(jib)

https://hg.mozilla.org/mozilla-central/rev/15139da9aa2a
https://hg.mozilla.org/mozilla-central/rev/e9d57d671fa5
https://hg.mozilla.org/mozilla-central/rev/10a2a44e4e25

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox88: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Regressions: 1698513
See Also: → 905696

Marking this as verified-fixed based on the testing done for the QA PI ticket. Testing was done on:

  • Firefox Nightly 88
  • Firefox Beta 88

Covered Platforms:

  • Windows 7/10
  • MacOS 10.15
  • Ubuntu 20.04
Status: RESOLVED → VERIFIED
status-firefox88: fixed → verified
Depends on: 1712957
Regressions: 1714410
No longer regressions: 1714410
Regressions: 1799822
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: