202313 - Lifetime = 0 makes cookies disappear and reappear w/ multiple refreshes

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[benc]

STEPS:
Prefs - Enable all cookise + [x] ask me before storing a cookie + [x] Limit
maximum lifetime of cookies to: [0] days 
Open cookie manager

go to www.nytimes.com

The site currently trys to send three cookies "spopunder, tpopunder, RMID".
Accept them all.

You will see zero to all cookies added to cookie manger's "Stored Cookies" display.

Normally, the cookie will appear as soon as you click "accept".

NOTES:
I did a dupe check, and looked at bug 53354 briefly (I didn't have time to read
the entire history of the feature).

I found this while doing some edge case testing for the UI.

I don't know what zero days is supposed to mean, maybe we should just change the
UI to disallow that value.

Comment 1

[benc]

Sorry, forgot build. Jumping around alot today...

2003-04-16-09-trunk, commercial.

Comment 2

[[e-mail]]

yeah, good catch. we don't explicitly check for the case where the
user-specified max-expiry-time is 0... we just set the (already expired) cookie.
it'll get flushed from the list the next time the list is refreshed, though i'm
really not quite sure why that doesn't happen immediately with the cookiemgr open...

(when wallet receives the cookieChanged notification, it reloads the list, which
should remove expired cookies first... probably another wallet bug, but hey.)

so
a) if wallet worked, this problem wouldn't be visible in the first place, and
wouldn't really be an issue
b) we should probably add a zero-check to GetExpiry() so it doesn't set the cookie.

Comment 3

[[e-mail]]

-> me

Comment 4

[Nicholas Allen]

It seems like 0 should simply be an illegal value.  Is there any reasonable
interpretation of "0 days" that doesn't already have a separate, well-described
option for it?  And if so, how is anyone supposed to figure out what it is?

Comment 5

[benc]

If we cannot immediately decide what "zero" means programatically, lets just
disable it in prefs. The patch is fast, and we have to prevent people from
typing it in there anyhow...

Comment 6

[[e-mail]]

Attachment: proposed fix

this cleans up the cookie_GetExpiry() code a little bit, and changes the expiry
semantics in a few places throughout cookie code.

I think that putting a zero (or less conceivably, a negative) value in the
'limit lifetime to:' field is valid. it would mean that any cookies a site
tries to _set_ will be rejected, but still allows that site to _get_ an
existing cookie. so, it's not the same as just disabling cookies altogether -
this is an obscure case, sure, but it's just logical behavior IMO.

so, what this patch really does:
1) fixes GetExpiry() to deal with zero/negative values correctly. note that
this has changed our overflow behavior in a subtle way (see comment in code).
we will now be less tolerant of overflows, by expiring the cookie rather than
making it a session cookie. i don't think this really matters.

2) changes the expiry semantics throughout cookie code: now if |expiryTime ==
currentTime| we consider the cookie expired. this allows us to deal with the
zero case correctly, and is more logical I think.

benc, can you think this over and see what you think? lemme know if you have
questions about what's going on.

Comment 7

[[e-mail]]

Comment on attachment 122289 [details] [diff] [review]
proposed fix

seeking reviews.

mvl, darin: this isn't a big issue so feel free to punt it 'til you have time.

Comment 8

[Darin Fisher]

Comment on attachment 122289 [details] [diff] [review]
proposed fix

>Index: extensions/cookie/nsCookies.cpp

>+      if (gCookiePrefObserver->mCookiesLifetimeCurrentSession) {
>+        // limit lifetime to session
>+        return PR_TRUE;
>+      } else if (delta > nsInt64(gCookiePrefObserver->mCookiesLifetimeSec)) {

nit: kill the |else| after |return|.

looks fine to me otherwise.  sr=darin

Comment 9

[[e-mail]]

Attachment: v2

heh, that'll teach me to search/replace.

Comment 10

[[e-mail]]

Comment on attachment 122317 [details] [diff] [review]
v2

carrying over sr=darin. care to do the honors, mvl?

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 122317 [details] [diff] [review]
v2

So the overflow thing means that if you set a cookie to expire after the year
294474, it will be expired now, not in the distant future. That should never be
a problem in the real world.

Comment 12

[[e-mail]]

Comment on attachment 122317 [details] [diff] [review]
v2

requesting approval. this isn't a big issue - it just fixes a cookie pref to
have more logical behavior, and cleans up a bit of code. this is a very
low-risk patch.

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 122317 [details] [diff] [review]
v2

a=sspitzer

Comment 14

[[e-mail]]

checked in.

benc: could you run some tests past this and confirm that the cookie dialog
doesn't do funny things for max-lifetime zero or negative now? thanks ;)

Comment 15

[benc]

VERIFIED/Mach-O, commercial 2003-04-29-03 + Mozilla 1.4b.

This seems to have been working correctly before you made the comment the fix
was checked in. Did you make some changes that make wallet act better?

Based on some additional explainations you gave to me over IRC, it sounds like
the current behavior is this:

zero means set the cookies to expire now, and the fix makes sure the cookies are
never sent to wallet. This has the net effect (from some testing I've done), of
making mozilla work as a session-only cookie handler.

Let me know if that is what you intended.

As for testing negative values, I'd rather not (sorry!), unless you have some
deliberate programatic behavior. If people are not supposed to use negative
numbers, we should patch the prefs UI to disallow that. Someday I'll test all my
assigned prefs fields for incorrect values, but not anytime soon.