|
|
Created:
4 years, 7 months ago by maksims (do not use this acc) Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaking 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}
Patch Set 1 #Patch Set 2 : strict cookie #
Total comments: 13
Patch Set 3 : rebasing + comments #
Total comments: 4
Patch Set 4 : comments #
Total comments: 1
Patch Set 5 : removing deprecated comments #Messages
Total messages: 56 (23 generated)
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 ==========
maksim.sisov@intel.com changed reviewers: + jww@chromium.org, mmenke@chromium.org
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 ==========
please take a look at this example
maksim.sisov@intel.com changed reviewers: + mkwst@chromium.org
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 ==========
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.
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.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
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.
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
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)
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.
maksim.sisov@intel.com changed reviewers: - mmenke@chromium.org
gentle reminder
maksim.sisov@intel.com changed reviewers: + mmenke@chromium.org
Patchset #3 (id:80001) has been deleted
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?
Patchset #3 (id:100001) has been deleted
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.
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.
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.
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.
Patchset #4 (id:140001) has been deleted
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.
gentle reminder
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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.
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.
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 ;-)
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 ==========
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 ==========
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
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 ==========
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?
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?
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976073002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
The CQ bit was checked by maksim.sisov@intel.com
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976073002/180001
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
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fe1fea6df040fff38a3a4567e20a3d5847e83170 Cr-Commit-Position: refs/heads/master@{#398049}
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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. |