32284 - [RFE]option for cookie treatment: "allow-for-one-session"

Status: VERIFIED DUPLICATE of bug 8530

(Core :: Networking: Cookies, enhancement, P3)

Description

[msuencks]

the option should make the browser accept
cookies but delete them after the browser
is shut down.

additionally if this option is checked by the user,
an additional panel could be activated where
the user can list domains where "permanent" storage
of cookies would be allowed.

IMO this could lead to a broader acceptance of 
cookies which are a useful tool in designing web sites.

[ when i let my imagination run wild this may even
  become the default setting for Mozilla .. :-) ]

Comment 1

[Stephen P. Morse]

Of course you can currently do exactly what you are asking for by always 
executing the browser from a two-line batch file.  The first line in the batch 
files invokes the browser and the second deletetes the cookies file.

I'm marking this as M20 unless somebody steps up and volunteers to implement it 
sooner.

Comment 2

[msuencks]

I am aware of this solution but being involved
in the design of web application I often hear from
clients that they fear loosing customers
if their e.g. shop relies on cookies for user
identification. this is mainly because cookies
are "bad" in public opinion (at least here in germany)

many online shops (Amazon for example) move
away from using cookies although they are 
so extremely nice to use for programming.

the proposed option could make a clear distinction
between cookies used for the needs of online shops
and those cookies who have expiry times up
to the year 2038 and which even I would consider "evil".

** another option comes to mind: a configurable
maximum duration for which a cookie is saved

Comment 3

[Stephen P. Morse]

For the shop that fears losing customers because they need cookies, they could 
always set their cookies to expire at end of session.  So for such nice sites, 
there is no need for the browser to do so.

The idea of limiting the maximum lifetime of a cookie has come up before.  See 
bug 8530.

BTW, please don't go removing or changing the Target Milestone designations 
unless you are prepared to do the implementation.  I need to schedule my bugs as 
to which ones I work on when and that is what the Target Milestone is used for.  
Only the person who is going to work on the bug should be the one changing the 
Target Milestone.

Comment 4

[msuencks]

> for the shop that fears losing customers because they need cookies, they could 
> always set their cookies to expire at end of session

the shops we design do just that :-)

the point I would like to make is: although I can
design my shop to be user friendly , I cannot 
protect the customers from other , bad designed
sites. so the users has only the choice to disable
completely or live with it (or block site after site
with the new feature from Mozilla5 which is just
the first the step in my eyes - ymmv)

on your comment about the implementor of all this -
just downloaded the source .. I have somehow the
mad idea to use mainly existing code snippets to do the
thing .. maybe I can produce a bug or two :-)


> BTW, please don't go removing or changing the Target Milestone designations 
> unless you are prepared to do the implementation

I'm sorry - that wasn't my intention. somehow navigated back
to my inital form window to submit .. will pay more
attention next time on this "submit anyway ?" button.

Comment 5

[Stephen P. Morse]

Sounds like you are volunteering to do the implementation.  In that case, I'll 
volunteer to do the code review for you and to check it in once it's working.

You might want to combine it with bug 8530.  There already exists a patch for 
that implementation so you could even use that as a starting point.  It allows a 
user to specify a max expiration time in days and trim cookies whose 
actual expiration time exceed that amount.  All you would need to do is 
special-case the max value of 0 to mean that all cookie expiration times get set 
to expire-at-end-of-session.

Comment 6

[msuencks]

sounds good. I will look what I can do and will ask
Greg what it status of his code is..

Comment 7

[Stephen P. Morse]

.

Comment 8

[Stephen P. Morse]

Reopened per management's every changing mind.

Comment 9

[msuencks]

I looked at the patch from the other bug
you referenced me to and that one was
already doing quite well. some changes
to the xul stuff were necessary and I implemented a
special-case as you suggested and which are
quite straight forward.

need to catchup with latest changes of the tree
however and will submit it a day or two.

Comment 10

[Stephen P. Morse]

There were some problems with the patch that I picked up in a code review.  Most 
notably, the test for excessive cookie lifetime was incorrect -- a "<" instead 
of a ">" -- which meant that the code couldn't possibly have worked.  Below are 
my code review comments (and Greg's replies, and my replies to his replies, etc) 
which you'll need to apply to his patch.

--------- MY CODE REVIEW ---------------

1. Argument to cookie_SetLifetimeLimit()

   shouldn't this be a time_t instead of an int?

2. GetLifetimeTime()

   shouldn't return value be a time_t as well?

   shouldn't name be GetLifetimeLimit to be consistent with your other
routines?

   use of time(NULL) doesn't work on mac -- it will return seconds since
1900 instead of 1970.  See the newly-added get_current_time routine.


3. GetLifetimeAsk

   shouldn't argument be a time_t instead of an int?

   shouldn't test be cookie_GetLifetimeTime() <= expireTime instead of
cookie_GetLifetimeTime() >= expireTime?

4. The following added code could have been done with a single if
statement

+   if(cookie_GetLifetimePref() == COOKIE_Discard) {
+     if(cookie_GetLifetimeTime() < timeToExpire) {
+       PR_Free(cur_path);
+       PR_Free(cur_host);
+       return;
+     }
+   }

5. You're going to be giving the same message in the cookie nag box when
the cookie warning pref is on as you will when an excessive lifetime
cookie is encountered.  Won't that be confusing.  The nag says:

   Site X wants to set another cookie.  You already have X cookies from
this site.  Do you want to allow it?

and the user has no idea why you are suddenly asking this.  For your
case, shouldn't you say something like

   Site X wants to set a cookie that will expire in greater than X
days.  Do you want to allow it?

6. I like some of the optimizations you made to other parts of the
code.  Thanks.

-------------- GREG'S REPLY TO MY CODE REVIEW ---------------------

> 1. Argument to cookie_SetLifetimeLimit()
>    shouldn't this be a time_t instead of an int?

No.  The argument is in days, not seconds.  An int is appropriate.

> 2. GetLifetimeTime()
>    shouldn't return value be a time_t as well?

Sure, it's probably more portable for the future.

>    shouldn't name be GetLifetimeLimit to be consistent with your other
> routines?

No, the internal value is the limit, but this accessor routine returns the
expiration _time_, not the limit.

>    use of time(NULL) doesn't work on mac -- it will return seconds since
> 1900 instead of 1970.  See the newly-added get_current_time routine.

OK.

> 3. GetLifetimeAsk
>    shouldn't argument be a time_t instead of an int?

Yes.

>    shouldn't test be cookie_GetLifetimeTime() <= expireTime instead of
> cookie_GetLifetimeTime() >= expireTime?

No, it returns true if (a) asking is enabled and (b) the cookie's lifetime
is excessive.  That is, if it returns true, ask about the cookie.

> 4. The following added code could have been done with a single if
> statement

Yes, it could.  I used the same style as the surrounding code.

> 5. You're going to be giving the same message in the cookie nag box when
> the cookie warning pref is on as you will when an excessive lifetime
> cookie is encountered.  Won't that be confusing.  [sic - "?"] ...

And you'll notice that my covering note said the same thing.  You'll also
notice that the UI doesn't have that mode.  There's much work to do in the
general area of remembering site-specific options; once the groundwork for
this cookie mode was established, I was going to contact [e-mail]
and co-ordinate that development with him.

-------------- MY REPLY TO GREG'S REPLY ----------------------------

> > 1. Argument to cookie_SetLifetimeLimit()
> >    shouldn't this be a time_t instead of an int?
> 
> No.  The argument is in days, not seconds.  An int is appropriate.

Ah, yes, you are correct.  I didn't look at that carefully enough.

One more comment here.  Since you are leaving it as an int, at least
make it a PRInt32.  Makes it a bit more portable.

> >    shouldn't name be GetLifetimeLimit to be consistent with your other
> > routines?
> 
> No, the internal value is the limit, but this accessor routine returns the
> expiration _time_, not the limit.

It confused me to have SetLifetimeLimit and LifetimeLimitPrefChanged but
then GetLifetimeTime.  But now that you explained, I understand why you
did that.  You're correct.

> >    shouldn't test be cookie_GetLifetimeTime() <= expireTime instead of
> > cookie_GetLifetimeTime() >= expireTime?
> 
> No, it returns true if (a) asking is enabled and (b) the cookie's lifetime
> is excessive.  That is, if it returns true, ask about the cookie.

I don't follow.  I understand that this function returning true means
that we have to ask about the cookie.  And I understand that one
condition for this asking is that the LifetimePref needs to be enabled. 
But let's look at the other condition -- the test for cookie lifetime
being excessive.

The actual lifetime of the cookie is in the argument "expireTime".  And
the maxable allowable lifetime is what we get from your function
cookie_GetLifetimeTime.  So we need to see if

   actual-lifetime-of-cookie > maximum-allowable-lifetime

which translates into

   expireTime > cookie_GetLifetimeTime()

and that's just the opposite sense of the test that you are making.

> > 5. You're going to be giving the same message in the cookie nag box when
> > the cookie warning pref is on as you will when an excessive lifetime
> > cookie is encountered.  Won't that be confusing.  [sic - "?"] ...
> 
> And you'll notice that my covering note said the same thing.  You'll also
> notice that the UI doesn't have that mode.  There's much work to do in the
> general area of remembering site-specific options; once the groundwork for
> this cookie mode was established, I was going to contact [e-mail]
> and co-ordinate that development with him.

OK, I missed the fact that the UI didn't allow for this.  I didn't
review your UI changes because, as we agreed, we won't be checking that
in at this time.  First we'll get the feature in as a stealth feature
(accessible only by hand-editing the prefs.js file) and later seeing if
we get approval from the UE team to make the UI change.

The only changes that I'll be checking in at this time are the ones to
nsCookie.cpp and to all.js.  However I'll make the changes to
pref-cookies.dtd that have to do with improving wording on existing
radio buttons but not those for the wording on new radio buttons.

So please send me the revised patch for nsCookie.cpp and I'll get it
into the tree as soon as the M14 freeze is lifted.  Thanks again.

------------------ GREG'S REPLY TO MY REPLY TO GREG'S REPLY -------------

> and that's just the opposite sense of the test that you are making.

There was a reason it had to be done this way, but looking at the code, I
don't see why now---maybe it was something I cleaned up.  I can't build
now due to breakage elsewhere, so I can't try it, but since it's part of
the code that won't be exercised for the time being, I'll change it as you
suggest and debug it later.

I also changed the argument value to RPInt32 as you suggested.  Since the
input value is constrained to four digits, even a 16-bit int would work,
but it's harmless to provide the additional portibility.

Comment 11

[Stephen P. Morse]

And, finally,

--------- MY REPLY TO GREG'S REPLY TO MY REPLY TO GREG'S REPLY -----------

That's PRInt, not RPInt

Comment 12

[msuencks]

took longer because I've been somewhat busy
the last weeks.

I talked with Greg Noel and there
is a new patch which also includes my
desired feature.

I will post it under the original bug id #8530

Comment 13

[Stephen P. Morse]

Marking this as a dup of bug 8530.  It's really a subset of that bug but we 
don't have such a catagory, so dup will have to do.

*** This bug has been marked as a duplicate of 8530 ***

Comment 14

[Katcha Da]

Verified duplicate.