Last Comment Bug 1319403 - deleting session cookies
: deleting session cookies
Status: NEW
[necko-active]
: regression
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: 50 Branch
: Unspecified Unspecified
P1 normal with 5 votes (vote)
: ---
Assigned To: Amy Chung [:Amy]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 1264192
  Show dependency treegraph
 
Reported: 2016-11-22 04:25 PST by rkarnia
Modified: 2017-02-22 08:05 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
affected
affected
affected


Attachments
Backed out the preference towards evicting session cookies on bug 1264192 (5.01 KB, patch)
2017-01-19 05:24 PST, Amy Chung [:Amy]
josh: review-
Details | Diff | Splinter Review
implementation + test cases (8.03 KB, patch)
2017-02-20 22:01 PST, Amy Chung [:Amy]
josh: review+
Details | Diff | Splinter Review

Description User image rkarnia 2016-11-22 04:25:37 PST
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 User image michal.kubec 2016-11-28 23:25:14 PST
Same here. Radnom logout from my site. PHPSESSID cookie is evicted
Comment 2 User image michal.kubec 2016-11-28 23:32:02 PST
Related with bug 1047231 ?
Comment 3 User image Josh Matthews [:jdm] 2016-11-29 05:15:40 PST
Bug 1047231 is only related if zattoo.com stores more than 165 cookies.
Comment 4 User image Dragana Damjanovic [:dragana] 2016-11-29 06:28:41 PST
Maybe this is related to bug 1047231.
Comment 5 User image tech_staff 2016-11-29 07:02:02 PST
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 User image michal.kubec 2016-11-29 07:05:10 PST
Its look like a subdomain cookies problem. No problems before FF50
Comment 7 User image Amy Chung [:Amy] 2016-11-30 04:15:59 PST
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 User image Bartosz Dziewoński 2016-12-26 13:07:44 PST
This issue might also be affecting Wikipedia and other Wikimedia wikis.
(Our bug report: https://phabricator.wikimedia.org/T151770)
Comment 9 User image Daniel Holbert [:dholbert] 2017-01-11 10:04:01 PST
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 User image Patrick McManus [:mcmanus] 2017-01-11 10:06:30 PST
amy - there is telemetry here, right? What does it say.

If we're impacting wiki* gotta back it out.
Comment 11 User image kkilmer6 2017-01-11 10:48:12 PST
Should add losing-users as we've had to tell our clients to use an alternate browser.
Comment 12 User image Bartosz Dziewoński 2017-01-11 11:06:52 PST
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 User image Josh Matthews [:jdm] 2017-01-11 11:09:25 PST
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 User image Amy Chung [:Amy] 2017-01-11 22:07:58 PST
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 User image Jason Duell [:jduell] (needinfo me) 2017-01-19 00:32:43 PST
> 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 User image Amy Chung [:Amy] 2017-01-19 05:24:22 PST
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!
Comment 17 User image Josh Matthews [:jdm] 2017-01-19 05:30:51 PST
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 User image Josh Matthews [:jdm] 2017-01-19 05:32:52 PST
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 User image Tisza Gergő 2017-01-25 13:27:04 PST
Preferring to keep httponly cookies would make this behavior slightly saner as those cookies usually have to do with login.
Comment 20 User image Ryan VanderMeulen [:RyanVM] 2017-02-01 06:14:38 PST
Amy, are you actively working on this?
Comment 21 User image Amy Chung [:Amy] 2017-02-02 19:27:05 PST
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 User image Amy Chung [:Amy] 2017-02-02 19:27:31 PST
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 User image Josh Matthews [:jdm] 2017-02-03 07:49:21 PST
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 User image Amy Chung [:Amy] 2017-02-12 19:21:28 PST
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 User image Josh Matthews [:jdm] 2017-02-13 06:50:37 PST
That sounds correct to me.
Comment 26 User image Amy Chung [:Amy] 2017-02-20 22:01:21 PST
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!
Comment 27 User image Josh Matthews [:jdm] 2017-02-22 08:05:22 PST
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.

Note You need to log in before you can comment on or make changes to this bug.