Closed Bug 1264192 Opened 9 years ago Closed 8 years ago

Browsing Twitter wipes out its login cookie because of a per-host limit

Categories

(Core :: Networking: Cookies, defect)

Product:
Core
Component:
Networking: Cookies
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
platform-rel --- -
firefox48 + wontfix
firefox49 + wontfix
relnote-firefox --- 48+
firefox50 + fixed
firefox51 + fixed

People

(Reporter: Unfocused, Assigned: jdm)

References

Details

(Keywords: losing-users, Whiteboard: [platform-rel-Twitter][necko-active])

Attachments

(4 files, 4 obsolete files)

cookie nspr log
2.87 KB, text/plain
Details
Screenshot 2016-05-03 21.59.11.png
233.11 KB, image/png
Details
Interdiff
16.39 KB, patch
Details | Diff | Splinter Review
Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host
24.77 KB, patch
jdm
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
I've been getting logged out of Twitter after restarting Nightly. Seems there's been a bunch of people noticing this. It may or may not be related to bug 1239671 - same symptoms, but that's supposedly been fixed for 1.5 months (and nothing's changed in SessionCookies.jsm since then). I have Twitter in a pinned tab, running latest 64bit Nightly, on Win10. See some reports in bug 1239671 comment 20 and the following thread on Twitter https://twitter.com/dolske/status/717912689456971776. Confirmed reports dating back about week.
(One more data point: I haven't run into this at all, myself. I have Twitter in a pinned tab, running latest 64bit nightly on Linux, with e10s disabled. So, this seems to not be 100% reproducible, and might require some particular configuration. Or, maybe some particular piece of my configuration [e10s being disabled maybe?] is saving me.)
Some testing shows that my sessionstore.js has an enormous history for my pinned twitter tab, and that the session store contains a sizable number of twitter cookies. However, manually deleting the history entries & cookies has no effect on the bug.
I just got logged out without even restarting Firefox! All I did was use "history -> recently closed windows" to reopen a tweet. I had faved the tweet before closing the window and reopened it in order to reply.
[Tracking Requested - why for this release]: Awful regression but unclear whose fault it is
tracking-firefox48: --- → ?
I did some debugging after my tweet. It doesn't _always_ happen, but I'm not sure what the trigger is. There were a couple of twitter.com cookies with fairly short expiration times (< 1 hr from login time, iirc), but even waiting until after that and restarting the browser was fine.
Restarted after today's Nightly update, and was logged out. Restarted again, right after logging in, and was logged out. Restarted a 3rd time, right after logging in, and... was still logged in. But opening twitter.com in a new tab had me logged out, and returning to my pinned Twitter tab had a error message from Twitter saying "could not authenticate you" for a moment before the page refreshed to the login page.
At least at the moment, I see to be able to reliably reproduce this by restarting the browser.
(Quoting Marco Bonardo [::mak] from bug 1221480 comment #6) > If someone can reproduce "easily", he may try to get some logging, > NSPR_LOG_MODULES=cookie:5,mozStorage:5 (along with NSPR_LOG_FILE) may get > the whole story, but it could be quite verbose. Probably worth starting with > just cookie logging and then can be enlarged to storage if needed. See more details at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/NSPR_LOG_MODULES#Examples I'm not sure if that environment changed with MOZ_LOG or not but it will be obvious if it doesn't work.
Hmm, now I've been logged out of the firefox-dev Mailman admin interface on mail.mozilla.org a few times (this never used to happen). That might be easier to diagnose, since it's storing exactly 1 cookie. (Encrypted connections only, expires at end of session). I also don't have it pinned in a tab, or generally even kept open across restarts.
FWIW, I see this much of the time (a significant percentage of restarts, but maybe not all) on Windows (where I run nightly builds), and never see it on Linux (where I run self-built debug builds). I have twitter in a pinned tab on Windows, and have at times had it in a pinned tab on Linux (although not the past few weeks). (But on Linux I feel like I get randomly logged out of nytimes.com for about the last 6 months or so, although I think that happens on Windows too. It's hard to know what's intentional behavior by the site and what's a Firefox bug...)
(In reply to Justin Dolske [:Dolske] from comment #9) > Hmm, now I've been logged out of the firefox-dev Mailman admin interface on > mail.mozilla.org a few times (this never used to happen). > > That might be easier to diagnose, since it's storing exactly 1 cookie. > (Encrypted connections only, expires at end of session). I also don't have > it pinned in a tab, or generally even kept open across restarts. Yeah, among other things I've lost bugzilla sessions and 'remember this machine' sessions for my bank. I don't have either of those pinned as tabs.
Possibly related: Today the cookies for my HMO's website got 'stuck' somehow, and I couldn't purge them even using the Cookies Manager+ addon - they came back every time I restarted the browser. I ended up having to kill it and then manually edit the cookies sqlite database (where I found additional cookies for the website), and then delete them. So that's a little weird, because it couldn't have been session restore... I should see if this helps the next time I have a problem with twitter.
If I accidentally close the Firefox window containing my twitter tab, reopening it (with 'restore closed windows -> x') logs me out of twitter. Wild.
That's not necessarily surprising, they use the same mechanism. Can we get QA to look at this?
Flags: needinfo?(ryanvm)
Keywords: qawanted, regressionwindow-wanted
Hi Blair, I have tested your issue on latest FF release (46.0) and latest Nightly build and could not reproduce it. I have pinned my twitter tab and updated Firefox, but I was not logged out after the browser restarted. I haven't checked the remember me option and I haven't saved my credentials in the browser. If I restart Firefox from the console, I'm still logged in my twitter account. Closing Firefox from the X closing button at the top-right edge of the browser does not log me out from twitter after I reopen it. I've tested this on Windows 7 x32, Windows 10 x64 and Ubuntu 14.04 x64 and I haven't manage to reproduce this issue. Is this still reproducible on your end ? If yes, can you please retest this using latest FF release and latest Nightly build (https://nightly.mozilla.org/) and report back the results ? When doing this, please use a new clean Firefox profile, maybe even safe mode, to eliminate custom settings as a possible cause (https://goo.gl/PNe90E). Thanks, Paul.
Flags: needinfo?(bmcbride)
I can reproduce this every time by restarting my browser. I am going to attach some snippet of the cookie log that might be interesting. This is a huge file and I don't really want to spend time censoring everything.
Attached file cookie nspr log —
Maybe this is interesting, otherwise I guess we can try to debug this some other way.
(In reply to Paul Pasca[:PoollyMcklayn] from comment #15) > I have tested your issue on latest FF release (46.0) and latest Nightly > build and could not reproduce it. [...] > Is this still reproducible on your end? If yes, can you please retest this [...] Clearing this needinfo for Blair, as various people (including Firefox devs who run Nightly as primary browser) have already reported that this continues to be reproducible in recent Nightlies. (Like Paul, I've been unable to reproduce; but others have.)
Flags: needinfo?(bmcbride)
On the topic of '[Main Thread]: D/cookie Too many cookies for this domain' from Tom's log, is it relevant that twitter creates a truckload of cookies? According to Cookies Manager I have 146 cookies right now that were created by twitter.com. Most of them are the 'suggested' cookie, set for individual user pages.
The default max number per domain is 150, which does sound suspicious.
When I upped network.cookie.maxPerHost to 300 and restarted my browser twitter logged me back in for the first time in weeks.
(In reply to Daniel Holbert [:dholbert] from comment #18) > (In reply to Paul Pasca[:PoollyMcklayn] from comment #15) > > I have tested your issue on latest FF release (46.0) and latest Nightly > > build and could not reproduce it. > [...] > > Is this still reproducible on your end? If yes, can you please retest this [...] > > Clearing this needinfo for Blair, as various people (including Firefox devs > who run Nightly as primary browser) have already reported that this > continues to be reproducible in recent Nightlies. (Like Paul, I've been > unable to reproduce; but others have.) I agree with this, but what was written above should be taken into consideration by all those who can reproduce this issue. Maybe a new profile or safe mode can help narrow down this issue or point to a faulty add-on. Can anyone who is able to reproduce this perform a regression? Information on the tool is available at http://mozilla.github.io/mozregression/. Please don't hesitate to contact us if you encounter any problems.
Keywords: qawanted
Flags: needinfo?(ryanvm)
From looking at the code, it looks like what's supposed to happen is that nsCookieService::FindStaleCookie is supposed to evict either: (a) the first expired cookie it finds (there probably shouldn't be any for twitter), or (b) the cookie with the oldest LastAccessed time Some possible things that could have regressed include: (1) maintaining the LastAccessed time is somehow broken (e.g., maybe with e10s) (although I haven't figured out how this could explain being logged out only on session restore and not mid session) (2) the way sessionstore interacts with session cookies has changed (e.g., with e10s) (since I *think* the sessionstore holds session cookies bot not permanent cookies), and interferes with the LastAccessed time (3) the way sessionstore restoration of cookies works has always interfered with the LastAccessed time, but twitter has recently started setting more than 150 cookies (Note that these suggested cookies that twitter is setting per-user-page are session cookies, so the massive accumulation is only possible with long sessions or with sessions extended by things like session restore.)
Does this help? This is after I purged a bunch of 'suggested' cookies to make it fit into a single screenshot. You can see roughly how long FF has been open. Restarting in this state didn't log me out of Twitter, but I suspect once the cookies pile up again it will.
Tracking for 48 and 49 since this isn't a good user experience. Adding Marco to see if he can help us find an owner for this bug.
tracking-firefox48: ? → +
tracking-firefox49: --- → +
Flags: needinfo?(mak77)
It's unclear whether this is related to the Cookies Manager or Session Restore yet. comment 23 looks like a good starting point for an investigation, but I'm not sure who is comfortable with session restore and cookies code. The best person for SR is mconley, but I expect him being very busy with e10s atm. For cookies, I see the most active are ehsan, jdm and mcmanus. I wonder if dolske has any available resources in the quality team, I don't think I'm the best person to take a decision here.
Flags: needinfo?(mak77) → needinfo?(dolske)
(In reply to Josh Matthews [:jdm] from comment #21) > When I upped network.cookie.maxPerHost to 300 and restarted my browser > twitter logged me back in for the first time in weeks. This also "fixes" the problem for me -- I was able to reproduce the problem 3x in a row, then added this pref, and was not able to reproduce 3x in a row. Mike, maybe this would make for a good first dive into session store?
Flags: needinfo?(dolske) → needinfo?(mdeboer)
Alright, sounds like it. I don't think finding a regression window is the most efficient first step here, so I'm first going to find a solid way to reproduce and fix the problem directly.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
All of the extra cookies are 'suggested' and attached to users' profile URLs, so I think the most likely source of the cookies is either direct visits to profiles/tweets, or from the 'who to follow' boxes that appear while scrolling/refreshing the timeline. IIRC Twitter's crazy lightbox changes do history.pushState whenever you open a user's tweet, so my guess would be the cookies are coming from there.
FWIW, bumping 150 to 300 is only a temporary solution. I eventually hit 300 cookies too. I suspect it's probably easiest to reproduce by *reducing* the number of cookies per host, and then probably visiting lots of twitter user pages (or maybe tweets -- whatever causes the "suggested" cookies to get set).
Whiteboard: [platform-rel-Twitter]
platform-rel: --- → ?
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #23) > From looking at the code, it looks like what's supposed to happen is that > nsCookieService::FindStaleCookie is supposed to evict either: > (a) the first expired cookie it finds (there probably shouldn't be any for > twitter), or > (b) the cookie with the oldest LastAccessed time > > Some possible things that could have regressed include: > > (1) maintaining the LastAccessed time is somehow broken (e.g., maybe with > e10s) (although I haven't figured out how this could explain being logged > out only on session restore and not mid session) After spending a few hours investigating this and trying to reproduce, I found that it doesn't have anything to do with session store and this point would be my most likely candidate as being the culprit. (This makes me quite sad, because if it were a session store problem, I could've fixed it more easily.) In other words: after I set the network.cookie.maxPerHost pref to '10' (less than that would log me out again without even navigating, thus not reliable to use) and navigated around twitter.com for a while after having logged in, I'd be logged out again after a while. I did not have any other windows+tabs open at that time, using a freshly baked debug build. Since it happens during a browser session, I think it's safe to strike session store off the list. David, where should I start looking to attempt a fix here? I think you might be on to something when you hint at e10s being a suspect, but I've never dabbled in cookie service code before. I'm willing, though! If there someone else you'd like to suggest as a better dev or peer, please feel free to do so! Mike, I'm already flagging you here, because this story might sound familiar to you(?)
Flags: needinfo?(dbaron)
Flags: needinfo?(mconley)
For the record, I have e10s disabled on dev edition and this is still a problem for me. Doesn't mean e10s isn't related, I suppose.
One other thought: What limits do other browsers have on number of cookies? Do they have similar limits, or no such limits? If they have similar limits, does twitter have similar problems for them? (In reply to Mike de Boer [:mikedeboer] from comment #31) > David, where should I start looking to attempt a fix here? I think you might > be on to something when you hint at e10s being a suspect, but I've never > dabbled in cookie service code before. I'm willing, though! If there someone > else you'd like to suggest as a better dev or peer, please feel free to do > so! One thing that seems relevant would be dumping out information about the LastAccessed time for all of the cookies on twitter to see what's actually happening with the expiration algorithm. Beyond that, I think you're probably better off talking to somebody who actually knows the cookie code. Judging from the commit logs, :jdm seems like the most prominent reviewer lately.
Flags: needinfo?(dbaron)
Hey - I was needinfo'd, but I don't think I have much to add to this discussion except good vibes: nothing that's being described here sounds like anything I'm too involved with.
Flags: needinfo?(mconley)
Setting the 'network.cookie.maxPerHost' pref to 20 makes it impossible to reproduce, for me.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Component: Session Restore → Networking: Cookies
Product: Firefox → Core
Flags: needinfo?(josh)
(In reply to Mike de Boer [:mikedeboer] from comment #31) > In other words: after I set the network.cookie.maxPerHost pref to '10' (less > than that would log me out again without even navigating, thus not reliable > to use) and navigated around twitter.com for a while after having logged in, > I'd be logged out again after a while. This is super helpful. I can also reproduce this way in Nightly, as well as in Firefox 47 (non-E10S). I also went back and was able to reproduce it in Firefox 40 and Firefox 30... So it seems like whatever caused this to start happening recently is likely due to a change in Twitter. Could still very much still be our bug, of course, just one that's been latent until now. Just to detail those STR a little more: 0) New profile, set maxPerHost to 10 1) Log in to twitter 2) Click a tweet (to open up the card view), then press escape to close it 3) Repeat #2 about seven times, after which twitter will toss you to a login screen.
(In reply to Justin Dolske [:Dolske] from comment #36) > I can also reproduce this way in Nightly, as well as in Firefox 47 > (non-E10S). I also went back and was able to reproduce it in Firefox 40 and > Firefox 30... So it seems like whatever caused this to start happening > recently is likely due to a change in Twitter. Could still very much still > be our bug, of course, just one that's been latent until now. Oh, I hadn't thought of checking other Fx versions! /facepalm. So yeah, if someone would be able to dive into the cookie service a bit with this STR, we can determine whether the problem can be isolated to be in our domain or twitter.com. Josh, that's why I requested info from you; you might not have time right now to look at this, but perhaps know someone who does!
Assignee: nobody → josh
Summary: Logged out of Twitter after restarting Nightly → Browsing Twitter wipes out its logging cookie because of a per-host limit
Whiteboard: [platform-rel-Twitter] → [platform-rel-Twitter][necko-active]
No longer depends on: 1239671
150 is a lot of cookies (especially considering they get pushed over the wire with every request), but because sites do sometimes hit it there's a proposal to let sites tag some of their cookies as "high priority" so they are less likely to be evicted. There's no guarantee this proposal will get standardized, built into browsers, or adopted by sites, but it could help in situations like this if the log-in cookie were tagged as high-priority. https://tools.ietf.org/html/draft-west-cookie-priority-00
(In reply to Daniel Veditz [:dveditz] from comment #38) > 150 is a lot of cookies (especially considering they get pushed over the > wire with every request) Hmm, well, I only see a small amount of cookie data being sent... Looking my Twitter cookies closer, they (mostly?) all have a Path set for various users... EG /user/conversation, /user2/status, (and sometimes the same thing but "/i/user3/conversation"). So most of the cookies would be rarely sent, but somehow they're still causing the main login cookie (on "/", it looks like) to get purged.
Summary: Browsing Twitter wipes out its logging cookie because of a per-host limit → Browsing Twitter wipes out its login cookie because of a per-host limit
I would like to suggest this bug gets the losing-users keyword. I think constantly being logged out of a service is a key reason for people to start trying out other browsers.
Keywords: losing-users
FWIW I have a similar issue on a website on my Firefox OS March build for a website that has (surprise surprise) 149 cookies currently.
I've set my network.cookie.maxPerHost to 30 in a new profile and can't reproduce the Twitter problem. I don't see any cookies for suggested users, which was a plausible cause from earlier in the bug, so maybe Twitter has changed in ways that don't cause this to reproduce any longer.
Flags: needinfo?(josh)
For the record, the cookie eviction strategy we implement right now is as follows: * if we would exceed the maximum number of cookies for the new cookie's host, evict an expired cookie for the same host, or the oldest cookie based on when it was last set * otherwise, if we would exceed the maximum number of cookies for the entire database, and the oldest cookie was set 30 days before, remove all expired cookies and cookies that are older than 30 days, then remove the oldest cookies until the maximum limit is reached This makes me wonder if we could avoid this problem by touching cookies' access times when they are sent as well. This would mean that if the host limit is exceeded, cookies with less common path values would be evicted first.
Looking more closely, we do appear to update the last access time when retrieving cookie values as well, so there's no clear solution here unless we can identify a case in which incorrect cookies are selected for eviction.
I got report on IRC that STRs described in comment 36 do work on a 48b9 firefox.
status-firefox48: --- → affected
(In reply to Justin Dolske [:Dolske] from comment #39) > (In reply to Daniel Veditz [:dveditz] from comment #38) > > 150 is a lot of cookies (especially considering they get pushed over the > > wire with every request) > > Hmm, well, I only see a small amount of cookie data being sent... > > Looking my Twitter cookies closer, they (mostly?) all have a Path set for > various users... EG /user/conversation, /user2/status, (and sometimes the > same thing but "/i/user3/conversation"). So most of the cookies would be > rarely sent, but somehow they're still causing the main login cookie (on > "/", it looks like) to get purged. I'd say this is the root of the problem. I see such cookies for lemonde.fr too, especially when I access their live feed which I did a lot for 1 year. But I also see a lot of cookies on subdomains; then "baseDomain" is the same, but not "host". Cutrently I have 161 cookies on this baseDomain (I set the pref to have 300 as a max value instead of 150 -- before this I had 149 cookies exactly). Of these 161 cookies: * 30 are for .lemonde.fr (so are sent for every subdomain). All of them have "/" as path. * 3 are for abonnes.lemonde.fr or .abonnes.lemonde.fr, the main domain for subscribed visitors. All of them have "/" as path. * 10 are for abonnes.mobile.lemonde.fr, main domain for subscribed visitors on mobile. - 7 have path = '/' - 3 have 3 different paths * All others are for various subdomains, most of them with specific paths (the articles I visited on these subdomains), some with '/'. Most of the cookies are the couple (wptouch-device-type, wptouch-device-orientation) for each article, looks like it's coming from a wordpress plugin wptouch. Please tell me if you'd like this cookes sqlite db -- I'd expunge it from some personal data and attach it. It seems to me that these cookies are counted when looking for the current count of cookies for this domain, but they're not considered for removal when we want to remove some in case we have too many. Note that the pref is "network.cookie.maxPerHost" but it looks like it's really "maxPerBaseDomain". Maybe this is a source of confusion in the code.
To be really clear: it looks like some session cookies from .lemonde.fr are removed whereas some subdomain cookies, accessed a lot earlier before, should be removed instead.
That might also explain why I have so many problems placing orders on train tickets on voyages-sncf.com, often loosing cookies-related informations or session. It is not just painful, it just made me miss the last place on a train after a plane.
platform-rel: ? → -
Added to the release notes as a know issue with this wording: "On some websites using an important number of cookies, under certain conditions, this can cause the user to be logged out (1264192)"
relnote-firefox: --- → 48+
(In reply to David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) from comment #33) > One other thought: > > What limits do other browsers have on number of cookies? Do they have > similar limits, or no such limits? If they have similar limits, does > twitter have similar problems for them? Turns out there's a rather useful tool for testing that: http://browsercookielimits.squawky.net/ Chrome 52: 180 Firefox 48: 150 IE11: 50 Edge 14: 50 So, if anything, IE/Edge should be running into cookie limits first. If this can't be reproduced there, I wonder what they do differently.
Another thought as to why Fx might behave differently: perhaps other browsers implement something like a priority queue and we don't? This is purely hypothetical, because I don't know the cookiestore code: if we were to treat TLD name cookies (thus _not_ scoped to a subdomain) with higher prio than others, they'd be kicked out of the bucket later than others when we hit the maxPerHost limit. Open question here of course is: are we already doing this? And if not, would it solve this issue and make it worth considering?
Flags: needinfo?(josh)
I made a cookie log for my issue with lemonde.fr that I can send to somebody if necessary. There is some private information so I don't want to attach to this bug.
(In reply to Josh Matthews [:jdm] from comment #43) > I've set my network.cookie.maxPerHost to 30 in a new profile and can't > reproduce the Twitter problem. I don't see any cookies for suggested users, > which was a plausible cause from earlier in the bug, so maybe Twitter has > changed in ways that don't cause this to reproduce any longer. I can still reproduce the problem in Nightly with the steps in comment 36. For each tweet from a user I click on, I get a twitter.com cookie with name "suggested" and path "/user/status". (where "user" is the twitter username). With 30 maxCookies you'd need to click on a lot of tweets, hence the suggestion of ~10. I don't see the cookies gathering up in IE11. (At least, I found a file with "twitter.com" in shell:cookies\low in explorer, but it's not really growing or containing "suggested" cookies). I wonder if that's because these per-user cookies are marked as session cookies... Do we not preferentially expire session cookies before non-session cookies? The login cookies are obviously not session cookies.
I also don't see these cookies being set in Chrome, via their devtools. So, not clear if Twitter is setting cookies differently in Firefox, or those browsers handle them differently. But this bug kinda makes it our problem. :/
We should step through the cookie code in a debugger to see what's going on here. > Looking more closely, we do appear to update the last access time when retrieving > cookie values as well. It's not clear to me that we update the access times often enough: we have a note in the code that says "we only update the access time if the timestamp is stale by a certain amount, to avoid thrashing the db during pageload." http://searchfox.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#3170 I'm wondering if we're not updating the main login cookie often enough--if not we could wind up evicting it as the "least recently used" cookie even though it gets used all the time? It's just a guess, but worth investigating. But I suspect the real answer here is to fire up a debugger and see what's going on. If the access timestamp is indeed the issue, I suspect there must be some way we could update the in-memory hashtable without updating the SQLite database each time? We could keep a list of cookies that need access time updated in the DB and only update it every N minutes, or we could only update a cookie's access time in the DB every 5 accesses, or something. (and in the end, even if the browser crashes and we lose an access time update, it's hardly the end of the world). My suggestion here is for someone to change the per-host pref to something easily debuggable (say 20 cookies per host), then run Twitter in the debugger and figure out what's going on. Maybe a function that dumps all cookies for the host (along with access times) into the web console (or STDOUT) would make that easier.
(In reply to Justin Dolske [:Dolske] from comment #55) > I also don't see these cookies being set in Chrome, via their devtools. > > So, not clear if Twitter is setting cookies differently in Firefox, or those > browsers handle them differently. But this bug kinda makes it our problem. :/ If info from Twitter would be helpful, please use our Mailing List with them. Contact myself for details and access if required.
Hi William, I tried to reproduce it by comment 36 with Firefox 47, but not succeeded. Could you help to see if you can reproduce it?
Flags: needinfo?(whsu)
(In reply to Shian-Yow Wu [:swu] from comment #58) > Hi William, I tried to reproduce it by comment 36 with Firefox 47, but not > succeeded. Could you help to see if you can reproduce it? Hi, Shian-Yow, I can reproduce this bug by using steps in comment 36, but it's not easy for me to reproduce(Reproduction rate: 20%~30%). I'll find you and talk this issue with you later. Thanks!
Flags: needinfo?(whsu)
(In reply to Justin Dolske [:Dolske] from comment #54) > (In reply to Josh Matthews [:jdm] from comment #43) > > I've set my network.cookie.maxPerHost to 30 in a new profile and can't > > reproduce the Twitter problem. I don't see any cookies for suggested users, > > which was a plausible cause from earlier in the bug, so maybe Twitter has > > changed in ways that don't cause this to reproduce any longer. > > I can still reproduce the problem in Nightly with the steps in comment 36. > > For each tweet from a user I click on, I get a twitter.com cookie with name > "suggested" and path "/user/status". (where "user" is the twitter username). > With 30 maxCookies you'd need to click on a lot of tweets, hence the > suggestion of ~10. > > I don't see the cookies gathering up in IE11. (At least, I found a file with > "twitter.com" in shell:cookies\low in explorer, but it's not really growing > or containing "suggested" cookies). I wonder if that's because these > per-user cookies are marked as session cookies... Do we not preferentially > expire session cookies before non-session cookies? The login cookies are > obviously not session cookies. I don't understand why I do not receive those cookies when clicking on specific tweets. They simply do not appear in my list of cookies, regardless of the limits I set :<
Flags: needinfo?(josh)
Interesting, I get those cookies when I use a different twitter account...
I have it for only one user, but I can't get to get new ones...
I have a patch which prioritizes session cookies with a different paths, the session cookies with matching paths, the non-session cookies with different paths, then non-session cookies with matching paths. I'm going to add some tests for subdomains too after reading comment 47 again.
Hey Josh, when you feel it, please attach it, I'd be happy to test it on my use case. Thanks !
Still only preferring session cookies; no checks for subdomains.
The eviction heuristics now incorporate domain and path matching against the request that triggered the eviction.
Attachment #8778428 - Flags: review?(ehsan)
Attachment #8778216 - Attachment is obsolete: true
Comment on attachment 8778428 [details] [diff] [review] Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host Review of attachment 8778428 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cookie/nsCookieService.cpp @@ +4441,5 @@ > + // Prefer to evict the oldest session cookies with a non-matching path/domain, > + // followed by the oldest session cookie with a matching path/domain, > + // followed by the oldest non-session cookie with a non-matching path/domain, > + // resorting to the oldest non-session cookie with a matching path/domain. > + if (oldestSessionCookieWithPath.entry) { I think there is something wrong with this patch. I can't find where these WithPath variables are declared. Sorry but I'll r-, since this is complicated enough and I'm not sure if I'm looking at the right patch.
Attachment #8778428 - Flags: review?(ehsan) → review-
I uploaded a version after renaming some variables and forgot to build it first. My mistake.
This revision compiles.
Attachment #8779863 - Flags: review?(ehsan)
Attachment #8778428 - Attachment is obsolete: true
(In reply to William Hsu [:whsu] from comment #59) > (In reply to Shian-Yow Wu [:swu] from comment #58) > > Hi William, I tried to reproduce it by comment 36 with Firefox 47, but not > > succeeded. Could you help to see if you can reproduce it? > > Hi, Shian-Yow, > > I can reproduce this bug by using steps in comment 36, but it's not easy for > me to reproduce(Reproduction rate: 20%~30%). I'll find you and talk this > issue with you later. Thanks! Update my test result here. Thanks to comment 31 and comment 36. Now, it's easy for me to reproduce this bug by using a new profile. Hope the clue is helpful. Thanks! 1. Install new Firefox nightly build or create a new profile (./firefox -P -no-remote) 2. Type "about:config" in URL bar to go to "Configuration Editor" 3. Set "network.cookie.maxPerHost" to 10 (Integer) 4. Go to twitter.com and log in service 5. Click a tweet or a hashtag to open it, and then press ESC key or GO_BACK button to exit it 6. Repeat #5 about several times # Expected Result - User can smoothly use twitter without seeing the twitter login screen # Actual Result - Twitter will toss you to a login screen # Attachment - Demo Video: ~ Ubuntu: https://youtu.be/L8xCU2taHTo ~ Windows: https://youtu.be/j4AbDjjcwGI - Profile: https://goo.gl/9tpSdy
Comment on attachment 8779863 [details] [diff] [review] Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host Review of attachment 8779863 [details] [diff] [review]: ----------------------------------------------------------------- Looks great overall, but it has a couple of bugs, I think. Also, I'd like to see moar tests! ::: netwerk/cookie/nsCookie.h @@ +62,5 @@ > , mIsHttpOnly(aIsHttpOnly) > , mOriginAttributes(aOriginAttributes) > { > + // Defaults to 60s > + mCookieStaleThreshold = mozilla::Preferences::GetInt("network.cookie.staleThreshold", 60); Nit: please initialize in the initializer list. ::: netwerk/cookie/nsCookieService.cpp @@ +4359,5 @@ > nsListIter &aIter) > { > + bool requireHostMatch; > + nsAutoCString baseDomain, sourceHost, sourcePath; > + GetBaseDomain(aSource, baseDomain, requireHostMatch); GetBaseDomain() doesn't null check its first argument, so this will crash where AddInternal() has been passed nullptr. @@ +4409,5 @@ > + bool pathMatches = sourcePath.IsEmpty() || > + StringBeginsWith(sourcePath, Substring(cookie->Path(), 0, cookiePathLen)); > + bool domainMatches = sourceHost.IsEmpty() || > + cookie->RawHost() == sourceHost || > + (cookie->IsDomain() && StringEndsWith(sourceHost, cookie->Host())); GetBaseDomain() sets requireHostMatch to true in case the host is an IP address or something like localhost, and that argument is supposed to tell the caller to _not_ do string comparisons against the host. I think this causes actual bugs if you have a cookie set for foo.localdomain and you're trying to match something from bar.localdomain, for example. Please add some tests for this. @@ +4410,5 @@ > + StringBeginsWith(sourcePath, Substring(cookie->Path(), 0, cookiePathLen)); > + bool domainMatches = sourceHost.IsEmpty() || > + cookie->RawHost() == sourceHost || > + (cookie->IsDomain() && StringEndsWith(sourceHost, cookie->Host())); > + bool isPrimaryEvictionCandidate = !pathMatches || !domainMatches; Hmm, I think you mean something like this? bool isPrimaryEvictionCandidate = !aSource || !(pathMatches || domainMatches); Furthermore, I think the pathMatches and domainMatches variables are misleading, since they will both be *true* if aSource is null. I would expect them to be initialized to false. Although since they aren't accessed after this line, perhaps you can first initialize isPrimaryEvictionCandidate to false, and then do these three statements in a |if (aSourceURI)| block? Can you please add a test for the case where the path matches but the domain doesn't and vice versa? I think that should trigger an incorrect eviction with your patch. ::: netwerk/cookie/test/unit/test_eviction.js @@ +5,5 @@ > +const BASE_URI = Services.io.newURI("http://example.org/", null, null); > +const SUBDOMAIN_URI = Services.io.newURI("http://www.example.org/", null, null); > +const OTHER_SUBDOMAIN_URI = Services.io.newURI("http://pub.example.org/", null, null); > +const FOO_PATH = Services.io.newURI("http://example.org/foo", null, null); > +const BAR_PATH = Services.io.newURI("http://example.org/bar", null, null); You need tests with IP address hostnames, localhost, and something.co.uk to test the eTLD+1 correctness. @@ +104,5 @@ > + > + // At this point all remaining cookies are non-session cookies, have a path of /, > + // and either don't have a domain or have one that matches subdomains. > + // They will therefore be evicted from oldest to newest if all new cookies added share > + // share similar characteristics. Nit: s/share share/share/
Attachment #8779863 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari from comment #72) > @@ +4410,5 @@ > > + StringBeginsWith(sourcePath, Substring(cookie->Path(), 0, cookiePathLen)); > > + bool domainMatches = sourceHost.IsEmpty() || > > + cookie->RawHost() == sourceHost || > > + (cookie->IsDomain() && StringEndsWith(sourceHost, cookie->Host())); > > + bool isPrimaryEvictionCandidate = !pathMatches || !domainMatches; > > Hmm, I think you mean something like this? > > bool isPrimaryEvictionCandidate = !aSource || !(pathMatches || > domainMatches); > > Can you please add a test for the case where the path matches but the domain > doesn't and vice versa? I think that should trigger an incorrect eviction > with your patch. I don't think your suggestion here is correct - if a cookie is encountered where either the path matches or the domain matches, but not both, it is a candidate for eviction because it is not associated with the original request. With your change, this would no longer be the case.
(In reply to Sylvestre Ledru [:sylvestre] from comment #50) Nit: As a native speaker of English, I find the release notes text in comment 50 to be a bit confusing. The word "important" is a classic one of the English/French partial "faux amis". A better English word in this context is simply "large", so I suggest: "On some websites using a large number of cookies, under certain conditions this can cause the user to be logged out (1264192)"
Attached patch Interdiff — — Splinter Review
Review comments addressed.
Attachment #8779863 - Attachment is obsolete: true
Attachment #8785469 - Flags: review?(ehsan)
Comment on attachment 8785469 [details] [diff] [review] Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host Review of attachment 8785469 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #8785469 - Flags: review?(ehsan) → review+
None of the failures on that try push look related.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ecd402d3934 Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host. r=ehsan
Keywords: checkin-needed
Josh, if you think this is safe for uplift, we could potentially still take this for the 49 RC. We would not have much time for testing but it sounds like we may want to give this a try.
Flags: needinfo?(josh)
We should also add this to release notes for 49 if we fix it there. Gerry can you do that on Monday or Tuesday if this lands and sticks? Thanks!
Flags: needinfo?(gchang)
https://hg.mozilla.org/mozilla-central/rev/2ecd402d3934
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Backed out for frequent xpcshell test_eviction.js failure: https://hg.mozilla.org/integration/mozilla-inbound/rev/9855046ffeb7c945f572de0c51e82cf3c9b30dfc First failure (in push following the push with this patch): https://treeherder.mozilla.org/logviewer.html#?job_id=35230332&repo=mozilla-inbound Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=35230332&repo=mozilla-inbound 09:48:02 INFO - TEST-PASS | netwerk/cookie/test/unit/test_eviction.js | verifyCookies - [verifyCookies : 192] 5 == 5 09:48:02 INFO - TEST-PASS | netwerk/cookie/test/unit/test_eviction.js | verifyCookies - [verifyCookies : 217] 5 == 5 09:48:02 INFO - TEST-PASS | netwerk/cookie/test/unit/test_eviction.js | verifyCookies - [verifyCookies : 226] "non_session_non_path_non_domain" == "non_session_non_path_non_domain" 09:48:02 INFO - TEST-PASS | netwerk/cookie/test/unit/test_eviction.js | verifyCookies - [verifyCookies : 227] false == false 09:48:02 WARNING - TEST-UNEXPECTED-FAIL | netwerk/cookie/test/unit/test_eviction.js | verifyCookies - [verifyCookies : 226] "session_foo_path" == "non_session_non_path_subdomain" 09:48:02 INFO - C:/slave/test/build/tests/xpcshell/tests/netwerk/cookie/test/unit/test_eviction.js:verifyCookies:226 09:48:02 INFO - C:/slave/test/build/tests/xpcshell/tests/netwerk/cookie/test/unit/test_eviction.js:test_basic_eviction:113 09:48:02 INFO - C:/slave/test/build/tests/xpcshell/tests/netwerk/cookie/test/unit/test_eviction.js:run_test:19 09:48:02 INFO - C:\slave\test\build\tests\xpcshell\head.js:_execute_test:535 09:48:02 INFO - -e:null:1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox49: affected → wontfix
status-firefox51: fixed → affected
Target Milestone: mozilla51 → ---
Flags: needinfo?(gchang)
NI myself for release notes.
Flags: needinfo?(gchang)
Fixed the frequent intermittent failures on Windows XP by introducing delays between setting cookies in the tests.
Attachment #8789850 - Flags: review?(ehsan)
Attachment #8785469 - Attachment is obsolete: true
Flags: needinfo?(josh)
We might be able to take this for 50 but I think it is too risky and late for 49, although this sounds annoying for XP users. If we started getting a lot of complaints or duplicates we could potentially reconsider for a dot release.
If folks could test this with an m-c build that will be useful. But I think we would also want some extra manual/exploratory testing if we go for a (very) late uplift.
Agreed. jdm, ehsan can you weigh-in on risk in the bug here? 49 is in RC mode.
Flags: needinfo?(josh)
Flags: needinfo?(ehsan)
Comment on attachment 8789850 [details] [diff] [review] Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host From ehsan in person: "That's nice."
Flags: needinfo?(josh)
Attachment #8789850 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/43a5351815ee79485d60cf1dc2d65f73a52fab4a Bug 1264192 - Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host. r=ehsan
Josh, even with your smarter cookie eviction strategy, isn't Firefox's 150 cookie limit per host still an arbitrary incompatibility with Chrome? Why not increase our limit to match? For comparison, the http://browsercookielimits.squawky.net/ cookie test says Safari has no (?!) limit per host. git blame of Chrome's 180 cookie limit points to a 2010 bug that, ironically, purport to "Update cookie expiry to match Firefox policy/numbers"! :) https://crbug.com/8850 https://chromium.googlesource.com/chromium/src/+/7a964a7f1edad8c8b9ef4391073b10cd84821485%5E%21/ https://chromium.googlesource.com/chromium/src/+/8807b329539d919dd9250ffe1903523dc072225f%5E%21/
Flags: needinfo?(josh)
https://hg.mozilla.org/mozilla-central/rev/43a5351815ee
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8789850 [details] [diff] [review] Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host Approval Request Comment [Feature/regressing bug #]: Twitter plays fast and loose with cookies [User impact if declined]: Twitter users frequently get signed out of their sessions after restarting [Describe test coverage new/current, TreeHerder]: New test verifies expected properties of new eviction heuristics [Risks and why]: Could change behaviour of other sites that set >150 cookies, but it's hard to imagine the result being worse than the current behaviour. [String/UUID change made/needed]: None.
Attachment #8789850 - Flags: approval-mozilla-aurora?
(In reply to Chris Peterson [:cpeterson] from comment #94) > Josh, even with your smarter cookie eviction strategy, isn't Firefox's 150 > cookie limit per host still an arbitrary incompatibility with Chrome? Why > not increase our limit to match? Given the current architecture of the cookie service, this could be a resource problem. If the extra consumption is acceptable, bumping the total cookie limit at the same time may mitigate side-effects on other sites. Details: nsCookieService keeps all cookies in memory all the time. There's a prefed-but-unset hard limit "network.cookie.maxNumber"=3000 on the number of cookies. The limit is enforced when there are 10% more than that many cookies stored AND it's been prefed-but-unset "network.cookie.purgeAge"=30days (pref unit in seconds) + 10% seconds since the oldest cookie by access time. (Because of how the cookieOldestTime is tracked, this is actually more like OLDEST(oldest cookie at firefox startup, oldest cookie currently known).) The current per-base-domain limit "network.cookie.maxPerHost"[misnomer]=150 at kMaxBytesPerCookie=4096 (no pref) means a max permanent in-memory contribution of 150*4=600KiB per base domain. Raising that to 180 means that a cookie-happy origin like Twitter could take up to another 120KiB of memory from every Firefox user using it even when they're not browsing twitter. In practice, most sites are unlikely to max out their cookie size unless they're using it as a tricky alternative to local storage, but the worst case scenario stands. There is an about:memory reporter that logs at "explicit/cookie-service" (check the "verbose" box to be able to easily ctrl-f it) that can help give a feel for your profile. Of the 12+MiB worst-case, my main profile is at ~2.2MiB, and that's with "SELECT COUNT(*) from moz_cookies" returning 6002 on my profile's cookies.sqlite, perhaps giving an idea of just how much growth there can be before the purge fires. Changing the cookie service to work more like the local storage implementation so that cookies are only loaded from disk as needed still probably would not want an increased limit since more cookies means more disk I/O that needs to happen before we have the cookies in memory. (It's also the case that the hit on local storage is less bad since it doesn't delay network requests, just JS code execution trying to read the local storage. Asynchronously loading cookies would delay the network requests and gets worse with 3rd-party cookies that aren't already in memory.)
Just a side note, I have been suffering for this for a long time. Today, nightly shipped with this. And I am happy to say my session was not lost on restart of the Browser :) Thanks for fiing this!
Clear myself as this won't be included in 49 at this moment.
Flags: needinfo?(gchang)
Comment on attachment 8789850 [details] [diff] [review] Adjust cookie eviction heuristics when exceeding the maximum cookies allowed per host Let's take it in aurora! Thanks for this work.
Attachment #8789850 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(ehsan)
Depends on: 1302824
(In reply to Chris Peterson [:cpeterson] from comment #94) > Josh, even with your smarter cookie eviction strategy, isn't Firefox's 150 > cookie limit per host still an arbitrary incompatibility with Chrome? Why > not increase our limit to match? > > For comparison, the http://browsercookielimits.squawky.net/ cookie test says > Safari has no (?!) limit per host. > > git blame of Chrome's 180 cookie limit points to a 2010 bug that, > ironically, purport to "Update cookie expiry to match Firefox > policy/numbers"! :) > > https://crbug.com/8850 > https://chromium.googlesource.com/chromium/src/+/ > 7a964a7f1edad8c8b9ef4391073b10cd84821485%5E%21/ > https://chromium.googlesource.com/chromium/src/+/ > 8807b329539d919dd9250ffe1903523dc072225f%5E%21/ Yes, although technically the limit is closer to 165 because we don't start evicting cookies until we're more than 10% over the stated maximum. Probably worth discussing whether to raise it in another bug, however.
Flags: needinfo?(josh)
(In reply to Chris Peterson [:cpeterson] from comment #94) > Josh, even with your smarter cookie eviction strategy, isn't Firefox's 150 > cookie limit per host still an arbitrary incompatibility with Chrome? Why > not increase our limit to match? > > For comparison, the http://browsercookielimits.squawky.net/ cookie test says > Safari has no (?!) limit per host. > > git blame of Chrome's 180 cookie limit points to a 2010 bug that, > ironically, purport to "Update cookie expiry to match Firefox > policy/numbers"! :) > > https://crbug.com/8850 > https://chromium.googlesource.com/chromium/src/+/7a964a7f1edad8c8b9ef4391073b10cd84821485%5E%21/ > https://chromium.googlesource.com/chromium/src/+/8807b329539d919dd9250ffe1903523dc072225f%5E%21/ According to the comment in cookie_monster.h, Chrome's behaviour seems about the same as Firefox's: "When the number of cookies gets to k{Domain,}MaxCookies purge down to k{Domain,}MaxCookies - k{Domain,}PurgeCookies.... So, e.g., the maximum number of cookies allowed for google.com and all of its subdomains will be 150-180." Chrome doesn't start evicting cookies until at 180 and then purges down to 150, which is the same number as Firefox. Firefox starts evicting at ~165 and then purges down to 150.
Depends on: 1317250
Since we noted this in 48 as a known issue and fixed it in 50, I added a release note to state that this was fixed in 50.
Depends on: 1319403
After updating our FF version to 50.0, we have been suffering from session lost issue. Earlier i.e. before updating to 50, it was working fine. It seem after fixing this bug (related to cookies) in version 50, some other problem is started occurring. Please can you suggest. Also please note that its happening randomly. Thanks Nimish Saxena
My web app has also been suffering from this cookie change. Users will be randomly kicked out of our app. Clearing all cookies temporarily helps them.
(Looks like comment 106 and comment 107 are instances of bug 1319403.)
Does this problem still happen in the current Nightly build? Are we still able to replicate it in 51 (or 52 if we're looking after the release next week)? I'm approving the work from bug 1319403 for uplift to 53.
Flags: needinfo?(andrei.vaida)
I could not reproduce this issue following steps from the description, comment 36 and 71. I've set my network.cookie.maxPerHost to 10 in a new profile and can't reproduce the Twitter problem. Am I misssing something?
Flags: needinfo?(andrei.vaida) → needinfo?(josh)
That sounds like the best possible outcome, in that case. It means that undoing part of the fix for this bug in bug 1319403 did not cause the original problem to reappear.
Flags: needinfo?(josh)
We are also seeing similar issue with Firefox 52.0 32 bit version. Our application is not able to set cookie after cookie count for domain including subdomain reaches to 150. few domain specific set-cookie requests are able to evict old cookie and set new cookie but some domains set-cookie instruction is completed ignored. One pattern is observed if there is 150 persistent cookie for domain including sub domain then these persistent cookies can't be replaced by non-persistent set-cookie request. Those cookies are only replaced by persistent [with max-age] set-cookie request. Also secure session/persistent cookies are not replaced by session cookies handler. These changes introduce inconsistent behavior to users and also not well documented about what is exactly being changed. Can we get firefox developer attention on this. This is causing lot of trouble organization wide SSO login.
Can you try out a Firefox 53 Beta and see if it works better? A fix landed in bug 1319403 that might help.
I tried with FF 53 beta build.[https://ftp.mozilla.org/pub/firefox/releases/53.0b10/win32/en-US/] I set 150 persistent domain based cookies [.abc.com] now i tried to access another url which sets another sub-domain [xyz.abc.com] session cookie, it fails with beta patch also, however domain based session cookie is set now. Another observation is first set "session secure cookies". Those cookies are only replaced by subsequent set "secure cookie" request only. No other setcookie request is being honored. I think only change with this patch is domain specific persistent cookies are allowed to be replaced by domain specific session/persistent cookie.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: