superreview requested: [Bug 874197] Allow session-based permissions to expire at a fixed time : [Attachment 752182] Allow EXPIRE_SESSION to also expire by time, rev. 1

[bugzill...@mozilla.org]

Benjamin Smedberg [:bsmedberg] <benj...@smedbergs.us> has asked Mounir
Lamouri (:mounir) <mou...@lamouri.fr> for superreview:
Bug 874197: Allow session-based permissions to expire at a fixed time
https://bugzilla.mozilla.org/show_bug.cgi?id=874197

Attachment 752182: Allow EXPIRE_SESSION to also expire by time, rev. 1
https://bugzilla.mozilla.org/attachment.cgi?id=752182&action=edit

[bugzill...@mozilla.org]

Mounir Lamouri (:mounir) <mou...@lamouri.fr> has granted Benjamin Smedberg
[:bsmedberg] <benj...@smedbergs.us>'s request for superreview:

Bug 874197: Allow session-based permissions to expire at a fixed time
https://bugzilla.mozilla.org/show_bug.cgi?id=874197

Attachment 752182: Allow EXPIRE_SESSION to also expire by time, rev. 1
https://bugzilla.mozilla.org/attachment.cgi?id=752182&action=edit

------- Additional Comments from Mounir Lamouri (:mounir) <mou...@lamouri.fr>
Review of attachment 752182:
-----------------------------------------------------------------

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".