Closed
Bug 1319403
Opened 7 years ago
Closed 7 years ago
deleting session cookies
Categories
(Core :: Networking: Cookies, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: rkarnia, Assigned: amchung, NeedInfo)
References
Details
(Keywords: regression, Whiteboard: [necko-active])
Attachments
(2 files, 1 obsolete file)
5.01 KB,
patch
|
jdm
:
review-
|
Details | Diff | Splinter Review |
11.20 KB,
patch
|
amchung
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36 Steps to reproduce: I work in interia.pl. We have a lot of services like fakty.interia.pl, sport.interia.pl, motoryzacja.interia.pl ... In all of them we create user cookies and we achieve the limit 165. Actual results: When user achieve limit of cookies browser does not allow create session cookie and our users can't login to services. It's connected to https://bugzilla.mozilla.org/show_bug.cgi?id=1264192 Expected results: You should check this limit on current cookie domain - not on main domain. If user achieves limit you should delete oldest normal cookie because session cookie is important to realization typical access process.
Comment 1•7 years ago
|
||
Same here. Radnom logout from my site. PHPSESSID cookie is evicted
Comment 2•7 years ago
|
||
Related with bug 1047231 ?
Comment 3•7 years ago
|
||
Bug 1047231 is only related if zattoo.com stores more than 165 cookies.
Updated•7 years ago
|
Whiteboard: [necko-next]
Comment 5•7 years ago
|
||
Same here. We manage a large blog platform with control panel domain shared with users blog domain and people using Firefox randomly cannot login since session cookie are discarded.
Comment 6•7 years ago
|
||
Its look like a subdomain cookies problem. No problems before FF50
Assignee | ||
Comment 7•7 years ago
|
||
I cant reproduce form https://bugzilla.mozilla.org/show_bug.cgi?id=1047231#c9 at first time. I already asked user to confirm the number of cookies from zattoo.com.
Flags: needinfo?(amchung)
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 8•7 years ago
|
||
This issue might also be affecting Wikipedia and other Wikimedia wikis. (Our bug report: https://phabricator.wikimedia.org/T151770)
Comment 9•7 years ago
|
||
Bug 1264192 comment 106 - 107 seem to be instances of this regression, too. mcmanus, can we get this on someone's radar to see what we can do here?
Comment 10•7 years ago
|
||
amy - there is telemetry here, right? What does it say. If we're impacting wiki* gotta back it out.
Flags: needinfo?(mcmanus)
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(amchung)
Comment 11•7 years ago
|
||
Should add losing-users as we've had to tell our clients to use an alternate browser.
Comment 12•7 years ago
|
||
The workaround we've been suggesting for Wikimedia users is to configure 'network.cookie.maxPerHost' to some high value (or to switch to a different browser until this is fixed).
Comment 13•7 years ago
|
||
I think we should back out the preference towards evicting session cookies that was added in bug 1264192. That is more complicated than backing out all of bug 1264192, because it still has useful heuristics about preferring to evict cookies that don't match the current request. https://dxr.mozilla.org/mozilla-central/rev/2963cf6be7f830c0d2155e2968cfc53585868a76/netwerk/cookie/nsCookieService.cpp#4666-4708 is the block that needs to be changed.
Assignee | ||
Comment 14•7 years ago
|
||
Hi Patrick, I have created a function nsCookieService::TelemetryForEvictingStaleCookie() to record the situations of evicting cookie. https://dxr.mozilla.org/mozilla-central/rev/2963cf6be7f830c0d2155e2968cfc53585868a76/netwerk/cookie/nsCookieService.cpp#4714 But this function just recording the situation as below: 1. if we evicted the non-secure cookie, recorded the situation when the evicting cookie is newer than the oldest cookie or it is the oldest cookie. 2. if we evicted the secure cookie, we would confirm it is the oldest cookie and this situation is the original condition. We don't record the situation when the session cookie is evicted, nsCookieService::TelemetryForEvictingStaleCookie() only recorded for secure cookie.
Flags: needinfo?(amchung)
Comment 15•7 years ago
|
||
> I think we should back out the preference towards evicting session cookies > that was added in bug 1264192. That is more complicated than backing out all > of bug 1264192, because it still has useful heuristics about preferring to > evict cookies that don't match the current request. That sounds like a good plan, as long as we can do it soon. Looking at bug 1264192 comment 94 onwards, I'm wondering if we might want to bump up our cookies-per-host max (to Chromium's 180, or even higher?) if that would alleviate this issue (though I'm not sure that it would). IIRC we can hot-fix prefs--could we ship a fix for current release that way?
Flags: needinfo?(jduell.mcbugs) → needinfo?(josh)
Assignee | ||
Comment 16•7 years ago
|
||
Hi Josh, I backed out the patch of evicting session cookie on bug 1264192 and passed TestCookie. Would you help me to review my patch? Thanks!
Attachment #8828310 -
Flags: review?(josh)
Comment 17•7 years ago
|
||
Comment on attachment 8828310 [details] [diff] [review] Backed out the preference towards evicting session cookies on bug 1264192 Review of attachment 8828310 [details] [diff] [review]: ----------------------------------------------------------------- netwerk/cookie/test/unit/test_eviction.js will presumably need to be adjusted based on whatever changes we make here. ::: netwerk/cookie/nsCookieService.cpp @@ -4578,5 @@ > - // the source request, or if it is not a path or domain match against the > - // source request. > - bool isPrimaryEvictionCandidate = true; > - if (aSource) { > - isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost); This reverts all of the eviction changes from bug 1264192, which will make us start hitting limits on twitter.com again. We should only revert the code that makes us prefer session cookies over non-session one - the code that looks for a path or domain match and prioritizes evicting cookies that do not match should remain.
Attachment #8828310 -
Flags: review?(josh) → review-
Comment 18•7 years ago
|
||
I have trouble believing that an extra 15 cookies will make a difference to sites that are already hitting the 165 limit. However, I guess we could alleviate it by raising it to an improbably high number instead.
Flags: needinfo?(josh)
Comment 19•7 years ago
|
||
Preferring to keep httponly cookies would make this behavior slightly saner as those cookies usually have to do with login.
Comment 20•7 years ago
|
||
Amy, are you actively working on this?
status-firefox51:
--- → wontfix
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Flags: needinfo?(amchung)
Assignee | ||
Comment 21•7 years ago
|
||
Hi Josh, If the solution to this problem is not just reverting the patch of bug 1264192, would you help to fix this bug? Thanks! Hi Ryan, I think I have to discuss with Josh to determine whether I am suitable for solving this bug.
Flags: needinfo?(amchung)
Assignee | ||
Comment 22•7 years ago
|
||
Hi Josh, If the solution to this problem is not just reverting the patch of bug 1264192, would you help to fix this bug? Thanks! Hi Ryan, I think I have to discuss with Josh to determine whether I am suitable for solving this bug.
Flags: needinfo?(josh)
Comment 23•7 years ago
|
||
https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/netwerk/cookie/nsCookieService.cpp#4698-4710 is the code that prioritizes evicting session cookies before non-session cookies. We should change it so we only prioritize the oldest non-matching cookie (regardless of session vs. non-session), then evict the oldest cookie. That means that the variables like oldestSessionCookie and oldestNonMatchingSessionCookie can be removed, and the others like oldestNonMatchingNonSessionCookie will need to be renamed and updated to track all cookies.
Flags: needinfo?(josh)
Assignee | ||
Comment 24•7 years ago
|
||
Hi Josh, Would you help me to confirm the steps of modifications? 1. Create a nsListIter and naming "oldestNonMatchingCookie". 2. Remove oldestNonMatchingSessionCookie and oldestSessionCookie. 3. Evict oldestNonMatchingCookie before than oldestCookie. 4. Modify test case. Thanks!
Flags: needinfo?(josh)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → amchung
Whiteboard: [necko-next] → [necko-active]
Updated•7 years ago
|
Priority: -- → P1
Assignee | ||
Comment 26•7 years ago
|
||
Hi Josh, I have modified this bug as below: 1. Created a nsListIter and naming "oldestNonMatchingCookie". 2. Removed oldestNonMatchingSessionCookie and oldestSessionCookie. 3. Evicted oldestNonMatchingCookie before than oldestCookie. 4. Only modified the test cases in test_basic_eviction(). Would you help me to review my patch? Thanks!
Attachment #8839334 -
Flags: review?(josh)
Comment 27•7 years ago
|
||
Comment on attachment 8839334 [details] [diff] [review] implementation + test cases Review of attachment 8839334 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cookie/nsCookieService.cpp @@ +4682,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. // Prefer to evict the oldest cookie with a non-matching path/domain, // followed by the oldest matching cookie. ::: netwerk/cookie/test/unit/test_eviction.js @@ +185,5 @@ > 'session_non_path_non_domain_3'], BASE_URI); > > // Evict oldest session cookie; all such cookies match example.org/bar (session_bar_path_2) > // note: this new cookie doesn't have an explicit path, but empty paths inherit the > // request's path The comments in this test need to be updated to reflect the changes in the expected results.
Attachment #8839334 -
Flags: review?(josh) → review+
Assignee | ||
Comment 28•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1bf60ec3f3de042f0bbad663658ef8ebdcf45ec
Attachment #8839334 -
Attachment is obsolete: true
Attachment #8841481 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 29•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b780bfdac971 Modified nsCookieService::FindStaleCookie() and test cases, r=jdm
Keywords: checkin-needed
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b780bfdac971
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 31•7 years ago
|
||
Thanks for working on this! Can you explain what the current situation is? What I can glean from the patch is that Firefox will still start dropping cookies when the 150-per-second-level-domain limit is reached, but the order in which cookies are dropped will be different and session cookies will probably be dropped later. If that's the case, I don't see how the bug is fixed; it just takes slightly longer for users to get logged out (or suffer some other ill UX effect from unexpected cookie loss). Am I misunderstanding the patch?
Comment 32•7 years ago
|
||
This has always been the case in Firefox, and is the case in other browsers too. If websites are hitting these limits, that's really something that needs to be fixed on their end. This patch is just reverting the behaviour change from bug 1264192 that made it especially visible when cookies were evicted.
Comment 33•7 years ago
|
||
Seems highly unlikely we'll be able to take this fix for Fx52 since we're already creating release candidates. But we should probably consider uplifting this to Aurora for 53 at least? And maybe even ESR52 if we're feeling really good about it.
status-firefox-esr52:
--- → affected
tracking-firefox-esr52:
--- → ?
Flags: needinfo?(amchung)
Flags: in-testsuite+
Assignee | ||
Comment 34•7 years ago
|
||
Comment on attachment 8841481 [details] [diff] [review] implementation + test cases Approval Request Comment [Feature/Bug causing the regression]:The patch of bug 1264192 caused the session cookies be deleted first, and some users replied the problem about they can't login on website. [User impact if declined]:Removed oldestNonMatchingSessionCookie and oldestSessionCookie on the modification of bug 1264192 to avoid session cookies be deleted first. [Is this code covered by automated tests?]:Manual and automated, https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1bf60ec3f3de042f0bbad663658ef8ebdcf45ec. [Needs manual test from QE? If yes, steps to reproduce]:no [List of other uplifts needed for the feature/fix]: [Is the change risky?]:Medium [String changes made/needed]:none
Flags: needinfo?(amchung)
Attachment #8841481 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 35•7 years ago
|
||
Hi Josh, I have requested a uplift on aurora 53, would you give me some suggestions? Thanks!
Flags: needinfo?(josh)
Comment 36•7 years ago
|
||
User impact if declined is continued poor behaviour on a number of websites that both use session cookies and send lots of cookies. I do think that uplifting this makes sense.
Flags: needinfo?(josh)
Comment 37•7 years ago
|
||
Comment on attachment 8841481 [details] [diff] [review] implementation + test cases Mitigation for session cookie issue, let's take it for aurora 53. Thanks Amy!
Attachment #8841481 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 38•7 years ago
|
||
Andrei, we may want to test this further (in nightly) From STR here and in bug 1333256.
Flags: needinfo?(andrei.vaida)
Comment 39•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c6b8d765254
Comment 40•7 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #38) > Andrei, we may want to test this further (in nightly) From STR here and in > bug 1333256. Hello, Liz! Is bug 1333256 the right one for following STR? I'm asking this because I don't see any connection between these two issues.
Flags: needinfo?(andrei.vaida)
Updated•7 years ago
|
Flags: needinfo?(lhenry)
Comment 41•7 years ago
|
||
Aha, you are right, I meant to say bug 1264192. Must have been a copy and paste error on my part.
Flags: needinfo?(lhenry) → needinfo?(iulia.cristescu)
Comment 42•7 years ago
|
||
Can yo please provide some test credentials for one of the sites you mentioned in comment 0, in order to properly investigate this issue? Thank you!
Flags: needinfo?(rkarnia)
Comment 43•7 years ago
|
||
(In reply to Iulia Cristescu, QA [:IuliaC] from comment #42) > Can yo please provide some test credentials You could likely create an account on https://www.mediawiki.org or https://en.wikipedia.org (they have a unified login anyway)
Comment 44•7 years ago
|
||
I wasn't able to reproduce the issue using 53.0a1 (2016-11-22) (since the bug was reported), neither on 50.0a1 (2016-07-05). Used https://twitter.com, https://www.mediawiki.org/ and the instructions from comment 0 and bug 1264192. rkarnia, Amy, any thoughts about this?
Flags: needinfo?(iulia.cristescu) → needinfo?(amchung)
Comment 45•7 years ago
|
||
This bug has been causing significant problems at the university where I work because we use the URL-rewriting EZproxy software (http://www.oclc.org/en/ezproxy.html) to access academic literature at hundreds of sites. EZproxy rewrites site hostnames with the university's domains as higher level domains. E.g., www.jstor.org is rewritten like www.jstor.org.ezproxy.myuniversity.edu. Cookies are usually passed through the EZproxy server to the browser. So, after accessing some of the university's own servers, plus one or two dozen proxied sites, with authentication for all handled by the university's single sign-on (SSO) system, the 165-cookie limit for myuniversity.edu is reached. Then the session cookie for the SSO system (I presume) gets evicted, and access to SSO-enabled sites fails, producing a variety of obscure error messages, depending on the site. I personally have experienced this problem numerous time since late Nov (although it was not until Feb that I realised the cause), but have not been able to reproduce it on demand after deleting cookies! If I could reproduce it on demand I would offer to test.
Comment 46•7 years ago
|
||
It would be great if you could try out a Firefox 53 Beta build and see if it behaves any better.
Comment 47•7 years ago
|
||
I also got issues around this in a recent Nightly/Android. But this happens much less frequently and much more randomly, so it's difficult to pinpoint a STR. For me this is on https://lemonde.fr where I log in as a subscribed user.
Comment 48•7 years ago
|
||
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 set another sub-domain [xyz.abc.com] session cookie, it fails with beta patch also, however domain based session cookie is set now.
Comment 49•7 years ago
|
||
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.
Comment 50•7 years ago
|
||
Not quite sure, but for EZProxy one solution could be to treat any subdomain as a separate domain (level) and delete cookies only for each subdomain reaching more than a given threshold.
Comment 51•7 years ago
|
||
(In reply to info from comment #50) > Not quite sure, but for EZProxy one solution could be to treat any subdomain > as a separate domain (level) and delete cookies only for each subdomain > reaching more than a given threshold. I also agree with it. FF should treat each sub domain as separate domain. Big organization have 100+ applications which user access. If most of them create secure cookie then application who is creating session cookie won't able to set cookie.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(amchung)
Comment 52•7 years ago
|
||
This feels risky for ESR, setting to wontfix. If people are still experiencing issues on firefox 53 or later, please file new bugs so we can track remaining issues separately.
You need to log in
before you can comment on or make changes to this bug.
Description
•