Johann Hofmann [:johannh] Assignee
	Description • 8 years ago

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.

	:Paolo Amadini
	Comment 1 • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 3 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 4 • 8 years ago

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.

	:Paolo Amadini
	Comment 5 • 8 years ago
mozreview-review

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.

	Johann Hofmann [:johannh] Assignee
	Comment 7 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 8 • 8 years ago

> 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 :)

	Johann Hofmann [:johannh] Assignee
	Comment 11 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 12 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 13 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 15 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 16 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 17 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 18 • 8 years ago

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

	Pulsebot
	Comment 20 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2368254878c5

	Johann Hofmann [:johannh] Assignee
	Comment 22 • 8 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 23 • 8 years ago

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!

	Hani Yacoub, Desktop QA
	Comment 24 • 8 years ago

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.

	Carsten Book [:Tomcat]
	Comment 25 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/f851b8e0286b

	Paul Silaghi, QA [:pauly]
	Comment 26 • 8 years ago

Verified fixed FX 53.0a2 (2017-02-02) (32-bit) Win 7