1319403 - deleting session cookies

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[rkarnia]

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

[michal.kubec]

Same here. Radnom logout from my site. PHPSESSID cookie is evicted

Comment 2

[michal.kubec]

Related with bug 1047231 ?

Comment 3

[Josh Matthews [:jdm]]

Bug 1047231 is only related if zattoo.com stores more than 165 cookies.

Comment 4

[Dragana Damjanovic [:dragana]]

Maybe this is related to bug 1047231.

Comment 5

[tech_staff]

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

[michal.kubec]

Its look like a subdomain cookies problem. No problems before FF50

Comment 7

[Amy Chung [:Amy]]

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.

Comment 8

[Bartosz Dziewoński]

This issue might also be affecting Wikipedia and other Wikimedia wikis.
(Our bug report: https://phabricator.wikimedia.org/T151770)

Comment 9

[Daniel Holbert [:dholbert]]

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

[Patrick McManus [:mcmanus]]

amy - there is telemetry here, right? What does it say.

If we're impacting wiki* gotta back it out.

Comment 11

[kkilmer6]

Should add losing-users as we've had to tell our clients to use an alternate browser.

Comment 12

[Bartosz Dziewoński]

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

[Josh Matthews [:jdm]]

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.

Comment 14

[Amy Chung [:Amy]]

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.

Comment 15

[Jason Duell]

> 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?

Comment 16

[Amy Chung [:Amy]]

Attachment: Backed out the preference towards evicting session cookies on bug 1264192

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!

Comment 17

[Josh Matthews [:jdm]]

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.

Comment 18

[Josh Matthews [:jdm]]

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.

Comment 19

[Tisza Gergő]

Preferring to keep httponly cookies would make this behavior slightly saner as those cookies usually have to do with login.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Amy, are you actively working on this?

Comment 21

[Amy Chung [:Amy]]

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.

Comment 22

[Amy Chung [:Amy]]

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.

Comment 23

[Josh Matthews [:jdm]]

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.

Comment 24

[Amy Chung [:Amy]]

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!

Comment 25

[Josh Matthews [:jdm]]

That sounds correct to me.

Comment 26

[Amy Chung [:Amy]]

Attachment: implementation + test cases

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!

Comment 27

[Josh Matthews [:jdm]]

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.

Comment 28

[Amy Chung [:Amy]]

Attachment: implementation + test cases

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1bf60ec3f3de042f0bbad663658ef8ebdcf45ec

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b780bfdac971
Modified nsCookieService::FindStaleCookie() and test cases, r=jdm

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b780bfdac971

Comment 31

[Tisza Gergő]

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

[Josh Matthews [:jdm]]

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

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 34

[Amy Chung [:Amy]]

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

Comment 35

[Amy Chung [:Amy]]

Hi Josh,
I have requested a uplift on aurora 53, would you give me some suggestions?

Thanks!

Comment 36

[Josh Matthews [:jdm]]

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.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8841481 [details] [diff] [review]
implementation + test cases

Mitigation for session cookie issue, let's take it for aurora 53. Thanks Amy!

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, we may want to test this further (in nightly) From STR here and in bug 1333256.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c6b8d765254

Comment 40

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(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.

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Aha, you are right, I meant to say bug 1264192. Must have been a copy and paste error on my part.

Comment 42

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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!

Comment 43

[Andre Klapper]

(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

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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?

Comment 45

[Bob Pearson]

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

[Ryan VanderMeulen [:RyanVM]]

It would be great if you could try out a Firefox 53 Beta build and see if it behaves any better.

Comment 47

[Julien Wajsberg [:julienw]]

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

[AM]

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

[AM]

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

[info]

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

[AM]

(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.

Comment 52

[Julien Cristau [:jcristau]]

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.