New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 609550 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Cookie eviction does not precisely follow priority quota guidelines

Project Member Reported by jww@chromium.org, May 5 2016

Issue description

According to https://tools.ietf.org/html/draft-west-cookie-priority#section-3, priority quotas are intended to prevent services that require low or medium priority cookies from being completely starved by services on the same registrable domain that set high priority cookies.

In our implementation, this is not completely the case. In particular, because quotas accumulate one priority round to the next, and are not reserved for the specific priority level, a later round may evict lower priority cookies that should be protected by the priority's quota.

Take, for example, a set of cookies, from least-recently accessed to most recently accessed, consisting of 50 low-priority cookies and 131 high-priority cookies. Based on the quota definition, we would expect first 20 low-priority cookies to be evicted, down to the 30 low-priority cookie quota. Then we would expect 11 high-priority cookies, down to 120 high-priority cookies, and the 150 total cookie quota.

However, what we actually see is 19 low-priority cookies and 131 high-priority cookies. Because the low-priority cookies are least-recently accessed, and the quotas accumulate, the last round evicts the low priority ones first.
 
I spend some time digging into this case, and to my mind the eviction does not work as it should.

As I understand, we should remove LN cookies not more than the quota, then LN + MN  not more than the quota, then LN+MN+HN  not more than the quota. Thus, we will not have any issues with too many LN or MN cookies deleted.

For example, this test is just adjusted to the code according to the current implementation.
Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
    TestPriorityCookieCase(cm.get(), "11HN 10MN 20LN 110MN 20LN 10HN", 20U,
                           109U, 21U, 150U, 0U); - it does not fail and does not preserve the quota

But TestPriorityCookieCase(cm.get(), "11HN 10MN 110MN 20LN 20LN 10HN", 30U,
                           109U, 21U, 150U, 0U); 
will behave differently. It goes like this - Round 1 => 10L; round 2 => 21M; round 3 => none. Having at least 30 LN, at least 50 MN (we can't have at least 70 HN, because we lacked many of them before the eviction).

Shouldn't that be right? After the eviction we should have at least 30 LN, 50 MN and 70 HN according to the quota. 

Comment 2 by jww@chromium.org, May 12 2016

Right, I believe you've summed up what I was trying to get at in the original comment. Also note that a week ago I put in a comment explaining this issue (and linking here) with a commented out example that would fail in today's code (see the TODO comment at https://code.google.com/p/chromium/codesearch#chromium/src/net/cookies/cookie_monster_unittest.cc&l=602)

We would expect the following test to pass:
// Round 1 => 20L; round 2 => 0; round 3 => 11H
TestPriorityCookieCase(cm.get(), "50LN 131HN", 30U, 0U, 120U, 150U, 0U);

But it fails today, resulting in 19L and 131L for all of the reasons cited above. I think you point out a good example of another test we have that passes "incorrectly" according to the spec.

I have a draft cl to be uploaded with the solution according to my thoughts. I will upload that in a while.
I added you as a reviewer. Please take a look. 
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe1fea6df040fff38a3a4567e20a3d5847e83170

commit fe1fea6df040fff38a3a4567e20a3d5847e83170
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Mon Jun 06 16:48:25 2016

Making cookies eviction quotas match spec

According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old cookie monster implementation doesn't
maintain priority quotas correctly.

This CL makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority
cookies if there had been enough of them before  the eviction.
Please note, secure cookies are more important than non-secure
per https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone.

A small example: if there are 70 cookies:
37 non-secure low priority and 33 secure low priority
cookies, 37 non-secure cookies are deleted during the first
round and other 3 secure low priority cookies are deleted
during the following round preserving 30 low priority
cookies according to the quota of those specific cookies.
For a bigger example that fully complies with the
implementation, check the unittests.

Before the fix, the unittests were just adjusted to the
behavior of cookies eviction.

For example, if we take the following unittest:
Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
TestPriorityCookieCase(cm.get(), "11HN 10MN 20LN 110MN 20LN
10HN", 20U,109U, 21U, 150U, 0U);
The problem here was that there were only 40 low priority
cookies, but the quota was not preserved for those cookies.
First, 10 low priority cookies were deleted and then more 10
low priority cookies were deleted leaving only 20 of them,
which was less than the quota (30 low priority cookies).
It happened because the eviction algorithm didn't know how
many cookies of a specific priority were deleted and
it had always started to delete all the cookies from the
beginning of the container removing even those cookies
that shouldn't have been deleted.

After we land this CL, we can have cookies in any order and
high priority cookies will be eventually deleted in order
to avoid excess of high priority cookies by some
applications within the same domain. Thus, after the
eviction algorithm runs, we should have at least 30 low, 50
mid and 70 high priority cookies if we had sufficient
amount of them in the beginning.

BUG= 609550 

Review-Url: https://codereview.chromium.org/1976073002
Cr-Commit-Position: refs/heads/master@{#398049}

[modify] https://crrev.com/fe1fea6df040fff38a3a4567e20a3d5847e83170/net/cookies/cookie_monster.cc
[modify] https://crrev.com/fe1fea6df040fff38a3a4567e20a3d5847e83170/net/cookies/cookie_monster_unittest.cc

Comment 6 by jww@chromium.org, Jun 6 2016

Status: Fixed (was: Available)
Thanks, Maksim, for the hard work on this!
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b89069d3f1167269aaf9fd117e743858475c3a37

commit b89069d3f1167269aaf9fd117e743858475c3a37
Author: mmenke <mmenke@chromium.org>
Date: Wed Jun 15 18:39:47 2016

Revert of Making cookies eviction quotas match spec (patchset #5 id:180001 of https://codereview.chromium.org/1976073002/ )

Reason for revert:
Experimentally reverting, seems to have possibly caused a perf regression.  See  https://crbug.com/618679 

Original issue's description:
> Making cookies eviction quotas match spec
>
> According to the section 3 and section 5 in
> https://tools.ietf.org/html/draft-west-cookie-priority-
> 00#section-3, the old cookie monster implementation doesn't
> maintain priority quotas correctly.
>
> This CL makes sure there will always be at least 30 low
> priority cookies, 50 mid priority and 70 high priority
> cookies if there had been enough of them before  the eviction.
> Please note, secure cookies are more important than non-secure
> per https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone.
>
> A small example: if there are 70 cookies:
> 37 non-secure low priority and 33 secure low priority
> cookies, 37 non-secure cookies are deleted during the first
> round and other 3 secure low priority cookies are deleted
> during the following round preserving 30 low priority
> cookies according to the quota of those specific cookies.
> For a bigger example that fully complies with the
> implementation, check the unittests.
>
> Before the fix, the unittests were just adjusted to the
> behavior of cookies eviction.
>
> For example, if we take the following unittest:
> Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
> TestPriorityCookieCase(cm.get(), "11HN 10MN 20LN 110MN 20LN
> 10HN", 20U,109U, 21U, 150U, 0U);
> The problem here was that there were only 40 low priority
> cookies, but the quota was not preserved for those cookies.
> First, 10 low priority cookies were deleted and then more 10
> low priority cookies were deleted leaving only 20 of them,
> which was less than the quota (30 low priority cookies).
> It happened because the eviction algorithm didn't know how
> many cookies of a specific priority were deleted and
> it had always started to delete all the cookies from the
> beginning of the container removing even those cookies
> that shouldn't have been deleted.
>
> After we land this CL, we can have cookies in any order and
> high priority cookies will be eventually deleted in order
> to avoid excess of high priority cookies by some
> applications within the same domain. Thus, after the
> eviction algorithm runs, we should have at least 30 low, 50
> mid and 70 high priority cookies if we had sufficient
> amount of them in the beginning.
>
> BUG= 609550 
>
> Committed: https://crrev.com/fe1fea6df040fff38a3a4567e20a3d5847e83170
> Cr-Commit-Position: refs/heads/master@{#398049}

TBR=jww@chromium.org,mkwst@chromium.org,maksim.sisov@intel.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 609550 

Review-Url: https://codereview.chromium.org/2071593002
Cr-Commit-Position: refs/heads/master@{#399971}

[modify] https://crrev.com/b89069d3f1167269aaf9fd117e743858475c3a37/net/cookies/cookie_monster.cc
[modify] https://crrev.com/b89069d3f1167269aaf9fd117e743858475c3a37/net/cookies/cookie_monster_unittest.cc

Sign in to add a comment