Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1705753002:
Evict non-secure cookies less agressively. (Closed)
DescriptionEvict non-secure cookies less agressively.
The current implementation of strict-secure cookies wipes all non-secure
cookies when an origin exceeds ~180. This is a bit agressive, and has
real impact on users.
This patch softens that to remove only the number of non-secure cookies
necessary to get under the ~150 cap. If a user has 150 secure cookies,
we'll still wipe all non-secure cookies, but that seems less likely to
impact users than the current behavior.
The patch is a bit more complicated than I expected due to interactions
with the Chrome-only "priority" feature we shipped back in 2013. In
short, we execute priority-driven removal of non-secure cookies first,
and only touch secure cookies if necessary.
BUG=581588
R=mmenke@chromium.org, jww@chromium.org
Committed: https://crrev.com/162d27135f2ee44ae01341de055d1b827a930767
Cr-Commit-Position: refs/heads/master@{#376204}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Test. #Patch Set 3 : Feedback. #Patch Set 4 : Compiling is fun. #
Messages
Total messages: 26 (10 generated)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Hi jww@ and mmenke@! Would you mind taking a look at this patch? -mike
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705753002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705753002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:2014: COOKIE_PRIORITY_MEDIUM); This seems so much more complicated than necessary. Why can't we just sort them in the order we want to delete them in, once (First by secure, then by priority), and then just delete the first how many ever cookies we want to purge? Is the issue safe_date?
On 2016/02/17 at 18:41:31, mmenke wrote: > https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.cc > File net/cookies/cookie_monster.cc (right): > > https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.... > net/cookies/cookie_monster.cc:2014: COOKIE_PRIORITY_MEDIUM); > This seems so much more complicated than necessary. > > Why can't we just sort them in the order we want to delete them in, once (First by secure, then by priority), and then just delete the first how many ever cookies we want to purge? Is the issue safe_date? Oh I so wholeheartedly agree. This was a mess to write. The issue is the somewhat complicated existing behavior of the priority system. Basically, we don't want to remove all LOW priority, and then all MEDIUM priority, and then all HIGH priority, because doing so could starve a service that just uses LOW priority cookies; the current implementation is a balance between respecting the cookie's declared priority, and culling based on last access date. That's why the existing code has this three-pass `for` loop and quota system. The current code's convolutions are an attempt to maintain exactly that existing behavior, while adding the new constraint that ensures that non-secure cookies are removed before secure cookies. That said: I don't know whether we _need_ to maintain the existing behavior. I'm a bit worried about Google's usage internally, on the one hand, but on the other, this is proprietary Chrome behavior that no other browser implements. I think there's some room for fiddling around with things, but I'd like to do that deliberately, and in concert with the internal Google folks who might be affected.
Yikes, this is a beast of a change for a fix. Sorry for having to clean up my mess, Mike. lgtm, with a few nits and some test suggestions. https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1954: // https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone-05 Should this link really have the '-05' at the end? https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:2121: for (int i = 0; i < 3 && purge_goal > 0; ++i) { 'i < 3' -> 'i < ARRAY_SIZE(quota)' in case the length changes in the future (doubtful as that might be). https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.h File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.h:132: static const float kDomainCookiesQuotaLow; I think an explanation of this being a float is needed in the comment, esp. because it's called a "quota" which I would normally expect to be an integer limit. In practice, I think this is more of a "proportional limit" or something, but as long as the comment explains it, we're probably good. https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster_... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/1705753002/diff/1/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:646: // Each test case adds 180 secure cookies, and some non-secure cookie. The Maybe add a few sanity check cases with > 180 cookies? Also, is it worth having some mashed up test cases with the eviction test cases below in EvictSecureCookies, to make sure that priorities are playing nice with some of the other subtle edge cases, such as weird ordering of secure cookies and non-secure cookies?
Feel free to land with just jww's signoff - I want to learn this code, but I'm a bit behind at the moment, so won't get to it in a timely manner.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705753002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705753002/20001
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705753002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705753002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705753002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705753002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mkwst@chromium.org
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/1705753002/#ps60001 (title: "Compiling is fun.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705753002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705753002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Evict non-secure cookies less agressively. The current implementation of strict-secure cookies wipes all non-secure cookies when an origin exceeds ~180. This is a bit agressive, and has real impact on users. This patch softens that to remove only the number of non-secure cookies necessary to get under the ~150 cap. If a user has 150 secure cookies, we'll still wipe all non-secure cookies, but that seems less likely to impact users than the current behavior. The patch is a bit more complicated than I expected due to interactions with the Chrome-only "priority" feature we shipped back in 2013. In short, we execute priority-driven removal of non-secure cookies first, and only touch secure cookies if necessary. BUG=581588 R=mmenke@chromium.org, jww@chromium.org ========== to ========== Evict non-secure cookies less agressively. The current implementation of strict-secure cookies wipes all non-secure cookies when an origin exceeds ~180. This is a bit agressive, and has real impact on users. This patch softens that to remove only the number of non-secure cookies necessary to get under the ~150 cap. If a user has 150 secure cookies, we'll still wipe all non-secure cookies, but that seems less likely to impact users than the current behavior. The patch is a bit more complicated than I expected due to interactions with the Chrome-only "priority" feature we shipped back in 2013. In short, we execute priority-driven removal of non-secure cookies first, and only touch secure cookies if necessary. BUG=581588 R=mmenke@chromium.org, jww@chromium.org Committed: https://crrev.com/162d27135f2ee44ae01341de055d1b827a930767 Cr-Commit-Position: refs/heads/master@{#376204} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/162d27135f2ee44ae01341de055d1b827a930767 Cr-Commit-Position: refs/heads/master@{#376204}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1762693002/ by mkwst@chromium.org. The reason for reverting is: Let's see if the automagical reversion thingie will work on a ~2 week old patch. ``` I updated the eviction algorithm with our initial pass at a merger between leave-secure-cookies-along and cookie-priorities, and neglected to ensure that that new algorithm was safely locked up behind the "strict secure cookies" experiment. That patch is in M50. I think the right thing to do is to revert https://codereview.chromium.org/1705753002 on the M50 branch so that we retain the status quo behavior for beta. That means that we're going to have to wait a bit longer to start ratcheting down on cookies, which is unfortunate, but the safe choice this late in the game. ``` BUG=591720. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
