Closed Bug 213963 Opened 21 years ago Closed 20 years ago

Cookies get lost after a huge number of cookies is reached

Categories

(Core :: Networking: Cookies, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: wfor, Assigned: dwitte)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5)

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312

Hard to explain ... There is s very popular site in germany, it is
http://www.heise.de, this site sets a lot of cookies. After a longer session the
number of cookies grows to a large number and suddenly some are lost or get
garbadged.

The output is created by doing a 'strings -a tcpdump | grep Cookie | uniq'
[[The X, Y & Z are just used to prevent any misuse]]

Here is one example of a garbagde Cookie, see end of the line
personality=md5&XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX&kind&p&un&YYYYY&uid&ZZZZZ;
collaps_expand=cea&c; forum_view=v&t; hide_show=a&s;
kill_list=kl&148684%2C170476%2C175404%2C175404%2C172937; show_preview=sp&y;
u2u_fora_list=u2ufl&; accept_cookie=ac&1; 45406=m&&cef&e; 7262=m&&cef&e;
35634=m&&cef&e; 45400=m&&cef&e;+#"?F

After accessing some pages it seems to be sane again ...
Set-Cookie: 44545=m&&cef&c; path=/
Cookie:
personality=md5&XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX&kind&p&un&YYYYY&uid&ZZZZZ;
collaps_expand=cea&c; forum_view=v&t; hide_show=a&s;
kill_list=kl&148684%2C170476%2C175404%2C175404%2C172937; show_preview=sp&y;
u2u_fora_list=u2ufl&; accept_cookie=ac&1; 45406=m&&cef&e; 7262=m&&cef&e;
35634=m&&cef&e; 45400=m&&cef&e; 45385=m&&cef&c; 45382=m&&cef&e; 45392=m&&cef&e;
45396=m&&cef&e; 45405=m&&cef&e; 44548=m&&cef&e; 44547=m&&cef&e; 44545=m&&cef&c

This one is the next request, just one click awy from the one above. As you can
see the 'personality' cookie is gone
Set-Cookie: 44545=m&&cef&e; path=/
Cookie: collaps_expand=cea&c; forum_view=v&t; hide_show=a&s;
kill_list=kl&148684%2C170476%2C175404%2C175404%2C172937; show_preview=sp&y;
u2u_fora_list=u2ufl&; accept_cookie=ac&1; 45406=m&&cef&e; 7262=m&&
cef&e; 35634=m&&cef&e; 45400=m&&cef&e; 45385=m&&cef&c; 45382=m&&cef&e;
45392=m&&cef&e; 45396=m&&cef&e; 45405=m&&cef&e; 44548=m&&cef&e; 44547=m&&cef&e;
44545=m&&cef&e

The full tcpdump is available, so feel free to ask questions

Maybe related ... Heise is transfering pages in compressed form as you can see
here: Transfer-Encoding: chunked

Reproducible: Always

Steps to Reproduce:
0. You need an account at www.heise.de, it is a german page, however
you can create an account here: http://www.heise.de/registration/ There might be
a delay to get write access, do not know of any delay for read access ...

1. Start with this page: http://www.heise.de/foren/go.shtml
2. In the list of discussions do
loop:
3. Click onto 'Programmieren:Sonstige' --> This sets an additional Cookie
4. Click onto 'Aufklappen' --> This changes a Cookie
5. Go Back in history
6. Go back again in history, now you reach the list again
7. Back to pint 3. above, but now click onto the link below 'Programmieren:Sonstige'
end loop
Actual Results:  
After a while (~10 loops), you can see that some cookie gets crambled or lost.
I just checked, the cookie cannot be seen in the cookie manager, although it was
a permanent cookie with an end of life set as :expires=Sun, 24-Aug-2003 18:48:02 GMT

Expected Results:  
Of course, no cookies should be lost :-)

The tcpdump file is saved, if any additional information is needed, feel free to
ask.
We actually have a hard cap on the number of cookies that can be stored to
prevent some sort of security attack, iirc...
Whiteboard: DUPEME
See
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookieService.cpp#92 :

static const PRInt32 kMaxNumberOfCookies = 300;
static const PRInt32 kMaxCookiesPerHost = 20;      <-- is it this one ?
static const PRUint32 kMaxBytesPerCookie = 4096;

When the limit is reached, we're supposed to start removing older cookies, based
on last access. Maybe we remove cookies that are still in use ?

Reporter, we had a big cookie-rewrite between Mozzill 1.3 and 1.4, so maybe this
is already fixed. Can you upgrade and test again ?
Upgraded as you suggested to 1.4, very similar behavior. I counted exactly, to
reproduce you need to open 15 of those links in the list as described in the
'Steps to Reproduce'.

Again, tcpdump and then ...

strings -10 tcpout1.4| grep Cookie | uniq

This here is just before 
Cookie: show_preview=sp&y;
personality=md5&XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX&kind&p&un&YYYY&uid&ZZZZZ;
collaps_expand=cea&c; forum_view=v&t; hide_show=a&s;
kill_list=kl&148684%2C170476%2C175404%2C175404%2C172937; u2u_fora_list=u2ufl&;
44548=m&&cef&e; 44547=m&&cef&e; 44546=cef&e&m&; 44545=m&&cef&e; 44438=cef&e&m&;
44342=m&&cef&e; 43922=m&&cef&e; 43282=m&&cef&e; 43281=m&&cef&e; 42177=m&&cef&e;
39795=m&&cef&e; 39787=m&&cef&e; 36485=m&&cef&c
Set-Cookie: collaps_expand=cea&c; path=/
Set-Cookie: forum_view=v&t; path=/
Set-Cookie: hide_show=a&s; path=/
Set-Cookie: kill_list=kl&148684%2C170476%2C175404%2C175404%2C172937; path=/
Set-Cookie: show_preview=sp&y; path=/
Set-Cookie: u2u_fora_list=u2ufl&; path=/
Set-Cookie: accept_cookie=ac&1; path=/
Set-Cookie: 36485=cef&e&m&; path=/
Cookie:
personality=md5&XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX&kind&p&un&YYYY&uid&ZZZZZ;
collaps_expand=cea&c; forum_view=v&t; hide_show=a&s;
kill_list=kl&148684%2C170476%2C175404%2C175404%2C172937; u2u_fora_list=u2ufl&;
44548=m&&cef&e; 44547=m&&cef&e; 44546=cef&e&m&; 44545=m&&cef&e; 44438=cef&e&m&;
44342=m&&cef&e; 43922=m&&cef&e; 43282=m&&cef&e; 43281=m&&cef&e; 42177=m&&cef&e;
39795=m&&cef&e; 39787=m&&cef&e; 36485=cef&e&m&; accept_cookie=ac&1
Set-Cookie: 36478=m&&cef&c; path=/
Set-Cookie: show_preview=sp&n; path=/

And here ist is gone :-(

Cookie: forum_view=v&t; hide_show=a&s;
kill_list=kl&148684%2C170476%2C175404%2C175404%2C172937; u2u_fora_list=u2ufl&;
44548=m&&cef&e; 44547=m&&cef&e; 44546=cef&e&m&; 44545=m&&cef&e; 44438=cef&e&m&;
44342=m&&cef&e; 43922=m&&cef&e; 43282=m&&cef&e; 43281=m&&cef&e; 42177=m&&cef&e;
39795=m&&cef&e; 39787=m&&cef&e; 36485=cef&e&m&; accept_cookie=ac&1;
36478=m&&cef&c; show_preview=sp&n
Set-Cookie: 36478=m&&cef&e; path=/
Cookie: forum_view=v&t; hide_show=a&s;
kill_list=kl&148684%2C170476%2C175404%2C175404%2C172937; u2u_fora_list=u2ufl&;
44548=m&&cef&e; 44547=m&&cef&e; 44546=cef&e&m&; 44545=m&&cef&e; 44438=cef&e&m&;
44342=m&&cef&e; 43922=m&&cef&e; 43282=m&&cef&e; 43281=m&&cef&e; 42177=m&&cef&e;
39795=m&&cef&e; 39787=m&&cef&e; 36485=cef&e&m&; accept_cookie=ac&1;
36478=m&&cef&e; show_preview=sp&n


My new user Agent string:  User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
rv:1.4) Gecko/20030624


If you allow me to do some suggestions. I think it is okay to store only some
number of cookies and to delete older ones when this number would get exceeded.

Maybe you can change to logic which decides which cookie gets deleted al littel
bit. For this page, the most important cookie is personality, which *I* usually
use as a permanent cookie, which should be valid for one month. All the other
cookies are temporary ones which do not hurt. So, maybe you can first delete by
whatever logic all those temporary cookies, identified by *no* expiration date.

However, maybe it still would make sense to increase the number of cookies per site?

Thank you for your quick response
Hmm... the cookie spec (rfc2109) specifies these 20/site and 300/total limits,
so the site is violating the spec here - I'd say this one is evang.

I'm not really sure about the idea of expiring session cookies before cookies
with expiry times set, when the limit is reached - although whatever we do will
certainly be affecting an edge case. I don't like adding random hacks like that...
Well I can understand that you guys do not want to violate any RFC, this is okay.

What about making those limits customizable? There is already a huge bulk of
parameters which can be changed from the outside, 2 more or less do not hurt?

[[However, in parallel I will contact www.heise.de for a different solution,
like resetting those *important* cookies every time]]
I'd recommend just getting the site to use cookies properly. It shouldn't be
setting so many cookies - it can just append its data to an existing cookie or
something.
Just quoting from RFC 2109.

-------------8<-----------------------
6.3  Implementation Limits

   Practical user agent implementations have limits on the number and
   size of cookies that they can store.  In general, user agents' cookie
   support should have no fixed limits.  They should strive to store as
   many frequently-used cookies as possible.  Furthermore, general-use
   user agents should provide each of the following minimum capabilities
   individually, although not necessarily simultaneously:

      * at least 300 cookies

      * at least 4096 bytes per cookie (as measured by the size of the
        characters that comprise the cookie non-terminal in the syntax
        description of the Set-Cookie header)

      * at least 20 cookies per unique host or domain name

   User agents created for specific purposes or for limited-capacity
   devices should provide at least 20 cookies of 4096 bytes, to ensure
   that the user can interact with a session-based origin server.
-------------8<-----------------------

I see always the wording 'at least' or 'minimum' I do not see anywhere a maximum
number. Well, I agree that Mozilla does not violate the RFC, because the minimum
requirements on cookies are satisfied.

Now, what speaks against introducing a new set of tunable parameter like

network.cookie.cookiesPerHost
network.cookie.maxNumberOfOverallCookies
network.cookie.maxBytesPerCookie

to initialize

static const PRInt32 kMaxNumberOfCookies = 300;
static const PRInt32 kMaxCookiesPerHost = 20;
static const PRUint32 kMaxBytesPerCookie = 4096;

to different values tunable by the user?

Those 2 variables are only used in this single file, so they should not cause
any harm to the rest of the code?
Sure, we could, but why? If your goal is to allow users to avoid random problems
like the one this site exhibits, that's not a sensible solution. We can't expect
users to go fiddling with these prefs, so I don't really see the point in making
them customizable. There's no good reason why a site should be exceeding these
limits, unless it's broken. (Making it a pref would also introduce other
complications - what if the user changes the pref during a mozilla session? We
need extra code to traverse the cookie database and expunge all cookies over
that limit, then.)

There may be other reasons for doing it, however. Now that cookies is going into
necko, would embeddors want to tune these things? e.g. if we increase these
limits to 20/1000, would minimo applications perhaps want less?

i wonder if darin has an opinion :)
Well I would like to come back to the limited cookie count. From my point of
view there is one reason to overcome this limitation: All the other browsers
seem to work without these constraints. From the user point of view all the
other browsers are working except Mozilla/Firebird. Therfore I would like to ask
you to reconsider about the current limitation.

Many thanks
Ulf
define "work"? can you give me a specific example of something that breaks, or
is otherwise suboptimal, with the current limits?

i'm not averse to increasing them, but we should have a good reason to do it.
Assignee: darin → dwitte
Many thanks for your feedback

Well I guess the original application was somehow much too complicated. We do
have a technical magazine editor over here, who offers a lot of services like
IT- news, virus alterts etc. by means of a single login procedure to the public
FOC. 

Pls.  visit
 
http://www.heise.de/mediadaten/online/zugriff.shtml 

just to get an idea of the different services they are offering and the access
figures. Even if it might look as a somehow strange language for your eyes, the
different services are listed in the lower half of the table. As they have a
huge traffic amount they are using one cookie for login purposes and are feeding
back individual cookies for setting up different kind of services and individual
setups. I guess the idea is to keep the traffic high by storing the setup
cookies on the client computer. If you are acessing different services in
parallel chances are that the client computer will run against the upper margin
of currently 20 cookies. And because of the FIFO handling of cookies the very
first entry which is in fact the user id will get lost. For the average user the
browser seems to be the cause of some strange misbehaviour. In fact the site is
adressing this in the FAQ section. As I understand the RFC- limit of 20
cookies/server is a lower limit. Therfore it should be no violation to the RFC
to increase the limit. 

I run accross this problem when I wanted to report a different behaviour of
Firebird while surfing on that site. Intentionally it seems that the browser
forgets about the visited links. This seems to be related to the lost login
cookie however I can not exactly isolate and reproduce this behaviour. When
visiting bugzilla I stumbled over this bug report.

As the editors are adding their own Linux edition FOC to their magazines for
promoting Open Source Software this would be another (minor) reason for exeeding
the cookies limit ;-) I am pretty shure that they will appreciate this
modification as well.
Hello & thanks for dealing with this subject...

I seem to gather that the site in question (heise.de) *does* use cookies
'properly' and does *not* violate the RFC, while on the other hand Mozilla fails
to support the sites' cookie management just because it implements only the
RFC's absolute minimum requirements?

By the way, this is from the site's FAQ (translation from German by me): "While
the changes within Mozilla would be done by changing one line of code, the
necessary changes in cookie management within our server software would mean a
complete revision of the whole source code, for which we have no resources at
this time. That means, as a Mozilla user using our message boards, you will be
confronted with the described problem for the time being."
OS->All

Confirming based on number of confirming comments and the entry in the FAQ at
www.heise.de.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
*** Bug 228695 has been marked as a duplicate of this bug. ***
All current Mozilla versions have a limit of 300 cookies that can be stored. I
currently have over 250 cookies that I need to keep around and I've gotten to
rejecting almost all new cookies that come in so they don't overwrite the ones I
need. Given that almost every site these days want to write at least one and
many times several cookies, the old limit of 300 is way too low.
I have also begun to be bitten by this problem lately. I don't think I'm
visiting any sites that are misusing cookies, it's just that I have collected so
many different ones that the ones I really need to keep are getting overwritten.

For instance the cookies used to keep track of what messages I've read on
various forums I read regularly keep getting overwritten, almost on a daily
basis lately which is extremely annoying. Annoying to the point I have been
forced to switch back to IE for keeping up with some sites.

This extremely low limit on cookies is ridiculous and will only grow more so as
time goes on. Please at least let the users configure a higher number if they
desire.
If the ones you are using keep geting overwritten due to the 300 limit, that 
would suggest that your daily browsing involves over 300 unique cookies getting 
set in a day.  I would suggest that perhaps something else is to blame for the 
sites not working, but then again, it only takes 15 sites that set 20 cookies 
apiece, although that might not be the issue.
When you surf a lot of pages in one day (which many of us do) they all want to
set many cookies and every damn popup or online ad also wants to set some kind
of cookie to track users. Blowing past the arbitrary limit of 300 is almost
trivial. All we are asking for is a user preference to override the hardcoded limit.
I would be against adding a pref to raise the limit.
I would be against removing the limits. i don't want 10GB of cookies on my system.
I wouldn't have a problem with just raising it. Today's computers can handle it :)
Why are you against added a pref so the user can do what they want? Making
arbitrary decisions about what a user wants isn't going to make this the best
browser out there.
Requiring the user to find the pref, find out a working value set isn't good. A
user doesn't want to play with prefs to get his browser working.
Making it so that is just works(tm) is good.
I agree with mvl, we are not going to have a pref for this. If we need to raise
the limit, so be it. Adding a pref is both user unfriendly and pointless, for
such a backend detail as this.
Well the current 'solution' isn't working and the user can't fix it since there
is no way to change the limit.

So why do we have prefs for History size and cache size? Most people are never
going to change those either, but at least we give them the option.
I've already told you. read my comment again. if we need to raise the limit, we
can do that. ok?

history and cache are not the same as cookies, because they have more
flexibility wrt browser implementation. lack of history and/or cache (due to the
user switching them off or setting their size to low values) doesn't have the
potential to stop a site from working - an insufficient limit on cookies does,
as you have discovered.
But we already allow the user to block all cookies, which would have the same
effect as someone setting the cookie limit to zero. 

I guess I just see more user options as a good thing, but others obviously feel
differently.
I guess making the limits accessible from the outside doesn't necessarily mean
to make the data availlable through some preferences pane. It would be
sufficient if the limits could be edited for the 'advanced' user from the
outside. That's the approach Apple is using on Safari. However if you want to
keep this hardcoded why don't you increase the number of cookies to 1,000 as a
starter. I would be glad to feed back if this would solve my current problems.
Suggested limits:
max 50 cookies per domain.
max 3000 cookies in total.
Yes, this is still arbitrary, but at least somewhat higher.

Maybe we should #ifdef stiff for minimo, so it doesn't have a huge table in
memory. (thanks to mconnor for the suggestion)

dwitte, darin: thoughts on the numbers?
thought I'd poke my head in here

3000 cookies is probably excessive, given a figure dwitte gave of the 300 cookie
table taking ~30k, this is along the lines of 300k of memory if we hit this
maximum.  I would be against raising the limit beyond 1000 unless we also
implement some sort of auto-pruning for unused cookies (i.e. bug 202690)

just simply increasing memory footprint of a feature tenfold without a control
mechanism is bad :)
50/1000 sounds reasonable, i agree with mconnor's points about overall size.

the per-domain and total numbers aren't necessarily related though; i remember
seeing one complaint that the per-domain numbers were too low, but otherwise it
seems like most of the complaint is about the total number. so it may not be
necessary to increase the per-domain number as much...
dwitte: This  bug is about a site that needs > 20 cookies. You are ironic :)
My 3000 number is more like: we do 300 now, lets just multiply it by 10. But
actaully, i think the 300 isn't the problem for most users. 1000 will do fine.
I'll speak up again. Regarding my recent complaint, I examined all the cookies I
had, and determined I was running up against the 300 limit. I went through and
removed a bunch of ad-related ones and other unimportant ones in order to get it
back down around 200. No site had more than about 10... the average was around 3
or 4.

So... I don't think the 20 limit per site is so bad - the 300 max is what hit me.
raising the cookie limits sounds like a good idea to me.

preferences might make sense in the context of an application embedding gecko. 
consider minimo for example.  it wants cookie support, but maybe it wants to
reduce footprint by restricting the cookies limits as much as the spec allows. 
not having preferences might encourage someone to #ifdef the code.

i might not bother setting default values in all.js for these prefs.  having
them be hidden prefs is probably sufficient (i.e., they don't need to be listed
in about:config).

at some point we need to provide better documentation to embedders that explains
the set of prefs like this that can be configured.
darin: if there's prefs that aren't documented in
http://www.mozilla.org/projects/netlib/cookies/cookie-prefs.html please let me know.
For me, as the original bug reporter, the 20 cookies per site limit DOES hurt,
it still hurts. On the other side, I never have a problem with the overal limit.
I do not have a problem with that limit because Mozilla asks me to set a cookie
and I do never agree to a cookie which smells like adv or for sites google leads
me too. I am sure that limit is very easily reached when one allows cookies for
every site.

I would be happy with being able to set whatever different value in the
preferences file, no need to set/change it in running Mozilla.

Whatever value is hardcoded in the source, there will be people argueing for a
smaller value (memory) and people argueing for a larger value.
Am I wrong, or do both RFC 2109 and RFC 2965 (which supercedes 2109) read as
following:

"In general, user agents' cookie support should have no fixed limits.  They
should strive to store as many frequently-used cookies as possible."

(See section 6.3 in 2109, and section 5.3 in 2965.)

There *must* be a better way to deal with the potential of a large cookies.txt
file than to delete all but the 300 most recent cookies.  (Not to mention that
we're talking about a file that, at its current incarnation, is around 20-30K in
size, as compared to a browser that's anywhere from 10 to 27 Mb in size.)

And just to give a real-work example of how the 300 cookie limit impacts
someone, if I don't log into my online banking site at least once every four to
five days, I have to hunt down my ATM card so that I can retype in my card
number.  Likewise, if I don't log into my mutual fund tracking site every four
to five days, I have to grab an account statement to find my account number. 
Neither is reasonable, given that there could be a single, persistent line in my
cookies.txt file to keep those pieces of information for me!

(And the platform for this bug should be changed to All, so far as I know; it's
a problem on Mac and Unix, as well.)
most banking sites I've seen have 5-15 minute timeouts, 4-5 days is a little
crazy.  And are you saying they disable saving passwords, but persist cookies
for login sessions?  That's a crazy security model, given default browser
settings for major browsers.

Regarding the RFCs, short of imposing an arbitrary method for determining when
to expire cookies (i.e. unused cookies in x days), this basically would amount
to "the longer you go between clearing cookies, the more memory you're stuck with"

also, any other option based on "commonly-used" instead of "most recently set
300" would involve tracking frequency of access/updates, with all of the privacy
concerns that would invoke (i.e. you could see that query bugzilla dozens of
times a day, based on the BUGLIST cookie).
No, the banking site login is based on my bank card number as my login name, and
that number can be saved in a persistent cookie.  But if I see 300 cookies
between logins, then that cookie goes away, and I have to dig out my card again
in order to use their site.  And it was one example; there are tons of others,
all of which demonstrate users who choose an option to have a piece of
information stored in a persistent cookie, and all of which involve that piece
of information deleted based on a totally arbitrary, unmodifiable limit.

And we're seriously talking about the memory use of 300 vs. 1000 vs. 3000
cookies?  I can't imagine that it *touches* the memory use of, say, caret
browsing or stored form data.
going from 30k to 300k of memory used is the different between 300 and 3000
cookies.  That's a big deal on a PC with 32 MB of RAM.

in any case, it'll be a pref at some point
Just a data point -- on my machine, Firefox's memory footprint is 95 Mb (iBook,
OS X), so the idea of running it on a 32 Mb machine already seems to be moot.

And if the notion truly is that adequate cookie support is not worth the memory
it takes, then it's hard to see how Mozilla aims to be anything but fringe. 
Trying to sell users on a browser that will, *by design*, forget supposedly
persistent data most likely involves those users not knowing that the behavior
exists.
Status: NEW → ASSIGNED
Whiteboard: DUPEME
Comment on attachment 146737 [details] [diff] [review]
increase limits and add prefs

this ups the limits to 50/1000 resp, and adds hidden prefs for them.

(note that if a user decreases the limit, say from 1000 down to 300, this won't
have an immediate effect on the cookielist size since the cookieservice is
designed with a fixed list size in mind - the list will gradually pare down as
cookies are set. limit increases are no problem.)

i figured we don't need kMaxBytesPerCookie to be tunable - darin, any opinion?
Attachment #146737 - Flags: superreview?(darin)
Attachment #146737 - Flags: review?(mconnor)
actually, for limit decreases, the list size won't pare down at all, except by
cookie expirations. if we care, i can fix this somewhat shabbily...
Comment on attachment 146737 [details] [diff] [review]
increase limits and add prefs

r=me, although I think we still need to find a better way of expiring unused
cookies, but that's beyond the scope of this bug really
Attachment #146737 - Flags: review?(mconnor) → review+
Comment on attachment 146737 [details] [diff] [review]
increase limits and add prefs

>Index: netwerk/cookie/src/nsCookieService.cpp
>@@ -1226,23 +1240,23 @@ nsCookieService::AddInternal(nsCookie   
>-    if (CountCookiesFromHost(aCookie, data) >= kMaxCookiesPerHost) {
>+    if (CountCookiesFromHost(aCookie, data) >= mMaxCookiesPerHost) {

>-      if (mCookieCount >= kMaxNumberOfCookies) {
>+      if (mCookieCount >= mMaxNumberOfCookies) {

If make those two a |while|, won't that fix the reducing the limits problem?
Granted, it would only work the next time a cookie is set, but better then not
at all.
Comment on attachment 146737 [details] [diff] [review]
increase limits and add prefs

>   PRInt32 val;
...
>+  if (NS_SUCCEEDED(aPrefBranch->GetIntPref(kPrefMaxNumberOfCookies, &val)))
>+    mMaxNumberOfCookies = val;
>+
>+  if (NS_SUCCEEDED(aPrefBranch->GetIntPref(kPrefMaxCookiesPerHost, &val)))
>+    mMaxCookiesPerHost = val;

so the type of val is PRInt32, but the type of mMaxNumberOfCookies and
mMaxCookiePerHost is PRUint16.	should you range check val at all?
what if someone sets maxNumber to 999999 in an attempt to allow unlimited
number of cookies.  maybe the result will not be what they expected.

you could write:

  mMaxNumberOfCookies = (val >= 0 && val <= 0xFFFF) ? val : 0xFFFF;

or maybe that's overkill.  i use the CLAMP macro defined in nsHttp.h for 
this purpose in nsHttpHandler.cpp, which you might want to duplicate here.


otherwise, looks fine

sr=darin
Attachment #146737 - Flags: superreview?(darin) → superreview+
checked in. i added a variation on your CLAMP macro, and made all the pref
getters use it. thx!
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Can we port this to the aviary branch, please?
Whiteboard: needed-aviary1.0
this fix was checked in to trunk 2004-04-23, and the aviary branch date was
20040515, so by my reckoning... it already is? ;)
No, it's not yet on the aviary branch. The aviary branch branched from the
1.7-branch, which is still unpatched.

Trunk:
http://lxr.mozilla.org/seamonkey/source/netwerk/cookie/src/nsCookieService.cpp#88

Aviary branch (what would I do without this lxr module?):
http://lxr.mozilla.org/aviarybranch/source/netwerk/cookie/src/nsCookieService.cpp#83

Graph (this is rev. 1.25):
http://bonsai.mozilla.org/cvsgraph.cgi?file=mozilla/netwerk/cookie/src/nsCookieService.cpp
yup, right. dwitte must have been confused (or something worse) :)
mconnor, care to port & checkin? i guess this should apply without much problems.
So, I'm assuming by the graph that this also isn't part of Firefox 0.9?  That's
a bummer...
it'll go into 1.0beta
Flags: blocking-aviary1.0?
Comment on attachment 146737 [details] [diff] [review]
increase limits and add prefs

Requesting approval1.7.2.
This is a low-risk patch which fixes a major German IT news site.
Attachment #146737 - Flags: approval1.7.2?
Flags: blocking-aviary1.0RC1?
Comment on attachment 146737 [details] [diff] [review]
increase limits and add prefs

a=mkaply for 1.7.2
Attachment #146737 - Flags: approval1.7.2? → approval1.7.2+
i'll check this into the 1.7 branch and jump on mconnor for an aviary checkin.
fixed on aviary1.0 and 1.7 branches.
Whiteboard: needed-aviary1.0
Flags: blocking-aviary1.0RC1?
Flags: blocking-aviary1.0?
Just a final comment. heise.de (the site where this bug caused troubles) wants
to thank you for fixing this problem. If you can read read german pages, read
their statement here:

http://www.heise.de/foren/faq/go.shtml?forum_id=7262#mozilla

[[otherwise use babelfish or google or or or]]

Many thanks from me too!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: