Closed Bug 1357676 Opened 7 years ago Closed 6 years ago

Implement batch eviction for cookie

Categories

(Core :: Networking: Cookies, enhancement, P2)

Product:
Core
Component:
Networking: Cookies
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: kershaw)

Details

(Whiteboard: [necko-next])

Attachments

(2 files, 1 obsolete file)

Bug 1357676 - Implement batch eviction
46 bytes, text/x-phabricator-request
jdm
: review+
jdm
: feedback+
Details | Review
Bug 1357676 - Modify failed tests
46 bytes, text/x-phabricator-request
jdm
: review+
Details | Review 
Valentin tells me Chrome is currently allowing 180 cookies per host (we allow 150), and a total cookie limit of 3300 (ours is 3000).

Probably worth bumping these up so we avoid Firefox-only cookie issues.
This should be a really easy patch.
Whiteboard: necko-next
Looks like the differences (30/300) are they can make sure they can keep at least 150/3000 cookies in hand ([1]).
If we clean old cookies as soon as 150/3000 achieved, we cannot guarantee that especially if a website needs exactly 150 cookies, right?

[1] https://cs.chromium.org/chromium/src/net/cookies/cookie_monster.cc?type=cs&l=112
More datapoints: According to John Wilander back in 2012, Safari allows/allowed 1000(!) cookies: https://twitter.com/johnwilander/status/253074683879096320 There's also a collection of cookie limits here: http://browsercookielimits.squawky.net/
Before increasing the limitation, we have to make sure the issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1344090#c14 doesn't happen.
Whiteboard: necko-next → [necko-next]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P2
Assignee: nobody → amchung
Hi Josh,
I have finished the batch eviction and algorithm as below:
1. in FindStaleCookies()
   i.   Found the cookie which can't pass DomainMatches() and PathMatches().
   ii.  Stored the index value to list.
   iii. If the list doesn't exist any index value when break for loop,
         a. Stored the index value which belongs to the oldest cookie.
2. in AddInternal()
   i.   Confirmed the length of list.
   ii.  Removed the cookies.

[Todo--Test cases]
1. Add test case on TestCookie.cpp
   i.  Test 1:
       a. Create 100 session cookies whose domain = "test1.example.com" from "http://test1.example.com".
       b. Create 80 secure cookies from "http://www.example.com".
       c. Confirm all session cookies are removed.
   ii. Test 2:
       a.  Create 100 secure cookies whose domain = "test1.example.com" from "http://test1.example.com"
       b.  Create 80 session cookies from "http://www.example.com".
       c.  Confirm the oldest session cookie is removed.

Would you help me to review this patch?
Thanks!
Attachment #8924948 - Flags: review?(josh)
Comment on attachment 8924948 [details] [diff] [review]
implementation -- Modified the limit to 180 and added batch eviction

Review of attachment 8924948 [details] [diff] [review]:
-----------------------------------------------------------------

This review has been 90% complete for a week, but I don't want to leave it sitting around until the final 10% is done. I have emailed mcmanus about my concerns with the eviction heuristics (PathMatches and DomainMatches look too risky for batch eviction to me), and also how I don't see a clear better option right now. I am hoping some people from the network team can focus on coming up with a better eviction heuristic, especially if it's based on real cookie data (which is something our previous choices have been lacking). Unfortunately, I do not have time right now to focus on figuring out a better model.

::: netwerk/cookie/nsCookieService.cpp
@@ +3814,2 @@
>          if (aCookie->IsSecure()) {
> +           printf_stderr("\n\n Find secure cookie first \n\n");

Remove this.

@@ +3818,3 @@
>          } else {
>            Telemetry::Accumulate(Telemetry::COOKIE_LEAVE_SECURE_ALONE,
> +                                  EVICTING_SECURE_BLOCKED);

nit: revert this indentation change.

@@ +3828,5 @@
> +      for (uint64_t i = 0; i < removedIterIndexList.Length(); i++) {
> +        nsListIter iter;
> +        iter.entry = entry;
> +        // i equals the number of cookies which be deleted.
> +        iter.index = removedIterIndexList.ElementAt(i) - i;

Let's iterate backwards over the list instead:
for (uint64_t i = removedIterIndexList.Length(); i > 0; i--) {
  ...
  iter.index = removedIterIndexList.ElementAt(i - 1);
  ...
}

@@ +3830,5 @@
> +        iter.entry = entry;
> +        // i equals the number of cookies which be deleted.
> +        iter.index = removedIterIndexList.ElementAt(i) - i;
> +        RefPtr<nsCookie> evictedCookie;
> +        evictedCookie = iter.Cookie();

RefPTr<nsCookie> evictedCookie = iter.Cookie();

@@ +3837,5 @@
> +                                          evictedCookie->LastAccessed());
> +        }
> +        COOKIE_LOGEVICTED(evictedCookie, "Too many cookies for this domain");
> +        RemoveCookieFromList(iter);
> +        purgedList = CreatePurgeList(evictedCookie);

We are recreating purgedList for each cookie. We should instead append the evicted cookie to an existing list.

@@ +4782,5 @@
>    nsListIter oldestCookie;
>    oldestCookie.entry = nullptr;
>  
>    int64_t actualOldestCookieTime = cookies.Length() ? cookies[0]->LastAccessed() : 0;
> +  uint32_t cookiesLen = cookies.Length();

This isn't used.

@@ +4787,4 @@
>    for (nsCookieEntry::IndexType i = 0; i < cookies.Length(); ++i) {
>      nsCookie *cookie = cookies[i];
>  
> +    if (!cookie->IsSecure())

Please add {} and indent the next statement correctly.

@@ +4814,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);

As I have mentioned in previous emails, we should not check PathMatches or DomainMatches if we evict cookies in batches. It's too easy to evict a lot of cookies that the user wants if the cookie that triggers eviction is a cookie for a subdomain or different path than the current document's request.
Attachment #8924948 - Flags: review?(josh) → review-
Hi Josh,
I tried to modify the algorithm as below:
1. Maximum of the cookies per host:180
2. Deleting algorithm
    1. If this cookie is expired
        1. Delete all kind of this cookies
    2. Otherwise, if this cookie doesn’t set secure flag, isn’t a session cookie, isn't a same origin(have to confirm DomainMatch(), port number and protocol)
        1. Delete all kind of this cookies.
    3. Otherwise, if this cookie is a session cookie.
        1. Delete one of this cookie
    4. Refuse to set the newest cookie.

Would you give me your suggestions?
Thanks!
Flags: needinfo?(josh)
Hi Josh,
Sorry, I have to modified the deleting algorithm:
* Deleting algorithm
    1. If this cookie is expired
        1. Delete all kind of this cookies
    2. Otherwise, if this cookie doesn’t set secure flag, isn’t a session cookie, isn't a same origin(have to confirm DomainMatch())
        1. Delete all kind of this cookies.
    3. Otherwise, if this cookie is a session cookie.
        1. Delete one of this cookie: for avoiding to remove the login info.
    4. Refuse to set the newest cookie.
Flags: needinfo?(josh)
Flags: needinfo?(josh)
(In reply to Amy Chung [:Amy] from comment #9)
> Hi Josh,
> Sorry, I have to modified the deleting algorithm:
> * Deleting algorithm
>     1. If this cookie is expired
>         1. Delete all kind of this cookies
>     2. Otherwise, if this cookie doesn’t set secure flag, isn’t a session
> cookie, isn't a same origin(have to confirm DomainMatch())
>         1. Delete all kind of this cookies.
>     3. Otherwise, if this cookie is a session cookie.
>         1. Delete one of this cookie: for avoiding to remove the login info.
>     4. Refuse to set the newest cookie.

I'm not sure if I understand step 2 - if there are no expired cookies for this origin, we collect all of the insecure non-session cookies that do not pass the DomainMatch check?

Let me try to explain my concern with using DomainMatch in these eviction checks. If we load http://subdomain.foo.com/, and that page contains an iframe for http://another-subdomain.foo.com/, and the iframe sets a cookie that forces us to evict cookies, we will end up evicting cookies that do not match http://another-subdomain.foo.com/. If we assume that the cookies for the top-level document are most important (http://subdomain.foo.com), this is not a good result because we will end up choosing to evict them.

Additionally, if I understand step 3 correctly, isn't that the same heuristic that I implemented in last year (https://bugzilla.mozilla.org/show_bug.cgi?id=1264192#c65)? We removed that later because we kept logging people out, so it's not clear why we want to return to evicting a single session cookie at a time.
Flags: needinfo?(josh) → needinfo?(amchung)
https://bugzilla.mozilla.org/show_bug.cgi?id=1319403 is where we reverted the heuristic to prefer evicting a single session cookie.
Hi Josh,
I have modified the batch-eviction algorithm as below:
1. Create new cookie attributes naming thirdParty.
2. Set thirdParty = true if the cookie is thirdParty cookie.
3. Batch eviction:
    1. If the cookie expired, FindStaleCookies will select the indexes of all expired cookies to the removing list.
        1. The removing list only store the cookie index.
    2. If the cookies match the following conditions, FindStaleCookies will select the indexes of the cookies to the removing list.
        1. non-secure cookie
        2. non-session cookie
        3. thirdParty cookie
    3. If no cookie matches the condition 1 and 2, FindStaleCookies will select the oldest  isPrimaryEvictionCandidate = true cookie to the removing list.
        1.  isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost).
    4. If no cookie matches the condition 1, 2 and 3, FindStaleCookies will select the oldest cookie to the revving list.
    5. If no cookie matches the condition 3, FindStaleCookies will not select any cookie index to the removing list.
        1. AddInternal() will refuse to set the newest cookie.

Would you give me your suggestions?
Thanks!
Flags: needinfo?(amchung) → needinfo?(josh)
Hi Josh,
I found Comment 12 doesn't process the situation when users reject the third party cookie.
Base on above reason, I redesign the batch eviction algorithm as below:
1. When users accept the third party cookie.
   a.  Create new cookie attributes naming thirdParty.
       i.  the strcut CookieAttributes add isThirdParty.
       ii. nsCookie.h add mThirdParty, and create IsThirdParty() & SetIsThirdParty
2. When users reject the third party cookie.
   a.   Create a hashtable to store the nsIURI, and the hash table key is base domain.
   b.   When nsDocument destructor is called,
        i.   Send uri which has to be removed to CookieServiceChild.
        ii . CookieServiceChild send the uri to CookieServiceParent.
        iii. CookieServiceParent send the uri to nsCookieService.
        v.   When nsCookieService revive the uri, nsCookieService will remove the uri from hash table.

[Batch eviction]
 1. If the cookie expired, FindStaleCookies will select the indexes of all expired cookies to the removing list.
        1. The removing list only store the cookie index.
    2. If user accept the thirdparty cookies:
        The cookies match the following conditions, FindStaleCookies will select the indexes of the cookies to the removing list.
           1. non-secure cookie
           2. non-session cookie
           3. thirdParty cookie
       Otherwise,
         The cookies match the following conditions, FindStaleCookies will select the indexes of the cookies to the removing list.
           1. non-secure cookie
           2. non-session cookie
           3. !DomainMatches(the uri which is in the hashtable, the cookie's domain).
              i.  the uri's base domain = the base domain of the cookie's domain 
    3. If no cookie matches the condition 1 and 2, FindStaleCookies will select the oldest  isPrimaryEvictionCandidate = true cookie to the removing list.
        1.  isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost).
    4. If no cookie matches the condition 1, 2 and 3, FindStaleCookies will select the oldest cookie to the revving list.
    5. If no cookie matches the condition 3, FindStaleCookies will not select any cookie index to the removing list.
        1. AddInternal() will refuse to set the newest cookie.


Would you give me your suggestions?
Thanks!
Flags: needinfo?(josh)
Could you explain further why rejecting a third party cookie is special and needs to be handled?
Assignee: amchung → hurley
Assignee: hurley → kershaw
1. Add network.cookie.QuotaPerHost, which has the default value 150.
2. When the cookies exceed more than 180, evict cookies to 150.
3. The concept of eviction is to sort all cookies by whether the cookie is expired and the cookie's last access time. Then, evict cookies by the given count.
4. Details of evict algorithm:
   4.1 Create a priority queue and push all cookies in it.
   4.2 Use custom comparator to compare the cookie by expiry and last access.
   4.3 Pop 30(180 - 150) cookies from the queue and append them to an output array.
Comment on attachment 8999975 [details]
Bug 1357676 - Implement batch eviction

Hi Josh,

Could you take a look at this patch?

Thanks.
Attachment #8999975 - Flags: feedback?(josh)
Comment on attachment 8999975 [details]
Bug 1357676 - Implement batch eviction

Josh Matthews [:jdm] has approved the revision.
Attachment #8999975 - Flags: review+
Attachment #8999975 - Flags: feedback?(josh) → feedback+
We've introduced a new pref network.cookie.quotaPerHost and also a rule that the value of network.cookie.maxPerHost should always be bigger than network.cookie.quotaPerHost. So, before changing the value of network.cookie.maxPerHost, we have to set network.cookie.quotaPerHost first.
This patch only sets network.cookie.quotaPerHost equal to network.cookie.maxPerHost - 1 in all failed tests.
(In reply to Kershaw Chang [:kershaw] from comment #19)
> Created attachment 9004259 [details]
> Bug 1357676 - Modify failed tests
> 
> We've introduced a new pref network.cookie.quotaPerHost and also a rule that
> the value of network.cookie.maxPerHost should always be bigger than
> network.cookie.quotaPerHost. So, before changing the value of
> network.cookie.maxPerHost, we have to set network.cookie.quotaPerHost first.
> This patch only sets network.cookie.quotaPerHost equal to
> network.cookie.maxPerHost - 1 in all failed tests.

Hi Josh,

Do you have a chance to look at this patch? I think it's just some small changes to tests.

Thanks.
Flags: needinfo?(josh)
Comment on attachment 9004259 [details]
Bug 1357676 - Modify failed tests

Josh Matthews [:jdm] has approved the revision.
Attachment #9004259 - Flags: review+
Flags: needinfo?(josh)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/513621c77f12
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Attachment #8924948 - Attachment is obsolete: true
Update the summary, since we already up the limit to 180 in bug 1460251.
Summary: Increase cookie count limits to match Chrome's → Implement batch eviction for cookie
Hi Raul,

I think there is another patch needs to commit [1].
Could you help me to do this?

Thanks.

[1] https://phabricator.services.mozilla.com/D3342
Flags: needinfo?(rgurzau)
Flags: needinfo?(rgurzau)
Hi,

Lando reports that you have to manually rebase your commits and try again: https://lando.services.mozilla.com/revisions/D3342/10924/
Also tried importing it as a patch but got this error:

applying D3342.diff
patching file netwerk/cookie/nsCookieService.h
Hunk #1 FAILED at 316
1 out of 2 hunks FAILED -- saving rejects to file netwerk/cookie/nsCookieService.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh D3342.diff

And this is the .rej file:


--- nsCookieService.h
+++ nsCookieService.h
@@ -317,13 +317,14 @@
     already_AddRefed<nsIArray>    PurgeCookies(int64_t aCurrentTimeInUsec);
     bool                          FindCookie(const nsCookieKey& aKey, const nsCString& aHost, const nsCString& aName, const nsCString& aPath, nsListIter &aIter);
     bool                          FindSecureCookie(const nsCookieKey& aKey, nsCookie* aCookie);
-    int64_t                       FindStaleCookie(nsCookieEntry *aEntry, int64_t aCurrentTime, nsIURI* aSource, const mozilla::Maybe<bool> &aIsSecure, nsListIter &aIter);
+    void                          FindStaleCookies(nsCookieEntry *aEntry, int64_t aCurrentTime, const mozilla::Maybe<bool> &aIsSecure, nsTArray<nsListIter>& aOutput, uint32_t aLimit);
     void                          TelemetryForEvictingStaleCookie(nsCookie* aEvicted, int64_t oldestCookieTime);
     void                          NotifyRejected(nsIURI *aHostURI);
     void                          NotifyThirdParty(nsIURI *aHostURI, bool aAccepted, nsIChannel *aChannel);
     void                          NotifyChanged(nsISupports *aSubject, const char16_t *aData, bool aOldCookieIsSession = false, bool aFromHttp = false);
     void                          NotifyPurged(nsICookie2* aCookie);
     already_AddRefed<nsIArray>    CreatePurgeList(nsICookie2* aCookie);
+    void                          CreateOrUpdatePurgeList(nsIArray** aPurgeList, nsICookie2* aCookie);
     void                          UpdateCookieOldestTime(DBState* aDBState, nsCookie* aCookie);

     nsresult                      GetCookiesWithOriginAttributes(const mozilla::OriginAttributesPattern& aPattern, const nsCString& aBaseDomain, nsISimpleEnumerator **aEnumerator);
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: