Currently persistent permissions can be set to expire at a particular time using EXPIRE_TIME. But session permissions can not be set to expire at a particular time. For the plugin click-to-activate UI, the current UX spec is that the short permission will expire when the sessions ends *or* an hour of inactivity. I managed to do this merely by redefining the meaning of TYPE_SESSION so that if expireTime is 0 (the default) then it behaves as it does currently. But if expireTime is a real number, then we expire it.

Comment on attachment 752182 [details] [diff] [review] Allow EXPIRE_SESSION to also expire by time, rev. 1 Review of attachment 752182 [details] [diff] [review]: ----------------------------------------------------------------- Another approach would have been to consider that EXPIRE_TIME permissions expire also when the session is terminated. Otherwise, adding a new permission expire type called EXPIRE_TIME_SESSION. FWIW, I'm fine with the current approach as much as I would have been okay with those two (though, the former could have created regressions assuming some consumers relied n the behaviour). r/sr=me, with the comments applied and the requested tests added. ::: extensions/cookie/nsPermissionManager.cpp @@ +625,5 @@ > } > > + if (aExpireTime == nsIPermissionManager::EXPIRE_SESSION && > + aExpireTime != 0 && > + aExpireTime <= (PR_Now() / 1000)) { I guess we can merge this condition with the one above: if ((aExpireType == nsIPermissionManager::EXPIRE_TIME || (aExpireType == nsIPermissionManager::EXPIRE_SESSION && aExpireTime != 0) && aExpireTime <= (PR_Now() / 1000)) { You should probably explain in a comment what this is actually doing though. Also, I guess you probably had a bug here because there is a typo in this code. Could you add a test to test that behaviour? Just add some EXPIRE_SESSION and EXPIRE_TIME permissions that have an expireTime before PR_Now() and check that when enumerating all permissions, you haven't any new permissions. Counting them should do. @@ +1152,5 @@ > + if ((permEntry.mExpireType == nsIPermissionManager::EXPIRE_TIME && > + permEntry.mExpireTime <= (PR_Now() / 1000)) || > + (permEntry.mExpireType == nsIPermissionManager::EXPIRE_SESSION && > + permEntry.mExpireTime != 0 && > + permEntry.mExpireTime <= (PR_Now() / 1000))) { nit: you could also merge them as above but either is fine. ::: extensions/cookie/nsPermissionManager.h @@ +54,4 @@ > int64_t mExpireTime; > uint32_t mNonSessionPermission; > uint32_t mNonSessionExpireType; > + uint32_t mNonSessionExpireTime; You should modify the ctor too which is going to require modifying ctor calls. ::: extensions/cookie/test/unit/test_permmanager_expiration.js @@ +60,5 @@ > + // Check that .getPermission returns a matching result > + do_check_null(pm.getPermission(principal, "test/expiration-perm-exp", false)); > + do_check_null(pm.getPermission(principal, "test/expiration-session-exp", false)); > + do_check_null(pm.getPermission(principal, "test/expiration-perm-exp2", false)); > + do_check_null(pm.getPermission(principal, "test/expiration-session-exp", false)); There is a typo here, I think you meant to check "test/expiration-session-exp2".

Comment on attachment 752182 [details] [diff] [review] Allow EXPIRE_SESSION to also expire by time, rev. 1 Review of attachment 752182 [details] [diff] [review]: ----------------------------------------------------------------- What Mounir said.

There was a typo in this patch ("== 0" should be "!= 0" in nsPermissionManager::GetPermissionHashKey) which caused tests to fail (yay). Pushed with that corrected. https://hg.mozilla.org/integration/mozilla-inbound/rev/15df9914a1ad