deleting session cookies

RESOLVED FIXED in Firefox 53

Status

()

Core
Networking: Cookies
P1
normal
RESOLVED FIXED
6 months ago
7 days ago

People

(Reporter: rkarnia, Assigned: Amy, NeedInfo)

Tracking

({regression})

50 Branch
mozilla54
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 fixed, firefox54 fixed, firefox-esr52- wontfix)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 months ago
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

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

Comment 2

6 months ago
Related with bug 1047231 ?

Comment 3

6 months ago
Bug 1047231 is only related if zattoo.com stores more than 165 cookies.
Maybe this is related to bug 1047231.
Flags: needinfo?(amchung)
Whiteboard: [necko-next]

Comment 5

6 months 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

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

Updated

6 months ago
Blocks: 1264192
(Assignee)

Comment 7

6 months 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)
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 8

5 months ago
This issue might also be affecting Wikipedia and other Wikimedia wikis.
(Our bug report: https://phabricator.wikimedia.org/T151770)
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?
status-firefox50: --- → affected
Flags: needinfo?(mcmanus)
Keywords: regression
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

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

Comment 12

5 months 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

5 months 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

5 months 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)
> 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

4 months ago
Created attachment 8828310 [details] [diff] [review]
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!
Attachment #8828310 - Flags: review?(josh)

Comment 17

4 months 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

4 months 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

4 months ago
Preferring to keep httponly cookies would make this behavior slightly saner as those cookies usually have to do with login.
Amy, are you actively working on this?
status-firefox50: affected → wontfix
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
Flags: needinfo?(amchung)
(Assignee)

Comment 21

4 months 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

4 months 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

4 months 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

3 months 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)

Comment 25

3 months ago
That sounds correct to me.
Flags: needinfo?(josh)
(Assignee)

Updated

3 months ago
Assignee: nobody → amchung
Whiteboard: [necko-next] → [necko-active]

Updated

3 months ago
Priority: -- → P1
(Assignee)

Comment 26

3 months ago
Created attachment 8839334 [details] [diff] [review]
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!
Attachment #8839334 - Flags: review?(josh)

Comment 27

3 months 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

3 months ago
Created attachment 8841481 [details] [diff] [review]
implementation + test cases

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1bf60ec3f3de042f0bbad663658ef8ebdcf45ec
Attachment #8839334 - Attachment is obsolete: true
Attachment #8841481 - Flags: review+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 29

3 months 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
https://hg.mozilla.org/mozilla-central/rev/b780bfdac971
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 31

3 months 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

3 months 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.
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-firefox52: affected → wontfix
status-firefox-esr52: --- → affected
tracking-firefox-esr52: --- → ?
Flags: needinfo?(amchung)
Flags: in-testsuite+
(Assignee)

Comment 34

3 months 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

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

Thanks!
Flags: needinfo?(josh)

Comment 36

3 months 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 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+
Andrei, we may want to test this further (in nightly) From STR here and in bug 1333256.
Flags: needinfo?(andrei.vaida)

Comment 39

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c6b8d765254
status-firefox53: affected → fixed
(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)
Flags: needinfo?(lhenry)
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)
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

2 months 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)
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

2 months 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.
It would be great if you could try out a Firefox 53 Beta build and see if it behaves any better.
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

2 months 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

2 months 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

2 months 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

a month 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

9 days ago
Flags: needinfo?(amchung)
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.
status-firefox-esr52: affected → wontfix
tracking-firefox-esr52: ? → -
You need to log in before you can comment on or make changes to this bug.