Cookie eviction policy changes are not correctly hidden behind the experimental flag. |
|||||||
Issue descriptionI 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. Friendly release managers, how do you feel about letting me revert https://crrev.com/162d27135f2ee44ae01341de055d1b827a930767 on the 2661 branch?
,
Mar 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8773435ab4441ce8f78611b694e92659e9b50a63 commit 8773435ab4441ce8f78611b694e92659e9b50a63 Author: mkwst <mkwst@chromium.org> Date: Thu Mar 03 17:36:23 2016 Revert of Evict non-secure cookies less agressively. (patchset #4 id:60001 of https://codereview.chromium.org/1705753002/ ) Reason for revert: 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 Original issue's description: > 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} TBR=jww@chromium.org,mmenke@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=581588 Review URL: https://codereview.chromium.org/1762693002 Cr-Commit-Position: refs/heads/master@{#379030} [modify] https://crrev.com/8773435ab4441ce8f78611b694e92659e9b50a63/net/cookies/cookie_monster.cc [modify] https://crrev.com/8773435ab4441ce8f78611b694e92659e9b50a63/net/cookies/cookie_monster.h [modify] https://crrev.com/8773435ab4441ce8f78611b694e92659e9b50a63/net/cookies/cookie_monster_unittest.cc
,
Mar 4 2016
Your change meets the bar and is auto-approved for M50 (branch: 2661)
,
Mar 4 2016
Pls add impacted OS. Thanks.
,
Mar 8 2016
,
Mar 8 2016
Please merge your change to M50 branch 2661 ASAP if you think it is a safe merge as we're very close to M50 Beta candidate cut. Thank you.
,
Mar 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4da5cad6894efb0efb77e91236d1aa6a823fbc86 commit 4da5cad6894efb0efb77e91236d1aa6a823fbc86 Author: Mike West <mkwst@google.com> Date: Tue Mar 08 09:43:47 2016 Revert of Evict non-secure cookies less agressively. (patchset #4 id:60001 of https://codereview.chromium.org/1705753002/ ) Reason for revert: 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 Original issue's description: > 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} TBR=jww@chromium.org,mmenke@chromium.org BUG=581588 Review URL: https://codereview.chromium.org/1762693002 Cr-Commit-Position: refs/heads/master@{#379030} (cherry picked from commit 8773435ab4441ce8f78611b694e92659e9b50a63) Review URL: https://codereview.chromium.org/1770183003 . Cr-Commit-Position: refs/branch-heads/2661@{#122} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} [modify] https://crrev.com/4da5cad6894efb0efb77e91236d1aa6a823fbc86/net/cookies/cookie_monster.cc [modify] https://crrev.com/4da5cad6894efb0efb77e91236d1aa6a823fbc86/net/cookies/cookie_monster.h [modify] https://crrev.com/4da5cad6894efb0efb77e91236d1aa6a823fbc86/net/cookies/cookie_monster_unittest.cc
,
Mar 14 2016
mkwst@ : Is there any steps to get verified by Test Eng team, if not could you please update the stats of the issue accordingly.
,
Mar 14 2016
No. This just reverts the behavior on ToT and M50 to the behavior we already had in M1-M49. :) |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by bugdroid1@chromium.org
, Mar 3 2016