Making cookies eviction quotas match spec

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}

[maksims (do not use this acc), 2016-05-13 04:55:13]

Description was changed from

==========
example of cookies' eviction

BUG=
==========

to

==========
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-00#section-3, I think
current implementation does things wrong. If you check old test case, it turns
out that quota of cookies is not preserved.

For instance, with this example and unittests, there will always be at least 30
low prio cookies, 50 mid and 70 high if there, of course, has been enough of
them before the eviction.

What is more, current implementation and unittests are just adjusted in
accordance to each other.

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). Just changing the
order of cookies, changes the logic of how it should work.

Consider the following tests as well -

// Round 1 => 20L; round 2 => 0; round 3 => 11H
    TestPriorityCookieCase(cm.get(), "131HN 50LN", 30U, 0U, 120U, 150U, 0U);

This test does not fail in the old implementation, because high cookies are at
the beginning of the cookies vector (we know that eviction goes from least
accessed). But if we do it like this 

// Round 1 => 20L; round 2 => 0; round 3 => 11L
    TestPriorityCookieCase(cm.get(), "50LN 131HN", 30U, 0U, 120U, 150U, 0U);

The test fails. Because, as I previously said it ruins all the logic behind the
eviction. Thus, many current test cases are just adjusted according to the
implementation. 

In my proposal, 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, if I understand the RFC draft right,
after the eviction algorithm, we should have at least 30 low, 50 mid and 70 high
priority cookies if we had sufficient amount of them in the beginning. You can
see what I mean in the unittests.




BUG=609550
==========

[maksims (do not use this acc), 2016-05-13 04:55:13]

maksim.sisov@intel.com changed reviewers:
+ jww@chromium.org, mmenke@chromium.org

[maksims (do not use this acc), 2016-05-13 04:56:46]

Description was changed from

==========
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-00#section-3, I think
current implementation does things wrong. If you check old test case, it turns
out that quota of cookies is not preserved.

For instance, with this example and unittests, there will always be at least 30
low prio cookies, 50 mid and 70 high if there, of course, has been enough of
them before the eviction.

What is more, current implementation and unittests are just adjusted in
accordance to each other.

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). Just changing the
order of cookies, changes the logic of how it should work.

Consider the following tests as well -

// Round 1 => 20L; round 2 => 0; round 3 => 11H
    TestPriorityCookieCase(cm.get(), "131HN 50LN", 30U, 0U, 120U, 150U, 0U);

This test does not fail in the old implementation, because high cookies are at
the beginning of the cookies vector (we know that eviction goes from least
accessed). But if we do it like this 

// Round 1 => 20L; round 2 => 0; round 3 => 11L
    TestPriorityCookieCase(cm.get(), "50LN 131HN", 30U, 0U, 120U, 150U, 0U);

The test fails. Because, as I previously said it ruins all the logic behind the
eviction. Thus, many current test cases are just adjusted according to the
implementation. 

In my proposal, 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, if I understand the RFC draft right,
after the eviction algorithm, we should have at least 30 low, 50 mid and 70 high
priority cookies if we had sufficient amount of them in the beginning. You can
see what I mean in the unittests.




BUG=609550
==========

to

==========
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-00#section-3, I think
current implementation does things wrong. If you check old test case, it turns
out that quota of cookies is not preserved.

For instance, with this example and unittests, there will always be at least 30
low prio cookies, 50 mid and 70 high if there, of course, has been enough of
them before the eviction.

What is more, current implementation and unittests are just adjusted in
accordance to each other.

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). Just changing the
order of cookies, changes the logic of how it should work.

Consider the following tests as well -

// Round 1 => 20L; round 2 => 0; round 3 => 11H
    TestPriorityCookieCase(cm.get(), "131HN 50LN", 30U, 0U, 120U, 150U, 0U);

This test does not fail in the old implementation, because high cookies are at
the beginning of the cookies vector (we know that eviction goes from least
accessed). But if we do it like this 

// Round 1 => 20L; round 2 => 0; round 3 => 11L
    TestPriorityCookieCase(cm.get(), "50LN 131HN", 30U, 0U, 120U, 150U, 0U);

The test fails. Because, as I previously said it ruins all the logic behind the
eviction. Thus, many current test cases are just adjusted according to the
implementation. 

In my proposal, 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, if I understand the RFC draft right,
after the eviction algorithm, we should have at least 30 low, 50 mid and 70 high
priority cookies if we had sufficient amount of them in the beginning. You can
see what I mean in the unittests.

PS this is just a quick example and secure cookies are not considered. I just
wanted to demonstrate the logic of the algorithm that complies with the RFC
where each priority cookies quota is preserved.



BUG=609550
==========

[maksims (do not use this acc), 2016-05-13 04:57:07]

please take a look at this example

[maksims (do not use this acc), 2016-05-13 05:00:02]

maksim.sisov@intel.com changed reviewers:
+ mkwst@chromium.org

[maksims (do not use this acc), 2016-05-13 05:05:12]

Description was changed from

==========
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-00#section-3, I think
current implementation does things wrong. If you check old test case, it turns
out that quota of cookies is not preserved.

For instance, with this example and unittests, there will always be at least 30
low prio cookies, 50 mid and 70 high if there, of course, has been enough of
them before the eviction.

What is more, current implementation and unittests are just adjusted in
accordance to each other.

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). Just changing the
order of cookies, changes the logic of how it should work.

Consider the following tests as well -

// Round 1 => 20L; round 2 => 0; round 3 => 11H
    TestPriorityCookieCase(cm.get(), "131HN 50LN", 30U, 0U, 120U, 150U, 0U);

This test does not fail in the old implementation, because high cookies are at
the beginning of the cookies vector (we know that eviction goes from least
accessed). But if we do it like this 

// Round 1 => 20L; round 2 => 0; round 3 => 11L
    TestPriorityCookieCase(cm.get(), "50LN 131HN", 30U, 0U, 120U, 150U, 0U);

The test fails. Because, as I previously said it ruins all the logic behind the
eviction. Thus, many current test cases are just adjusted according to the
implementation. 

In my proposal, 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, if I understand the RFC draft right,
after the eviction algorithm, we should have at least 30 low, 50 mid and 70 high
priority cookies if we had sufficient amount of them in the beginning. You can
see what I mean in the unittests.

PS this is just a quick example and secure cookies are not considered. I just
wanted to demonstrate the logic of the algorithm that complies with the RFC
where each priority cookies quota is preserved.



BUG=609550
==========

to

==========
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-00#section-3, I think
current implementation does things wrong. If you check old test case, it turns
out that quota of cookies is not preserved.

For instance, with this example and unittests, there will always be at least 30
low prio cookies, 50 mid and 70 high if there, of course, has been enough of
them before the eviction.

What is more, current implementation and unittests are just adjusted in
accordance to each other.

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). Just changing the
order of cookies, the logic of how it should work changes as well.

Consider the following tests as well -

// Round 1 => 20L; round 2 => 0; round 3 => 11H
    TestPriorityCookieCase(cm.get(), "131HN 50LN", 30U, 0U, 120U, 150U, 0U);

This test does not fail in the old implementation, because high cookies are at
the beginning of the cookies vector (we know that eviction goes from least
accessed). But if we do it like this 

// Round 1 => 20L; round 2 => 0; round 3 => 11L
    TestPriorityCookieCase(cm.get(), "50LN 131HN", 30U, 0U, 120U, 150U, 0U);

The test fails. Because, as I previously said it ruins all the logic behind the
eviction. Thus, many current test cases are just adjusted according to the
implementation. 

In my proposal, 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, if I understand the RFC draft right,
after the eviction algorithm, we should have at least 30 low, 50 mid and 70 high
priority cookies if we had sufficient amount of them in the beginning. You can
see what I mean in the unittests.

PS this is just a quick example and secure cookies are not considered. I just
wanted to demonstrate the logic of the algorithm that complies with the RFC
where each priority cookies quota is preserved.



BUG=609550
==========

[jww, 2016-05-14 00:50:38]

On 2016/05/13 04:57:07, maksims wrote:
> please take a look at this example

Unfortunately, I think this CL punts on all of the hard questions with this
implementation, in particular how to match this all up with Strict Secure
Cookies. While the approach generally makes sense in a world without Strict
Secure Cookies, that's where stuff gets complicated. Namely, it's the issue of
of each priority round actually having two rounds (one for non-secure and one
for secure), and the fact that those rounds may not be sequential (e.g. the
medium and high non-secure rounds both occur before the medium and high secure
rounds).

Additionally, given that we've never done this with priority, and, to my
understanding, Chrome is the only user agent to implement priorities, we should
have more discussions about whether the spec is correct. IMO, it probably is,
but we should think this out a bit more thoroughly before implementing this
change, since it would actually be a behavior change for Chrome.

In any case, before we go much further with this or a similar CL, I'd love to
see a comment or design of an algorithm that would solve this for the Strict
Secure Cookies case.

And just so we're clear and this doesn't accidentally get added to the commit
queue, this is not lgtm as it has commented out all of our Strict Secure Cookie
logic.

[maksims (do not use this acc), 2016-05-16 04:41:34]

On 2016/05/14 00:50:38, jww wrote:
> On 2016/05/13 04:57:07, maksims wrote:
> > please take a look at this example
> 
> Unfortunately, I think this CL punts on all of the hard questions with this
> implementation, in particular how to match this all up with Strict Secure
> Cookies. While the approach generally makes sense in a world without Strict
> Secure Cookies, that's where stuff gets complicated. Namely, it's the issue of
> of each priority round actually having two rounds (one for non-secure and one
> for secure), and the fact that those rounds may not be sequential (e.g. the
> medium and high non-secure rounds both occur before the medium and high secure
> rounds).
> 
> Additionally, given that we've never done this with priority, and, to my
> understanding, Chrome is the only user agent to implement priorities, we
should
> have more discussions about whether the spec is correct. IMO, it probably is,
> but we should think this out a bit more thoroughly before implementing this
> change, since it would actually be a behavior change for Chrome.
> 
> In any case, before we go much further with this or a similar CL, I'd love to
> see a comment or design of an algorithm that would solve this for the Strict
> Secure Cookies case.
> 
> And just so we're clear and this doesn't accidentally get added to the commit
> queue, this is not lgtm as it has commented out all of our Strict Secure
Cookie
> logic.

The point was not to get this cl approved (see the PS in the CL) rather than to
show basic eviction algorithm which allows to have the cookies in what ever
order they are and to have strict quota preserved for sure. I will work on that
a bit more and present the logic with Strict Secure Cookies as well.

[maksims (do not use this acc), 2016-05-16 12:52:49]

Patchset #2 (id:20001) has been deleted

[maksims (do not use this acc), 2016-05-16 12:57:07]

Patchset #2 (id:40001) has been deleted

[maksims (do not use this acc), 2016-05-16 12:58:25]

ptal.

This is how I thing it should work.

It preserves the quota for cookies and no application is starving for cookies.
Should you have questions, I am here to ask.

[maksims (do not use this acc), 2016-05-16 12:58:42]

On 2016/05/16 12:58:25, maksims wrote:
> ptal.
> 
> This is how I thing it should work.
> 
> It preserves the quota for cookies and no application is starving for cookies.
> Should you have questions, I am here to ask.

*to answer

[mmenke, 2016-05-17 17:21:13]

On 2016/05/16 12:58:42, maksims wrote:
> On 2016/05/16 12:58:25, maksims wrote:
> > ptal.
> > 
> > This is how I thing it should work.
> > 
> > It preserves the quota for cookies and no application is starving for
cookies.
> > Should you have questions, I am here to ask.
> 
> *to answer

I'm going to defer to mkwst and jww on this, who both know a lot more about
cookie priorities than I do (May take a look more from a code health standpoint,
if I can find the time, but probably best to iron out the algorithm first)

[jww, 2016-05-17 17:51:06]

On 2016/05/17 17:21:13, mmenke wrote:
> On 2016/05/16 12:58:42, maksims wrote:
> > On 2016/05/16 12:58:25, maksims wrote:
> > > ptal.
> > > 
> > > This is how I thing it should work.
> > > 
> > > It preserves the quota for cookies and no application is starving for
> cookies.
> > > Should you have questions, I am here to ask.
> > 
> > *to answer
> 
> I'm going to defer to mkwst and jww on this, who both know a lot more about
> cookie priorities than I do (May take a look more from a code health
standpoint,
> if I can find the time, but probably best to iron out the algorithm first)

I started taking a look but will probably be unable to take a full look until
tomorrow.

[maksims (do not use this acc), 2016-05-20 05:29:27]

maksim.sisov@intel.com changed reviewers:
- mmenke@chromium.org

[maksims (do not use this acc), 2016-05-20 05:29:27]

gentle reminder

[maksims (do not use this acc), 2016-05-20 05:29:40]

maksim.sisov@intel.com changed reviewers:
+ mmenke@chromium.org

[maksims (do not use this acc), 2016-05-20 07:42:52]

Patchset #3 (id:80001) has been deleted

[jww, 2016-05-23 23:19:03]

Thanks for this CL. I've added in a bunch of clarifications and questions. I
haven't gone over the tests yet; I'm going to wait until we're convinced that
the logic is basically correct.

Also, I'm still pretty concerned about breaking what has been "working" already
for a while, even though it differs from the spec. If we get the CL to a
spec-compliant place, we'll then have to still make the decision that it's the
right thing to do.

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:316: for (const auto& cookie : *cookies) {
Can't this loop be simplified now to just:
if (cookie->second->IsSecure() && cookie_priority == cookie->second->Priority())
  num_protected_secure_cookies++;

And, assuming that's true, the comments below should also be updated
accordingly.

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:346: if (cookie->Priority() ==
current_priority_level && cookie->IsSecure() &&
These two conditionals can be simplified to:

if (cookie->Priority() == current_priority_level && protect_secure_cookies)
  return !cookie->IsSecure();

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1980: quota = kDomainCookiesQuotaLow;
This is the change that I'm most confused about. Doesn't this mean that, for
example, in the second round where we're looking at low-priority, secure
cookies, the quota gets reset, so we potentially delete an entire new
quota-number of cookies? Or am I misunderstanding something?

For example, if I have 40 low-priority, non-secure cookies and 40 low-priority,
secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds go:
1. Delete 10 low-priority, non-secure cookies
2. Delete 20 low-priority, secure cookies
3. Do nothing
4. Do nothing
5. Do nothing
6. Delete 130 high priority, secure cookies (or whatever it is to get down to
the final purge goal)

Hmm, looking below, I think I *may* have misunderstood how this works because I
*think* what you're going for is that you'll count the secure cookies as
"protected" already in Purge...(). Can you clarify?

At the very least, I think this comment needs updating.

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2059: size_t cookies_count_possibly_to_be_deleted
=
While this logic is generally sound, it might be good to simplify the
CountCookiesAtPriority() and CountProtectedSecureCookiesAtPriority() functions
into a single CountCookiesForPossibleDeletion() that takes
protect_secure_cookies as an argument.

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2073: cookies_count_possibly_to_be_deleted -=
secure_cookies;
What if secure_cookies < to_protect? Don't you still need to reduce
cookies_count_possibly_to_be_deleted to by to_protect - secure_cookies?

[maksims (do not use this acc), 2016-05-24 06:23:05]

Patchset #3 (id:100001) has been deleted

[maksims (do not use this acc), 2016-05-24 06:24:57]

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:316: for (const auto& cookie : *cookies) {
On 2016/05/23 23:19:03, jww wrote:
> Can't this loop be simplified now to just:
> if (cookie->second->IsSecure() && cookie_priority ==
cookie->second->Priority())
>   num_protected_secure_cookies++;
> 
> And, assuming that's true, the comments below should also be updated
> accordingly.

Right. Done.

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:346: if (cookie->Priority() ==
current_priority_level && cookie->IsSecure() &&
On 2016/05/23 23:19:03, jww wrote:
> These two conditionals can be simplified to:
> 
> if (cookie->Priority() == current_priority_level && protect_secure_cookies)
>   return !cookie->IsSecure();

Done.

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1980: quota = kDomainCookiesQuotaLow;
On 2016/05/23 23:19:03, jww wrote:
> This is the change that I'm most confused about. Doesn't this mean that, for
> example, in the second round where we're looking at low-priority, secure
> cookies, the quota gets reset, so we potentially delete an entire new
> quota-number of cookies? Or am I misunderstanding something?
> 
> For example, if I have 40 low-priority, non-secure cookies and 40
low-priority,
> secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds go:
> 1. Delete 10 low-priority, non-secure cookies
> 2. Delete 20 low-priority, secure cookies
> 3. Do nothing
> 4. Do nothing
> 5. Do nothing
> 6. Delete 130 high priority, secure cookies (or whatever it is to get down to
> the final purge goal)
> 
> Hmm, looking below, I think I *may* have misunderstood how this works because
I
> *think* what you're going for is that you'll count the secure cookies as
> "protected" already in Purge...(). Can you clarify?
> 
> At the very least, I think this comment needs updating.


Not exactly. First, we take a quota at |priority| that we mustn't delete (30 for
low priority cookies, for example).
Then we count secure and non-secure cookies at |priority|.
If the total number of cookies at |priority| is lower than the quota, we skip
the round, which guarantees we will not go below than the quota and there will
be no starvation for cookies at the |priority|.
It is the line 2063, which does the trick:
if (cookies_count_possibly_to_be_deleted <= to_protect)
return 0u;

Then, if we should protect low-priority secure cookies, we count and subtract
them from the overall number of cookies that cannot be lower than 31 because we
have already checked the number of cookies at line 2063. Afterwards, we delete
the non-secure cookies till purge goal is hit or number of those cookies is 0.

Next round does the same and deletes low-priority secure cookies if removing
low-priority non-secure cookies has not been enough to hit the quota (30 for
low-priority cookies).

> For example, if I have 40 low-priority, non-secure cookies and 40
low-priority,
> secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds go:

1. Delete 40 low-priority, non-secure cookies, 0 left
2. Delete 10 low-priority, secure cookies, 30 left
----> preserve quota of 30 low-priority cookies.
3. Do nothing
4. Do nothing
5. Do nothing
if total number of cookies shouldn't be less than 180, then
6. Delete 150 high priority secure cookies, which preserves 70 high-priority
cookies according to a quota of 70 high priority cookies.

As a result there are 30 low priority secure cookies and 150 high priority
secure cookies.

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2059: size_t cookies_count_possibly_to_be_deleted
=
On 2016/05/23 23:19:03, jww wrote:
> While this logic is generally sound, it might be good to simplify the
> CountCookiesAtPriority() and CountProtectedSecureCookiesAtPriority() functions
> into a single CountCookiesForPossibleDeletion() that takes
> protect_secure_cookies as an argument.

Hmm, ok!

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2073: cookies_count_possibly_to_be_deleted -=
secure_cookies;
On 2016/05/23 23:19:03, jww wrote:
> What if secure_cookies < to_protect? Don't you still need to reduce
> cookies_count_possibly_to_be_deleted to by to_protect - secure_cookies?

It doesn't matter. |secure_cookies| are subtracted from the total number of
cookies in order to avoid removing them on the low priority non-secure cookies
round. For example, there are 90 low priority cookies: 55 low non-secure and 35
secure cookies. Line 2063 guarantees we will not go below than the quota, which
is 30 low priority cookies. 

cookies_count_possibly_to_be_deleted = 90
check cookies_count_possibly_to_be_deleted <= to_protect or quota
protect_secure_cookies is true,
then secure_cookies = CountProtectedSecureCookiesAtPriority(priority, cookies) =
35.
cookies_count_possibly_to_be_deleted = 90 - 35 = 55 low non-secure cookies.

The "while loop" deletes 55 low non-secure cookies.
Next round: 
cookies_count_possibly_to_be_deleted = 35.
check cookies_count_possibly_to_be_deleted <= to_protect or quota (35 <= 30)
then cookies_count_possibly_to_be_deleted = 35 - 30 = 5 low priority secure
cookies to be deleted.

[jww, 2016-05-25 00:29:20]

On 2016/05/24 06:24:57, maksims wrote:
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> File net/cookies/cookie_monster.cc (right):
> 
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:316: for (const auto& cookie : *cookies) {
> On 2016/05/23 23:19:03, jww wrote:
> > Can't this loop be simplified now to just:
> > if (cookie->second->IsSecure() && cookie_priority ==
> cookie->second->Priority())
> >   num_protected_secure_cookies++;
> > 
> > And, assuming that's true, the comments below should also be updated
> > accordingly.
> 
> Right. Done.
> 
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:346: if (cookie->Priority() ==
> current_priority_level && cookie->IsSecure() &&
> On 2016/05/23 23:19:03, jww wrote:
> > These two conditionals can be simplified to:
> > 
> > if (cookie->Priority() == current_priority_level && protect_secure_cookies)
> >   return !cookie->IsSecure();
> 
> Done.
> 
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:1980: quota = kDomainCookiesQuotaLow;
> On 2016/05/23 23:19:03, jww wrote:
> > This is the change that I'm most confused about. Doesn't this mean that, for
> > example, in the second round where we're looking at low-priority, secure
> > cookies, the quota gets reset, so we potentially delete an entire new
> > quota-number of cookies? Or am I misunderstanding something?
> > 
> > For example, if I have 40 low-priority, non-secure cookies and 40
> low-priority,
> > secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds
go:
> > 1. Delete 10 low-priority, non-secure cookies
> > 2. Delete 20 low-priority, secure cookies
> > 3. Do nothing
> > 4. Do nothing
> > 5. Do nothing
> > 6. Delete 130 high priority, secure cookies (or whatever it is to get down
to
> > the final purge goal)
> > 
> > Hmm, looking below, I think I *may* have misunderstood how this works
because
> I
> > *think* what you're going for is that you'll count the secure cookies as
> > "protected" already in Purge...(). Can you clarify?
> > 
> > At the very least, I think this comment needs updating.
> 
> 
> Not exactly. First, we take a quota at |priority| that we mustn't delete (30
for
> low priority cookies, for example).
> Then we count secure and non-secure cookies at |priority|.
> If the total number of cookies at |priority| is lower than the quota, we skip
> the round, which guarantees we will not go below than the quota and there will
> be no starvation for cookies at the |priority|.
> It is the line 2063, which does the trick:
> if (cookies_count_possibly_to_be_deleted <= to_protect)
> return 0u;
> 
> Then, if we should protect low-priority secure cookies, we count and subtract
> them from the overall number of cookies that cannot be lower than 31 because
we
> have already checked the number of cookies at line 2063. Afterwards, we delete
> the non-secure cookies till purge goal is hit or number of those cookies is 0.
> 
> Next round does the same and deletes low-priority secure cookies if removing
> low-priority non-secure cookies has not been enough to hit the quota (30 for
> low-priority cookies).
> 
> > For example, if I have 40 low-priority, non-secure cookies and 40
> low-priority,
> > secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds
go:
> 
> 1. Delete 40 low-priority, non-secure cookies, 0 left
> 2. Delete 10 low-priority, secure cookies, 30 left
> ----> preserve quota of 30 low-priority cookies.
> 3. Do nothing
> 4. Do nothing
> 5. Do nothing
> if total number of cookies shouldn't be less than 180, then
> 6. Delete 150 high priority secure cookies, which preserves 70 high-priority
> cookies according to a quota of 70 high priority cookies.
> 
> As a result there are 30 low priority secure cookies and 150 high priority
> secure cookies.
> 
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:2059: size_t
cookies_count_possibly_to_be_deleted
> =
> On 2016/05/23 23:19:03, jww wrote:
> > While this logic is generally sound, it might be good to simplify the
> > CountCookiesAtPriority() and CountProtectedSecureCookiesAtPriority()
functions
> > into a single CountCookiesForPossibleDeletion() that takes
> > protect_secure_cookies as an argument.
> 
> Hmm, ok!
> 
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:2073: cookies_count_possibly_to_be_deleted -=
> secure_cookies;
> On 2016/05/23 23:19:03, jww wrote:
> > What if secure_cookies < to_protect? Don't you still need to reduce
> > cookies_count_possibly_to_be_deleted to by to_protect - secure_cookies?
> 
> It doesn't matter. |secure_cookies| are subtracted from the total number of
> cookies in order to avoid removing them on the low priority non-secure cookies
> round. For example, there are 90 low priority cookies: 55 low non-secure and
35
> secure cookies. Line 2063 guarantees we will not go below than the quota,
which
> is 30 low priority cookies. 
> 
> cookies_count_possibly_to_be_deleted = 90
> check cookies_count_possibly_to_be_deleted <= to_protect or quota
> protect_secure_cookies is true,
> then secure_cookies = CountProtectedSecureCookiesAtPriority(priority, cookies)
=
> 35.
> cookies_count_possibly_to_be_deleted = 90 - 35 = 55 low non-secure cookies.
> 
> The "while loop" deletes 55 low non-secure cookies.
> Next round: 
> cookies_count_possibly_to_be_deleted = 35.
> check cookies_count_possibly_to_be_deleted <= to_protect or quota (35 <= 30)
> then cookies_count_possibly_to_be_deleted = 35 - 30 = 5 low priority secure
> cookies to be deleted.

Sorry I haven't gotten back to you today on this; I've got a bunch on my plate
over the next 24 hours. Will get back to you tomorrow afternoon.

[jww, 2016-05-26 03:46:39]

This is looking like it's on the right track, but I still have some questions.
Thanks for the help on this!

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1980: quota = kDomainCookiesQuotaLow;
On 2016/05/24 06:24:57, maksims wrote:
> On 2016/05/23 23:19:03, jww wrote:
> > This is the change that I'm most confused about. Doesn't this mean that, for
> > example, in the second round where we're looking at low-priority, secure
> > cookies, the quota gets reset, so we potentially delete an entire new
> > quota-number of cookies? Or am I misunderstanding something?
> > 
> > For example, if I have 40 low-priority, non-secure cookies and 40
> low-priority,
> > secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds
go:
> > 1. Delete 10 low-priority, non-secure cookies
> > 2. Delete 20 low-priority, secure cookies
> > 3. Do nothing
> > 4. Do nothing
> > 5. Do nothing
> > 6. Delete 130 high priority, secure cookies (or whatever it is to get down
to
> > the final purge goal)
> > 
> > Hmm, looking below, I think I *may* have misunderstood how this works
because
> I
> > *think* what you're going for is that you'll count the secure cookies as
> > "protected" already in Purge...(). Can you clarify?
> > 
> > At the very least, I think this comment needs updating.
> 
> 
> Not exactly. First, we take a quota at |priority| that we mustn't delete (30
for
> low priority cookies, for example).
> Then we count secure and non-secure cookies at |priority|.
> If the total number of cookies at |priority| is lower than the quota, we skip
> the round, which guarantees we will not go below than the quota and there will
> be no starvation for cookies at the |priority|.
> It is the line 2063, which does the trick:
> if (cookies_count_possibly_to_be_deleted <= to_protect)
> return 0u;
> 
> Then, if we should protect low-priority secure cookies, we count and subtract
> them from the overall number of cookies that cannot be lower than 31 because
we
> have already checked the number of cookies at line 2063. Afterwards, we delete
> the non-secure cookies till purge goal is hit or number of those cookies is 0.
> 
> Next round does the same and deletes low-priority secure cookies if removing
> low-priority non-secure cookies has not been enough to hit the quota (30 for
> low-priority cookies).
> 
> > For example, if I have 40 low-priority, non-secure cookies and 40
> low-priority,
> > secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds
go:
> 
> 1. Delete 40 low-priority, non-secure cookies, 0 left
> 2. Delete 10 low-priority, secure cookies, 30 left
> ----> preserve quota of 30 low-priority cookies.
> 3. Do nothing
> 4. Do nothing
> 5. Do nothing
> if total number of cookies shouldn't be less than 180, then
> 6. Delete 150 high priority secure cookies, which preserves 70 high-priority
> cookies according to a quota of 70 high priority cookies.
> 
> As a result there are 30 low priority secure cookies and 150 high priority
> secure cookies. 
> 

Ok, I've got it. As mentioned before, can you update the comment about the
comment here? I believe it's no longer accurate. At the very least, your
explanation you just gave was very helpful, and it would be useful to have a
shorter version of it somewhere in the comments :-)

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2069: // |priority_cookies_might_be_deleted| is
higher.
This variable name changed, correct?

https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2073: cookies_count_possibly_to_be_deleted -=
secure_cookies;
On 2016/05/24 06:24:57, maksims wrote:
> On 2016/05/23 23:19:03, jww wrote:
> > What if secure_cookies < to_protect? Don't you still need to reduce
> > cookies_count_possibly_to_be_deleted to by to_protect - secure_cookies?
> 
> It doesn't matter. |secure_cookies| are subtracted from the total number of
> cookies in order to avoid removing them on the low priority non-secure cookies
> round. For example, there are 90 low priority cookies: 55 low non-secure and
35
> secure cookies. Line 2063 guarantees we will not go below than the quota,
which
> is 30 low priority cookies. 
> 
> cookies_count_possibly_to_be_deleted = 90
> check cookies_count_possibly_to_be_deleted <= to_protect or quota
> protect_secure_cookies is true,
> then secure_cookies = CountProtectedSecureCookiesAtPriority(priority, cookies)
=
> 35.
> cookies_count_possibly_to_be_deleted = 90 - 35 = 55 low non-secure cookies.
> 
> The "while loop" deletes 55 low non-secure cookies.
> Next round: 
> cookies_count_possibly_to_be_deleted = 35.
> check cookies_count_possibly_to_be_deleted <= to_protect or quota (35 <= 30)
> then cookies_count_possibly_to_be_deleted = 35 - 30 = 5 low priority secure
> cookies to be deleted.
> 


What if there are 90 low priority, non-secure cookies and 0 secure cookies? In
that case, in the first round:
cookies_count_possibly_to_be_deleted = 90
secure_cookies = CountProtectedSecureCookiesAtPriority(...), which is 0
cookies_count_possibly_to_be_deleted = 90 - 0

which sets up the round to delete 90 cookies, which is all of them, thus not
maintaining the quota. So I believe line 2073 should be:
cookies_count_possibly_to_be_deleted -= std::max(secure_cookies, to_protect -
secure_cookies)

Am I still misunderstanding?

https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
net/cookies/cookie_monster.cc:324: bool protect_secure_cookies = false) {
Please do not use a default argument here; it caused me a bit of confusion
below, especially since there are only two call sites, so this doesn't help with
an exceptional case (just pass in protect_secure_cookies at the two call sites
and it will be much clearer).

https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
net/cookies/cookie_monster.cc:327: if (cookie->second->Priority() == priority &&
!protect_secure_cookies)
Please simplify this conditional. I'm finding the two paths to cookies_count++ a
bit hard to follow. Maybe something like:

if (cookie->second->Priority() == priority) {
  if (!protect_secure_cookies || cookie->second->IsSecure())
    cookies_count++;
}

Also, as a style note, you should have braces on both branches of the
conditional. According to the style guide, braces on one conditional means
braces on all branches.

[maksims (do not use this acc), 2016-05-26 07:53:10]

On 2016/05/26 03:46:39, jww wrote:
> This is looking like it's on the right track, but I still have some questions.
> Thanks for the help on this!
> 
Thanks!
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> File net/cookies/cookie_monster.cc (right):
> 
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:1980: quota = kDomainCookiesQuotaLow;
> On 2016/05/24 06:24:57, maksims wrote:
> > On 2016/05/23 23:19:03, jww wrote:
> > > This is the change that I'm most confused about. Doesn't this mean that,
for
> > > example, in the second round where we're looking at low-priority, secure
> > > cookies, the quota gets reset, so we potentially delete an entire new
> > > quota-number of cookies? Or am I misunderstanding something?
> > > 
> > > For example, if I have 40 low-priority, non-secure cookies and 40
> > low-priority,
> > > secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds
> go:
> > > 1. Delete 10 low-priority, non-secure cookies
> > > 2. Delete 20 low-priority, secure cookies
> > > 3. Do nothing
> > > 4. Do nothing
> > > 5. Do nothing
> > > 6. Delete 130 high priority, secure cookies (or whatever it is to get down
> to
> > > the final purge goal)
> > > 
> > > Hmm, looking below, I think I *may* have misunderstood how this works
> because
> > I
> > > *think* what you're going for is that you'll count the secure cookies as
> > > "protected" already in Purge...(). Can you clarify?
> > > 
> > > At the very least, I think this comment needs updating.
> > 
> > 
> > Not exactly. First, we take a quota at |priority| that we mustn't delete (30
> for
> > low priority cookies, for example).
> > Then we count secure and non-secure cookies at |priority|.
> > If the total number of cookies at |priority| is lower than the quota, we
skip
> > the round, which guarantees we will not go below than the quota and there
will
> > be no starvation for cookies at the |priority|.
> > It is the line 2063, which does the trick:
> > if (cookies_count_possibly_to_be_deleted <= to_protect)
> > return 0u;
> > 
> > Then, if we should protect low-priority secure cookies, we count and
subtract
> > them from the overall number of cookies that cannot be lower than 31 because
> we
> > have already checked the number of cookies at line 2063. Afterwards, we
delete
> > the non-secure cookies till purge goal is hit or number of those cookies is
0.
> > 
> > Next round does the same and deletes low-priority secure cookies if removing
> > low-priority non-secure cookies has not been enough to hit the quota (30 for
> > low-priority cookies).
> > 
> > > For example, if I have 40 low-priority, non-secure cookies and 40
> > low-priority,
> > > secure cookies, and 300 high-priority, secure cookies, wouldn't the rounds
> go:
> > 
> > 1. Delete 40 low-priority, non-secure cookies, 0 left
> > 2. Delete 10 low-priority, secure cookies, 30 left
> > ----> preserve quota of 30 low-priority cookies.
> > 3. Do nothing
> > 4. Do nothing
> > 5. Do nothing
> > if total number of cookies shouldn't be less than 180, then
> > 6. Delete 150 high priority secure cookies, which preserves 70 high-priority
> > cookies according to a quota of 70 high priority cookies.
> > 
> > As a result there are 30 low priority secure cookies and 150 high priority
> > secure cookies. 
> > 
> 
> Ok, I've got it. As mentioned before, can you update the comment about the
> comment here? I believe it's no longer accurate. At the very least, your
> explanation you just gave was very helpful, and it would be useful to have a
> shorter version of it somewhere in the comments :-)
> 

Ok! And by the way, it is not possible to have 80 non-sec and sec low priority
cookies
and 300 high priority, secure cookies. As soon as the GC notices there are 181
cookies,
it immediately starts removing the cookies. Thus, it will go like this - 
40 non-secure and 40 secure cookies are uploaded, then when additional 101 high
priority secure 
cookies are uploaded, it starts all the 6 rounds -
1. Delete 31 low non-sec -> skip other rounds
Then more high priority secure cookies arrived (131 to be exact), we have 9
non-sec low, 40 sec low 
priority cookies and 132 high priority secure cookies arrive, which results in
181 all together.
GC starts 6 rounds
1. Delete 9 low non-sec
2. Delete 10 low sec
3-5. none
6. Delete 12 high secure cookies
And so on. It was not me, It had already worked like this.

check the TestPriorityCookieCase(cm.get(), "100HS 100LN 100MN", 30U, 76U, 70U,
106U, 70U); unittests.
I added comments how rounds should go.

>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:2069: // |priority_cookies_might_be_deleted| is
> higher.
> This variable name changed, correct?
> 
right. Done.
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:2073: cookies_count_possibly_to_be_deleted -=
> secure_cookies;
> On 2016/05/24 06:24:57, maksims wrote:
> > On 2016/05/23 23:19:03, jww wrote:
> > > What if secure_cookies < to_protect? Don't you still need to reduce
> > > cookies_count_possibly_to_be_deleted to by to_protect - secure_cookies?
> > 
> > It doesn't matter. |secure_cookies| are subtracted from the total number of
> > cookies in order to avoid removing them on the low priority non-secure
cookies
> > round. For example, there are 90 low priority cookies: 55 low non-secure and
> 35
> > secure cookies. Line 2063 guarantees we will not go below than the quota,
> which
> > is 30 low priority cookies. 
> > 
> > cookies_count_possibly_to_be_deleted = 90
> > check cookies_count_possibly_to_be_deleted <= to_protect or quota
> > protect_secure_cookies is true,
> > then secure_cookies = CountProtectedSecureCookiesAtPriority(priority,
cookies)
> =
> > 35.
> > cookies_count_possibly_to_be_deleted = 90 - 35 = 55 low non-secure cookies.
> > 
> > The "while loop" deletes 55 low non-secure cookies.
> > Next round: 
> > cookies_count_possibly_to_be_deleted = 35.
> > check cookies_count_possibly_to_be_deleted <= to_protect or quota (35 <= 30)
> > then cookies_count_possibly_to_be_deleted = 35 - 30 = 5 low priority secure
> > cookies to be deleted.
> > 
> 
> 
> What if there are 90 low priority, non-secure cookies and 0 secure cookies? In
> that case, in the first round:
> cookies_count_possibly_to_be_deleted = 90
> secure_cookies = CountProtectedSecureCookiesAtPriority(...), which is 0
> cookies_count_possibly_to_be_deleted = 90 - 0
> 
> which sets up the round to delete 90 cookies, which is all of them, thus not
> maintaining the quota. So I believe line 2073 should be:
> cookies_count_possibly_to_be_deleted -= std::max(secure_cookies, to_protect -
> secure_cookies)
> 
> Am I still misunderstanding?

I don't think that's possible, because the total number of cookies for a certain
domain
is much higher. But if that is changed some day, it would cause troubles. Better
to
explicitly check as you proposed.
Done! 

> 
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> File net/cookies/cookie_monster.cc (right):
> 
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> net/cookies/cookie_monster.cc:324: bool protect_secure_cookies = false) {
> Please do not use a default argument here; it caused me a bit of confusion
> below, especially since there are only two call sites, so this doesn't help
with
> an exceptional case (just pass in protect_secure_cookies at the two call sites
> and it will be much clearer).
> 
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> net/cookies/cookie_monster.cc:327: if (cookie->second->Priority() == priority
&&
> !protect_secure_cookies)
> Please simplify this conditional. I'm finding the two paths to cookies_count++
a
> bit hard to follow. Maybe something like:
> 
> if (cookie->second->Priority() == priority) {
>   if (!protect_secure_cookies || cookie->second->IsSecure())
>     cookies_count++;
> }
> 
> Also, as a style note, you should have braces on both branches of the
> conditional. According to the style guide, braces on one conditional means
> braces on all branches.

[maksims (do not use this acc), 2016-05-26 08:54:24]

Patchset #4 (id:140001) has been deleted

[maksims (do not use this acc), 2016-05-26 11:30:20]

https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
net/cookies/cookie_monster.cc:324: bool protect_secure_cookies = false) {
On 2016/05/26 03:46:39, jww wrote:
> Please do not use a default argument here; it caused me a bit of confusion
> below, especially since there are only two call sites, so this doesn't help
with
> an exceptional case (just pass in protect_secure_cookies at the two call sites
> and it will be much clearer).

Done.

https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
net/cookies/cookie_monster.cc:327: if (cookie->second->Priority() == priority &&
!protect_secure_cookies)
On 2016/05/26 03:46:39, jww wrote:
> Please simplify this conditional. I'm finding the two paths to cookies_count++
a
> bit hard to follow. Maybe something like:
> 
> if (cookie->second->Priority() == priority) {
>   if (!protect_secure_cookies || cookie->second->IsSecure())
>     cookies_count++;
> }
> 
> Also, as a style note, you should have braces on both branches of the
> conditional. According to the style guide, braces on one conditional means
> braces on all branches.

Then it will be incorrect if I will pass original |protect_secure_cookies| in
two calls. I just want to count the number of secure and non-secure cookies of a
certain priority all together. Thus, |protect_secure_cookies| must always be
false, see the first caller.

[maksims (do not use this acc), 2016-06-01 05:05:03]

gentle reminder

[maksims (do not use this acc), 2016-06-01 05:37:21]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 05:37:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976073002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976073002/160001

[commit-bot: I haz the power, 2016-06-01 06:49:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 06:49:10]

Dry run: This issue passed the CQ dry run.

[jww, 2016-06-02 00:54:52]

On 2016/06/01 06:49:10, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Thanks for the reminder, and thanks for the newest version! I'll take a look at
the latest version in the morning.

[jww, 2016-06-03 00:10:34]

On 2016/05/26 07:53:10, maksims wrote:
> On 2016/05/26 03:46:39, jww wrote:
> > This is looking like it's on the right track, but I still have some
questions.
> > Thanks for the help on this!
> > 
> Thanks!
> >
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> > File net/cookies/cookie_monster.cc (right):
> > 
> >
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> > net/cookies/cookie_monster.cc:1980: quota = kDomainCookiesQuotaLow;
> > On 2016/05/24 06:24:57, maksims wrote:
> > > On 2016/05/23 23:19:03, jww wrote:
> > > > This is the change that I'm most confused about. Doesn't this mean that,
> for
> > > > example, in the second round where we're looking at low-priority, secure
> > > > cookies, the quota gets reset, so we potentially delete an entire new
> > > > quota-number of cookies? Or am I misunderstanding something?
> > > > 
> > > > For example, if I have 40 low-priority, non-secure cookies and 40
> > > low-priority,
> > > > secure cookies, and 300 high-priority, secure cookies, wouldn't the
rounds
> > go:
> > > > 1. Delete 10 low-priority, non-secure cookies
> > > > 2. Delete 20 low-priority, secure cookies
> > > > 3. Do nothing
> > > > 4. Do nothing
> > > > 5. Do nothing
> > > > 6. Delete 130 high priority, secure cookies (or whatever it is to get
down
> > to
> > > > the final purge goal)
> > > > 
> > > > Hmm, looking below, I think I *may* have misunderstood how this works
> > because
> > > I
> > > > *think* what you're going for is that you'll count the secure cookies as
> > > > "protected" already in Purge...(). Can you clarify?
> > > > 
> > > > At the very least, I think this comment needs updating.
> > > 
> > > 
> > > Not exactly. First, we take a quota at |priority| that we mustn't delete
(30
> > for
> > > low priority cookies, for example).
> > > Then we count secure and non-secure cookies at |priority|.
> > > If the total number of cookies at |priority| is lower than the quota, we
> skip
> > > the round, which guarantees we will not go below than the quota and there
> will
> > > be no starvation for cookies at the |priority|.
> > > It is the line 2063, which does the trick:
> > > if (cookies_count_possibly_to_be_deleted <= to_protect)
> > > return 0u;
> > > 
> > > Then, if we should protect low-priority secure cookies, we count and
> subtract
> > > them from the overall number of cookies that cannot be lower than 31
because
> > we
> > > have already checked the number of cookies at line 2063. Afterwards, we
> delete
> > > the non-secure cookies till purge goal is hit or number of those cookies
is
> 0.
> > > 
> > > Next round does the same and deletes low-priority secure cookies if
removing
> > > low-priority non-secure cookies has not been enough to hit the quota (30
for
> > > low-priority cookies).
> > > 
> > > > For example, if I have 40 low-priority, non-secure cookies and 40
> > > low-priority,
> > > > secure cookies, and 300 high-priority, secure cookies, wouldn't the
rounds
> > go:
> > > 
> > > 1. Delete 40 low-priority, non-secure cookies, 0 left
> > > 2. Delete 10 low-priority, secure cookies, 30 left
> > > ----> preserve quota of 30 low-priority cookies.
> > > 3. Do nothing
> > > 4. Do nothing
> > > 5. Do nothing
> > > if total number of cookies shouldn't be less than 180, then
> > > 6. Delete 150 high priority secure cookies, which preserves 70
high-priority
> > > cookies according to a quota of 70 high priority cookies.
> > > 
> > > As a result there are 30 low priority secure cookies and 150 high priority
> > > secure cookies. 
> > > 
> > 
> > Ok, I've got it. As mentioned before, can you update the comment about the
> > comment here? I believe it's no longer accurate. At the very least, your
> > explanation you just gave was very helpful, and it would be useful to have a
> > shorter version of it somewhere in the comments :-)
> > 
> 
> Ok! And by the way, it is not possible to have 80 non-sec and sec low priority
> cookies
> and 300 high priority, secure cookies. As soon as the GC notices there are 181
> cookies,
> it immediately starts removing the cookies. Thus, it will go like this - 
> 40 non-secure and 40 secure cookies are uploaded, then when additional 101
high
> priority secure 
> cookies are uploaded, it starts all the 6 rounds -
> 1. Delete 31 low non-sec -> skip other rounds
> Then more high priority secure cookies arrived (131 to be exact), we have 9
> non-sec low, 40 sec low 
> priority cookies and 132 high priority secure cookies arrive, which results in
> 181 all together.
> GC starts 6 rounds
> 1. Delete 9 low non-sec
> 2. Delete 10 low sec
> 3-5. none
> 6. Delete 12 high secure cookies
> And so on. It was not me, It had already worked like this.
> 
> check the TestPriorityCookieCase(cm.get(), "100HS 100LN 100MN", 30U, 76U, 70U,
> 106U, 70U); unittests.
> I added comments how rounds should go.
> 
> >
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> > net/cookies/cookie_monster.cc:2069: // |priority_cookies_might_be_deleted|
is
> > higher.
> > This variable name changed, correct?
> > 
> right. Done.
> >
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> > net/cookies/cookie_monster.cc:2073: cookies_count_possibly_to_be_deleted -=
> > secure_cookies;
> > On 2016/05/24 06:24:57, maksims wrote:
> > > On 2016/05/23 23:19:03, jww wrote:
> > > > What if secure_cookies < to_protect? Don't you still need to reduce
> > > > cookies_count_possibly_to_be_deleted to by to_protect - secure_cookies?
> > > 
> > > It doesn't matter. |secure_cookies| are subtracted from the total number
of
> > > cookies in order to avoid removing them on the low priority non-secure
> cookies
> > > round. For example, there are 90 low priority cookies: 55 low non-secure
and
> > 35
> > > secure cookies. Line 2063 guarantees we will not go below than the quota,
> > which
> > > is 30 low priority cookies. 
> > > 
> > > cookies_count_possibly_to_be_deleted = 90
> > > check cookies_count_possibly_to_be_deleted <= to_protect or quota
> > > protect_secure_cookies is true,
> > > then secure_cookies = CountProtectedSecureCookiesAtPriority(priority,
> cookies)
> > =
> > > 35.
> > > cookies_count_possibly_to_be_deleted = 90 - 35 = 55 low non-secure
cookies.
> > > 
> > > The "while loop" deletes 55 low non-secure cookies.
> > > Next round: 
> > > cookies_count_possibly_to_be_deleted = 35.
> > > check cookies_count_possibly_to_be_deleted <= to_protect or quota (35 <=
30)
> > > then cookies_count_possibly_to_be_deleted = 35 - 30 = 5 low priority
secure
> > > cookies to be deleted.
> > > 
> > 
> > 
> > What if there are 90 low priority, non-secure cookies and 0 secure cookies?
In
> > that case, in the first round:
> > cookies_count_possibly_to_be_deleted = 90
> > secure_cookies = CountProtectedSecureCookiesAtPriority(...), which is 0
> > cookies_count_possibly_to_be_deleted = 90 - 0
> > 
> > which sets up the round to delete 90 cookies, which is all of them, thus not
> > maintaining the quota. So I believe line 2073 should be:
> > cookies_count_possibly_to_be_deleted -= std::max(secure_cookies, to_protect
-
> > secure_cookies)
> > 
> > Am I still misunderstanding?
> 
> I don't think that's possible, because the total number of cookies for a
certain
> domain
> is much higher. But if that is changed some day, it would cause troubles.
Better
> to
> explicitly check as you proposed.
> Done! 
Well, I was leaving off the number of medium and high priority cookies, but I
was
assuming it's enough to trigger a GC. For example, 90 low priority, non-secure
cookies
and 91 medium priority cookies.
> 
> > 
> >
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> > File net/cookies/cookie_monster.cc (right):
> > 
> >
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> > net/cookies/cookie_monster.cc:324: bool protect_secure_cookies = false) {
> > Please do not use a default argument here; it caused me a bit of confusion
> > below, especially since there are only two call sites, so this doesn't help
> with
> > an exceptional case (just pass in protect_secure_cookies at the two call
sites
> > and it will be much clearer).
> > 
> >
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> > net/cookies/cookie_monster.cc:327: if (cookie->second->Priority() ==
priority
> &&
> > !protect_secure_cookies)
> > Please simplify this conditional. I'm finding the two paths to
cookies_count++
> a
> > bit hard to follow. Maybe something like:
> > 
> > if (cookie->second->Priority() == priority) {
> >   if (!protect_secure_cookies || cookie->second->IsSecure())
> >     cookies_count++;
> > }
> > 
> > Also, as a style note, you should have braces on both branches of the
> > conditional. According to the style guide, braces on one conditional means
> > braces on all branches.

[jww, 2016-06-03 00:11:08]

On 2016/06/03 00:10:34, jww wrote:
> On 2016/05/26 07:53:10, maksims wrote:
> > On 2016/05/26 03:46:39, jww wrote:
> > > This is looking like it's on the right track, but I still have some
> questions.
> > > Thanks for the help on this!
> > > 
> > Thanks!
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> > > File net/cookies/cookie_monster.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> > > net/cookies/cookie_monster.cc:1980: quota = kDomainCookiesQuotaLow;
> > > On 2016/05/24 06:24:57, maksims wrote:
> > > > On 2016/05/23 23:19:03, jww wrote:
> > > > > This is the change that I'm most confused about. Doesn't this mean
that,
> > for
> > > > > example, in the second round where we're looking at low-priority,
secure
> > > > > cookies, the quota gets reset, so we potentially delete an entire new
> > > > > quota-number of cookies? Or am I misunderstanding something?
> > > > > 
> > > > > For example, if I have 40 low-priority, non-secure cookies and 40
> > > > low-priority,
> > > > > secure cookies, and 300 high-priority, secure cookies, wouldn't the
> rounds
> > > go:
> > > > > 1. Delete 10 low-priority, non-secure cookies
> > > > > 2. Delete 20 low-priority, secure cookies
> > > > > 3. Do nothing
> > > > > 4. Do nothing
> > > > > 5. Do nothing
> > > > > 6. Delete 130 high priority, secure cookies (or whatever it is to get
> down
> > > to
> > > > > the final purge goal)
> > > > > 
> > > > > Hmm, looking below, I think I *may* have misunderstood how this works
> > > because
> > > > I
> > > > > *think* what you're going for is that you'll count the secure cookies
as
> > > > > "protected" already in Purge...(). Can you clarify?
> > > > > 
> > > > > At the very least, I think this comment needs updating.
> > > > 
> > > > 
> > > > Not exactly. First, we take a quota at |priority| that we mustn't delete
> (30
> > > for
> > > > low priority cookies, for example).
> > > > Then we count secure and non-secure cookies at |priority|.
> > > > If the total number of cookies at |priority| is lower than the quota, we
> > skip
> > > > the round, which guarantees we will not go below than the quota and
there
> > will
> > > > be no starvation for cookies at the |priority|.
> > > > It is the line 2063, which does the trick:
> > > > if (cookies_count_possibly_to_be_deleted <= to_protect)
> > > > return 0u;
> > > > 
> > > > Then, if we should protect low-priority secure cookies, we count and
> > subtract
> > > > them from the overall number of cookies that cannot be lower than 31
> because
> > > we
> > > > have already checked the number of cookies at line 2063. Afterwards, we
> > delete
> > > > the non-secure cookies till purge goal is hit or number of those cookies
> is
> > 0.
> > > > 
> > > > Next round does the same and deletes low-priority secure cookies if
> removing
> > > > low-priority non-secure cookies has not been enough to hit the quota (30
> for
> > > > low-priority cookies).
> > > > 
> > > > > For example, if I have 40 low-priority, non-secure cookies and 40
> > > > low-priority,
> > > > > secure cookies, and 300 high-priority, secure cookies, wouldn't the
> rounds
> > > go:
> > > > 
> > > > 1. Delete 40 low-priority, non-secure cookies, 0 left
> > > > 2. Delete 10 low-priority, secure cookies, 30 left
> > > > ----> preserve quota of 30 low-priority cookies.
> > > > 3. Do nothing
> > > > 4. Do nothing
> > > > 5. Do nothing
> > > > if total number of cookies shouldn't be less than 180, then
> > > > 6. Delete 150 high priority secure cookies, which preserves 70
> high-priority
> > > > cookies according to a quota of 70 high priority cookies.
> > > > 
> > > > As a result there are 30 low priority secure cookies and 150 high
priority
> > > > secure cookies. 
> > > > 
> > > 
> > > Ok, I've got it. As mentioned before, can you update the comment about the
> > > comment here? I believe it's no longer accurate. At the very least, your
> > > explanation you just gave was very helpful, and it would be useful to have
a
> > > shorter version of it somewhere in the comments :-)
> > > 
> > 
> > Ok! And by the way, it is not possible to have 80 non-sec and sec low
priority
> > cookies
> > and 300 high priority, secure cookies. As soon as the GC notices there are
181
> > cookies,
> > it immediately starts removing the cookies. Thus, it will go like this - 
> > 40 non-secure and 40 secure cookies are uploaded, then when additional 101
> high
> > priority secure 
> > cookies are uploaded, it starts all the 6 rounds -
> > 1. Delete 31 low non-sec -> skip other rounds
> > Then more high priority secure cookies arrived (131 to be exact), we have 9
> > non-sec low, 40 sec low 
> > priority cookies and 132 high priority secure cookies arrive, which results
in
> > 181 all together.
> > GC starts 6 rounds
> > 1. Delete 9 low non-sec
> > 2. Delete 10 low sec
> > 3-5. none
> > 6. Delete 12 high secure cookies
> > And so on. It was not me, It had already worked like this.
> > 
> > check the TestPriorityCookieCase(cm.get(), "100HS 100LN 100MN", 30U, 76U,
70U,
> > 106U, 70U); unittests.
> > I added comments how rounds should go.
> > 
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> > > net/cookies/cookie_monster.cc:2069: // |priority_cookies_might_be_deleted|
> is
> > > higher.
> > > This variable name changed, correct?
> > > 
> > right. Done.
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/60001/net/cookies/cookie_mons...
> > > net/cookies/cookie_monster.cc:2073: cookies_count_possibly_to_be_deleted
-=
> > > secure_cookies;
> > > On 2016/05/24 06:24:57, maksims wrote:
> > > > On 2016/05/23 23:19:03, jww wrote:
> > > > > What if secure_cookies < to_protect? Don't you still need to reduce
> > > > > cookies_count_possibly_to_be_deleted to by to_protect -
secure_cookies?
> > > > 
> > > > It doesn't matter. |secure_cookies| are subtracted from the total number
> of
> > > > cookies in order to avoid removing them on the low priority non-secure
> > cookies
> > > > round. For example, there are 90 low priority cookies: 55 low non-secure
> and
> > > 35
> > > > secure cookies. Line 2063 guarantees we will not go below than the
quota,
> > > which
> > > > is 30 low priority cookies. 
> > > > 
> > > > cookies_count_possibly_to_be_deleted = 90
> > > > check cookies_count_possibly_to_be_deleted <= to_protect or quota
> > > > protect_secure_cookies is true,
> > > > then secure_cookies = CountProtectedSecureCookiesAtPriority(priority,
> > cookies)
> > > =
> > > > 35.
> > > > cookies_count_possibly_to_be_deleted = 90 - 35 = 55 low non-secure
> cookies.
> > > > 
> > > > The "while loop" deletes 55 low non-secure cookies.
> > > > Next round: 
> > > > cookies_count_possibly_to_be_deleted = 35.
> > > > check cookies_count_possibly_to_be_deleted <= to_protect or quota (35 <=
> 30)
> > > > then cookies_count_possibly_to_be_deleted = 35 - 30 = 5 low priority
> secure
> > > > cookies to be deleted.
> > > > 
> > > 
> > > 
> > > What if there are 90 low priority, non-secure cookies and 0 secure
cookies?
> In
> > > that case, in the first round:
> > > cookies_count_possibly_to_be_deleted = 90
> > > secure_cookies = CountProtectedSecureCookiesAtPriority(...), which is 0
> > > cookies_count_possibly_to_be_deleted = 90 - 0
> > > 
> > > which sets up the round to delete 90 cookies, which is all of them, thus
not
> > > maintaining the quota. So I believe line 2073 should be:
> > > cookies_count_possibly_to_be_deleted -= std::max(secure_cookies,
to_protect
> -
> > > secure_cookies)
> > > 
> > > Am I still misunderstanding?
> > 
> > I don't think that's possible, because the total number of cookies for a
> certain
> > domain
> > is much higher. But if that is changed some day, it would cause troubles.
> Better
> > to
> > explicitly check as you proposed.
> > Done! 
> Well, I was leaving off the number of medium and high priority cookies, but I
> was
> assuming it's enough to trigger a GC. For example, 90 low priority, non-secure
> cookies
> and 91 medium priority cookies.
Also, can you make sure we have a unit test for this exact case?
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> > > File net/cookies/cookie_monster.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> > > net/cookies/cookie_monster.cc:324: bool protect_secure_cookies = false) {
> > > Please do not use a default argument here; it caused me a bit of confusion
> > > below, especially since there are only two call sites, so this doesn't
help
> > with
> > > an exceptional case (just pass in protect_secure_cookies at the two call
> sites
> > > and it will be much clearer).
> > > 
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/120001/net/cookies/cookie_mon...
> > > net/cookies/cookie_monster.cc:327: if (cookie->second->Priority() ==
> priority
> > &&
> > > !protect_secure_cookies)
> > > Please simplify this conditional. I'm finding the two paths to
> cookies_count++
> > a
> > > bit hard to follow. Maybe something like:
> > > 
> > > if (cookie->second->Priority() == priority) {
> > >   if (!protect_secure_cookies || cookie->second->IsSecure())
> > >     cookies_count++;
> > > }
> > > 
> > > Also, as a style note, you should have braces on both branches of the
> > > conditional. According to the style guide, braces on one conditional means
> > > braces on all branches.

[jww, 2016-06-03 00:15:47]

Okay, the code is looking pretty good to me, and I've started going over the
test cases, but I want to go over them with a fine tooth comb, so I'll do that
tomorrow. Thanks again for the work on this! In the meantime, it would be great
if you could cleanup the description for this CL a bit. Something more in the
format of:

==========
One line, quick description (< 72 chars)

One brief paragraph explaining what the problem is and what the issue is that
you're aiming to fix is.

One brief paragraph giving a high level overview of what you changed to fix the
problem.
==========

Obviously, you don't really have to stick to that format, but basically I'd like
to see fewer of the test cases in the description ;-)

[maksims (do not use this acc), 2016-06-03 05:06:37]

Description was changed from

==========
According to the section 3 and section 5 in
https://tools.ietf.org/html/draft-west-cookie-priority-00#section-3, I think
current implementation does things wrong. If you check old test case, it turns
out that quota of cookies is not preserved.

For instance, with this example and unittests, there will always be at least 30
low prio cookies, 50 mid and 70 high if there, of course, has been enough of
them before the eviction.

What is more, current implementation and unittests are just adjusted in
accordance to each other.

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). Just changing the
order of cookies, the logic of how it should work changes as well.

Consider the following tests as well -

// Round 1 => 20L; round 2 => 0; round 3 => 11H
    TestPriorityCookieCase(cm.get(), "131HN 50LN", 30U, 0U, 120U, 150U, 0U);

This test does not fail in the old implementation, because high cookies are at
the beginning of the cookies vector (we know that eviction goes from least
accessed). But if we do it like this 

// Round 1 => 20L; round 2 => 0; round 3 => 11L
    TestPriorityCookieCase(cm.get(), "50LN 131HN", 30U, 0U, 120U, 150U, 0U);

The test fails. Because, as I previously said it ruins all the logic behind the
eviction. Thus, many current test cases are just adjusted according to the
implementation. 

In my proposal, 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, if I understand the RFC draft right,
after the eviction algorithm, we should have at least 30 low, 50 mid and 70 high
priority cookies if we had sufficient amount of them in the beginning. You can
see what I mean in the unittests.

PS this is just a quick example and secure cookies are not considered. I just
wanted to demonstrate the logic of the algorithm that complies with the RFC
where each priority cookies quota is preserved.



BUG=609550
==========

to

==========
Making cookies eviction to stick with quota more strictly

According to the section 3 and section 5 in 
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old implementation does things wrong. 
It turns out that quota of specific cookies isn't preserved.

This cl makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority 
cookies if there, of course, has been enough of them before 
the eviction. Please note, secure cookies are more important
than non-secure. 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 run, 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
==========

[maksims (do not use this acc), 2016-06-03 05:06:54]

Description was changed from

==========
Making cookies eviction to stick with quota more strictly

According to the section 3 and section 5 in 
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old implementation does things wrong. 
It turns out that quota of specific cookies isn't preserved.

This cl makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority 
cookies if there, of course, has been enough of them before 
the eviction. Please note, secure cookies are more important
than non-secure. 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 run, 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
==========

to

==========
Making cookies eviction to stick with quota more strictly

According to the section 3 and section 5 in 
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old implementation does things wrong. 
It turns out that quota of specific cookies isn't preserved.

This cl makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority 
cookies if there, of course, has been enough of them before 
the eviction. Please note, secure cookies are more important
than non-secure. 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 run, 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
==========

[maksims (do not use this acc), 2016-06-03 05:10:30]

On 2016/06/03 00:15:47, jww wrote:
> Okay, the code is looking pretty good to me, and I've started going over the
> test cases, but I want to go over them with a fine tooth comb, so I'll do that
> tomorrow. Thanks again for the work on this! In the meantime, it would be
great
> if you could cleanup the description for this CL a bit. Something more in the
> format of:
> 
> ==========
> One line, quick description (< 72 chars)
> 
> One brief paragraph explaining what the problem is and what the issue is that
> you're aiming to fix is.
> 
> One brief paragraph giving a high level overview of what you changed to fix
the
> problem.
> ==========
> 
> Obviously, you don't really have to stick to that format, but basically I'd
like
> to see fewer of the test cases in the description ;-)

Thank you! I cleaned up the description a bit

[jww, 2016-06-03 22:25:33]

Description was changed from

==========
Making cookies eviction to stick with quota more strictly

According to the section 3 and section 5 in 
https://tools.ietf.org/html/draft-west-cookie-priority-
00#section-3, the old implementation does things wrong. 
It turns out that quota of specific cookies isn't preserved.

This cl makes sure there will always be at least 30 low
priority cookies, 50 mid priority and 70 high priority 
cookies if there, of course, has been enough of them before 
the eviction. Please note, secure cookies are more important
than non-secure. 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 run, 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
==========

to

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

[jww, 2016-06-03 22:26:25]

lgtm with one nit in the unit tests about deleting a comment. Thanks again for
sticking with this, and also thanks a lot for updating the comments in the unit
tests to all have rounds. That's super helpful!

https://codereview.chromium.org/1976073002/diff/160001/net/cookies/cookie_mon...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1976073002/diff/160001/net/cookies/cookie_mon...
net/cookies/cookie_monster_unittest.cc:603: //    //
https://tools.ietf.org/html/draft-west-cookie-priority#section-3,
Your bug fixes what this comment is discussing, right? (hoo-ray!) If so, can you
delete it?

[maksims (do not use this acc), 2016-06-04 10:49:56]

On 2016/06/03 22:26:25, jww wrote:
> lgtm with one nit in the unit tests about deleting a comment. Thanks again for
> sticking with this, and also thanks a lot for updating the comments in the
unit
> tests to all have rounds. That's super helpful!
> 
>
https://codereview.chromium.org/1976073002/diff/160001/net/cookies/cookie_mon...
> File net/cookies/cookie_monster_unittest.cc (right):
> 
>
https://codereview.chromium.org/1976073002/diff/160001/net/cookies/cookie_mon...
> net/cookies/cookie_monster_unittest.cc:603: //    //
> https://tools.ietf.org/html/draft-west-cookie-priority#section-3,
> Your bug fixes what this comment is discussing, right? (hoo-ray!) If so, can
you
> delete it?

Thank you! Sure, I will delete.

Matt, can you take a look into code as well?

[maksims (do not use this acc), 2016-06-06 09:14:35]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-06 09:14:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976073002/180001

[commit-bot: I haz the power, 2016-06-06 10:35:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-06 10:35:21]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-06-06 14:48:24]

On 2016/06/04 10:49:56, maksims wrote:
> On 2016/06/03 22:26:25, jww wrote:
> > lgtm with one nit in the unit tests about deleting a comment. Thanks again
for
> > sticking with this, and also thanks a lot for updating the comments in the
> unit
> > tests to all have rounds. That's super helpful!
> > 
> >
>
https://codereview.chromium.org/1976073002/diff/160001/net/cookies/cookie_mon...
> > File net/cookies/cookie_monster_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1976073002/diff/160001/net/cookies/cookie_mon...
> > net/cookies/cookie_monster_unittest.cc:603: //    //
> > https://tools.ietf.org/html/draft-west-cookie-priority#section-3,
> > Your bug fixes what this comment is discussing, right? (hoo-ray!) If so, can
> you
> > delete it?
> 
> Thank you! Sure, I will delete.
> 
> Matt, can you take a look into code as well?

I just don't have the time, unfortunately, so I'll defer to jww on this one.  He
knows this code better, anyways.  Feel free to land when ready.

[maksims (do not use this acc), 2016-06-06 16:43:24]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-06-06 16:43:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org
Link to the patchset: https://codereview.chromium.org/1976073002/#ps180001
(title: "removing deprecated comments")

[commit-bot: I haz the power, 2016-06-06 16:43:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976073002/180001

[maksims (do not use this acc), 2016-06-06 16:44:27]

On 2016/06/06 14:48:24, mmenke wrote:
> On 2016/06/04 10:49:56, maksims wrote:
> > On 2016/06/03 22:26:25, jww wrote:
> > > lgtm with one nit in the unit tests about deleting a comment. Thanks again
> for
> > > sticking with this, and also thanks a lot for updating the comments in the
> > unit
> > > tests to all have rounds. That's super helpful!
> > > 
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/160001/net/cookies/cookie_mon...
> > > File net/cookies/cookie_monster_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1976073002/diff/160001/net/cookies/cookie_mon...
> > > net/cookies/cookie_monster_unittest.cc:603: //    //
> > > https://tools.ietf.org/html/draft-west-cookie-priority#section-3,
> > > Your bug fixes what this comment is discussing, right? (hoo-ray!) If so,
can
> > you
> > > delete it?
> > 
> > Thank you! Sure, I will delete.
> > 
> > Matt, can you take a look into code as well?
> 
> I just don't have the time, unfortunately, so I'll defer to jww on this one. 
He
> knows this code better, anyways.  Feel free to land when ready.

Thanks! I just thoughy you would like to take a look if not that busy as you
mentioned in the beginning of the discussion

[commit-bot: I haz the power, 2016-06-06 16:50:31]

Description was changed from

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

to

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

[commit-bot: I haz the power, 2016-06-06 16:50:32]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2016-06-06 16:53:23]

Description was changed from

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

to

==========
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}
==========

[commit-bot: I haz the power, 2016-06-06 16:53:24]

Patchset 5 (id:??) landed as
https://crrev.com/fe1fea6df040fff38a3a4567e20a3d5847e83170
Cr-Commit-Position: refs/heads/master@{#398049}

[mmenke, 2016-06-15 17:41:05]

This seems to have caused a perf regression on some of the bots.  It's unclear
why.  I'm going to revert, so we can verify that this did cause the issue.  If
not, we can reland.  If it did, needs to be investigated.

[mmenke, 2016-06-15 17:42:04]

A revert of this CL (patchset #5 id:180001) has been created in
https://codereview.chromium.org/2071593002/ by mmenke@chromium.org.

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