Expired temporary permissions don't always update the identity block

VERIFIED FIXED in Firefox 53

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

unspecified
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox53 verified, firefox54 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 attachments)

STR:

- Go to about:config
- Set privacy.temporary_permission_expire_time_ms to a low value, like 1000 (ms)
- Go to https://permission.site
- Temporarily deny a permission
- Wait a second
- Request the same permission again

Expected:

The identity block should hide the "blocked" icon

Actual:

The "blocked" icon prevails (see screenshot).

This problem obviously also occurs for the default pref value, e.g. when the user returns to their computer after an hour. Hence it's pretty relevant.

We probably have to emit a permission change event to the browser in the internal expiry check, too. I remember Paolo mentioned something about this in bug 1206232 but it got lost.
Flags: qe-verify+
Yeah, I didn't think about this possible real-world case, so I only asked to add a code comment.

However, it just occurred to me that firing the event in the "get" method is dangerous because we could inadvertently re-enter the identity block refresh code, with weird results like duplicate lines for other permissions. We should either find another way, or delay the event to the next tick. The latter might cause a small flicker though, which we might be able to avoid using a "Promise.resolve()" microtask instead of a full event loop tick.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Paolo, I thought about this over the weekend and I believe that this is the solution that comes with the least complexity and makes me feel best. This comes with the cost of the slight overhead of having to do this "manual" refresh in our permission consumers. But after thinking about this more I'm not sure if it's the task of the SitePermissions module to update the browser UI when permissions have expired. So I feel pretty confident about this change.
Comment on attachment 8829372 [details]
Bug 1331579 - Explicitly update the identity block on re-requesting expired permissions.

https://reviewboard.mozilla.org/r/106500/#review107474

Add a description of the behavior of PermissionStateChange in SitePermissions.jsm, which we forgot to do in the version that landed.

That's part of the module's public API, so the comments should be on a new JSDoc for the SitePermissions object, on the "set" method, and/or on the "get" method (mentioning the event isn't raised).

::: browser/base/content/test/general/browser_temporary_permissions.js:43
(Diff revision 1)
> +  SpecialPowers.pushPrefEnv({set: [
> +        ["privacy.temporary_permission_expire_time_ms", 100],
> +        ["media.navigator.permission.fake", true],
> +  ]});
> +
> +  let uri = Services.io.newURI("https://example.com")

nit: semicolon

::: browser/base/content/test/general/browser_temporary_permissions.js:46
(Diff revision 1)
> +  ]});
> +
> +  let uri = Services.io.newURI("https://example.com")
> +
> +  yield BrowserTestUtils.withNewTab(PERMISSIONS_PAGE, function*(browser) {
> +    for (let id of ["camera", "geo"]) {

Add a comment to mention that we've selected these two permissions specifically to test the two different code paths.

::: browser/base/content/test/general/browser_temporary_permissions.js:59
(Diff revision 1)
> +        scope: SitePermissions.SCOPE_TEMPORARY,
> +      });
> +
> +      ok(blockedIcon.hasAttribute("showing"), "blocked permission icon is shown");
> +
> +      yield new Promise((c) => setTimeout(c, 500));

Worth defining 100ms and 500ms as constants in head.js, so we can change them in a single place if needed, and conversely find out which tests depend on them.
Attachment #8829372 - Flags: review?(paolo.mozmail) → review+
> Worth defining 100ms and 500ms as constants in head.js, so we can change them in a single place if needed, and conversely find out which tests depend on them.

One test is testing the module in browser/modules and the other is in browser/base/content because it's testing UI functionality mostly. I've moved them to constants to avoid future copy pasting anyway :)
Depends on: 1334421
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2368254878c5
Explicitly update the identity block on re-requesting expired permissions. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/2368254878c5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Iteration: --- → 54.1 - Feb 6
Comment on attachment 8829372 [details]
Bug 1331579 - Explicitly update the identity block on re-requesting expired permissions.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1206232
[User impact if declined]: Users who have a tab with temporarily blocked permissions open for longer than an hour will experience inconsistent UI (leftover blocked permission icons)
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: See STR in comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not much
[Why is the change risky/not risky?]: The change consists of only two lines that simply trigger an event and do not interact with the remaining code. An exception in the new code would arguably make things worse, but that's very unlikely and the code is well tested.
[String changes made/needed]: None
Attachment #8829372 - Flags: approval-mozilla-aurora?
QA Contact: paul.silaghi
Comment on attachment 8829372 [details]
Bug 1331579 - Explicitly update the identity block on re-requesting expired permissions.

Recent regression, let's uplift to 53. 
Looks like we already have testing lined up, thanks Paul!
Attachment #8829372 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Build ID: 20170131030205
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1.
Status: RESOLVED → VERIFIED
Verified fixed FX 53.0a2 (2017-02-02) (32-bit) Win 7
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.