Skip to content

Commit 4da5cad

Browse files
committed
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 > [email protected], [email protected] > > Committed: https://crrev.com/162d27135f2ee44ae01341de055d1b827a930767 > Cr-Commit-Position: refs/heads/master@{#376204} [email protected],[email protected] BUG=581588 Review URL: https://codereview.chromium.org/1762693002 Cr-Commit-Position: refs/heads/master@{#379030} (cherry picked from commit 8773435) Review URL: https://codereview.chromium.org/1770183003 . Cr-Commit-Position: refs/branch-heads/2661@{crosswalk-project#122} Cr-Branched-From: ef6f6ae-refs/heads/master@{#378081}
1 parent ced1644 commit 4da5cad

File tree

3 files changed

+157
-419
lines changed

3 files changed

+157
-419
lines changed

net/cookies/cookie_monster.cc

Lines changed: 81 additions & 171 deletions
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,11 @@ const size_t CookieMonster::kDomainPurgeCookies = 30;
111111
const size_t CookieMonster::kMaxCookies = 3300;
112112
const size_t CookieMonster::kPurgeCookies = 300;
113113

114-
const double CookieMonster::kDomainCookiesQuotaLow = 0.2;
115-
const double CookieMonster::kDomainCookiesQuotaMedium = 0.333333333333333333333;
116-
const double CookieMonster::kDomainCookiesQuotaHigh =
117-
1 - kDomainCookiesQuotaLow - kDomainCookiesQuotaMedium;
114+
const size_t CookieMonster::kDomainCookiesQuotaLow = 30;
115+
const size_t CookieMonster::kDomainCookiesQuotaMedium = 50;
116+
const size_t CookieMonster::kDomainCookiesQuotaHigh =
117+
kDomainMaxCookies - kDomainPurgeCookies - kDomainCookiesQuotaLow -
118+
kDomainCookiesQuotaMedium;
118119

119120
const int CookieMonster::kSafeFromGlobalPurgeDays = 30;
120121

@@ -249,6 +250,19 @@ void SplitCookieVectorIntoSecureAndNonSecure(
249250
}
250251
}
251252

253+
// Predicate to support PartitionCookieByPriority().
254+
struct CookiePriorityEqualsTo
255+
: std::unary_function<const CookieMonster::CookieMap::iterator, bool> {
256+
explicit CookiePriorityEqualsTo(CookiePriority priority)
257+
: priority_(priority) {}
258+
259+
bool operator()(const CookieMonster::CookieMap::iterator it) const {
260+
return it->second->Priority() == priority_;
261+
}
262+
263+
const CookiePriority priority_;
264+
};
265+
252266
// For a CookieItVector iterator range [|it_begin|, |it_end|),
253267
// moves all cookies with a given |priority| to the beginning of the list.
254268
// Returns: An iterator in [it_begin, it_end) to the first element with
@@ -257,11 +271,7 @@ CookieMonster::CookieItVector::iterator PartitionCookieByPriority(
257271
CookieMonster::CookieItVector::iterator it_begin,
258272
CookieMonster::CookieItVector::iterator it_end,
259273
CookiePriority priority) {
260-
return std::partition(
261-
it_begin, it_end,
262-
[priority](const CookieMonster::CookieMap::iterator& it) {
263-
return it->second->Priority() == priority;
264-
});
274+
return std::partition(it_begin, it_end, CookiePriorityEqualsTo(priority));
265275
}
266276

267277
bool LowerBoundAccessDateComparator(const CookieMonster::CookieMap::iterator it,
@@ -1897,113 +1907,77 @@ size_t CookieMonster::GarbageCollect(const Time& current,
18971907
if (cookies_.count(key) > kDomainMaxCookies) {
18981908
VLOG(kVlogGarbageCollection) << "GarbageCollect() key: " << key;
18991909

1900-
CookieItVector cookie_its;
1910+
CookieItVector* cookie_its;
19011911

1902-
// First, we purge all expired cookies for this key (regardless of our
1903-
// target number of cookies, as we have no need to keep expired cookies
1904-
// around).
1912+
CookieItVector non_expired_cookie_its;
1913+
cookie_its = &non_expired_cookie_its;
19051914
num_deleted +=
1906-
GarbageCollectExpired(current, cookies_.equal_range(key), &cookie_its);
1907-
1908-
if (cookie_its.size() > kDomainMaxCookies) {
1909-
// Then, if we're over the maximum allowed for this key, we'll remove
1910-
// cookies in the following order:
1911-
//
1912-
// 1. Low-priority non-secure cookies.
1913-
// 2. Medium-priority non-secure cookies.
1914-
// 3. High-priority non-secure cookies.
1915-
// 4. Low-priority secure cookies.
1916-
// 5. Medium-priority secure cookies.
1917-
// 6. High-priority secure cookies.
1918-
//
1919-
// Note that this implies that _all_ non-secure cookies will be removed
1920-
// before _any_ secure cookie is removed, in accordance with
1921-
// https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone
1915+
GarbageCollectExpired(current, cookies_.equal_range(key), cookie_its);
1916+
1917+
CookieItVector secure_cookie_its;
1918+
if (enforce_strict_secure && cookie_its->size() > kDomainMaxCookies) {
1919+
VLOG(kVlogGarbageCollection) << "Garbage collecting non-Secure cookies.";
1920+
num_deleted +=
1921+
GarbageCollectNonSecure(non_expired_cookie_its, &secure_cookie_its);
1922+
cookie_its = &secure_cookie_its;
1923+
}
19221924

1925+
if (cookie_its->size() > kDomainMaxCookies) {
19231926
VLOG(kVlogGarbageCollection) << "Deep Garbage Collect domain.";
19241927
size_t purge_goal =
1925-
cookie_its.size() - (kDomainMaxCookies - kDomainPurgeCookies);
1928+
cookie_its->size() - (kDomainMaxCookies - kDomainPurgeCookies);
19261929
DCHECK(purge_goal > kDomainPurgeCookies);
19271930

1928-
// To execute the above removals, we'll divide |cookies_its| into 6
1929-
// partitions, using 7 boundaries (note that the last boundary is off the
1930-
// end of the list):
1931-
//
1932-
// If both non-secure and secure cookies are present, the list will look
1933-
// like this:
1934-
//
1935-
// LLLLMMMMHHHHHLLLLMMMMHHHH
1936-
// ^ ^ ^ ^ ^ ^ ^
1937-
// 0 1 2 3 4 5 6
1938-
//
1939-
// If only secure cookies are present, the list will look like this:
1940-
//
1941-
// LLLLMMMMHHHHH
1942-
// ^ ^ ^ ^
1943-
// 0 4 5 6
1944-
// 1
1945-
// 2
1946-
// 3
1947-
//
1948-
// If only non-secure cookies are present, the list will look like this:
1949-
//
1950-
// LLLLMMMMHHHHH
1951-
// ^ ^ ^ ^
1952-
// 0 1 2 3
1953-
// 4
1954-
// 5
1955-
// 6
1956-
CookieItVector::iterator it_bdd[7];
1957-
it_bdd[0] = cookie_its.begin();
1958-
it_bdd[1] = it_bdd[0];
1959-
it_bdd[2] = it_bdd[0];
1960-
it_bdd[3] = cookie_its.end();
1961-
it_bdd[4] = it_bdd[3];
1962-
it_bdd[5] = it_bdd[3];
1963-
it_bdd[6] = it_bdd[3];
1964-
1965-
size_t num_nonsecure = 0;
1966-
1967-
// Move all non-secure cookies to the front of the list, and set boundary
1968-
// #3 to the first secure cookie (or off the end of the list, in the case
1969-
// where no secure cookies are present).
1970-
it_bdd[3] = std::partition(it_bdd[0], it_bdd[6],
1971-
[](const CookieMap::iterator& it) {
1972-
return !it->second->IsSecure();
1973-
});
1974-
1975-
// If we have non-secure cookies, partition them into priorities:
1976-
if (it_bdd[3] > it_bdd[0]) {
1977-
it_bdd[1] = PartitionCookieByPriority(it_bdd[0], it_bdd[3],
1978-
COOKIE_PRIORITY_LOW);
1979-
it_bdd[2] = PartitionCookieByPriority(it_bdd[1], it_bdd[3],
1980-
COOKIE_PRIORITY_MEDIUM);
1981-
num_nonsecure = it_bdd[3] - it_bdd[0];
1982-
}
1983-
1984-
// Likewise, if we have secure cookies, partition them into priorities:
1985-
if (it_bdd[3] < it_bdd[6]) {
1986-
it_bdd[4] = PartitionCookieByPriority(it_bdd[3], it_bdd[6],
1987-
COOKIE_PRIORITY_LOW);
1988-
it_bdd[5] = PartitionCookieByPriority(it_bdd[4], it_bdd[6],
1989-
COOKIE_PRIORITY_MEDIUM);
1990-
}
1991-
1992-
// Start with the non-secure cookies.
1993-
if (purge_goal >= num_nonsecure) {
1994-
// If we need to purge more cookies than we have non-secure, remove
1995-
// them all, update |purge_goal| then purge the new |purge_goal| from
1996-
// the secure cookies.
1931+
// Boundary iterators into |cookie_its| for different priorities.
1932+
CookieItVector::iterator it_bdd[4];
1933+
// Intialize |it_bdd| while sorting |cookie_its| by priorities.
1934+
// Schematic: [MLLHMHHLMM] => [LLL|MMMM|HHH], with 4 boundaries.
1935+
it_bdd[0] = cookie_its->begin();
1936+
it_bdd[3] = cookie_its->end();
1937+
it_bdd[1] =
1938+
PartitionCookieByPriority(it_bdd[0], it_bdd[3], COOKIE_PRIORITY_LOW);
1939+
it_bdd[2] = PartitionCookieByPriority(it_bdd[1], it_bdd[3],
1940+
COOKIE_PRIORITY_MEDIUM);
1941+
size_t quota[3] = {kDomainCookiesQuotaLow,
1942+
kDomainCookiesQuotaMedium,
1943+
kDomainCookiesQuotaHigh};
1944+
1945+
// Purge domain cookies in 3 rounds.
1946+
// Round 1: consider low-priority cookies only: evict least-recently
1947+
// accessed, while protecting quota[0] of these from deletion.
1948+
// Round 2: consider {low, medium}-priority cookies, evict least-recently
1949+
// accessed, while protecting quota[0] + quota[1].
1950+
// Round 3: consider all cookies, evict least-recently accessed.
1951+
size_t accumulated_quota = 0;
1952+
CookieItVector::iterator it_purge_begin = it_bdd[0];
1953+
for (int i = 0; i < 3 && purge_goal > 0; ++i) {
1954+
accumulated_quota += quota[i];
1955+
1956+
size_t num_considered = it_bdd[i + 1] - it_purge_begin;
1957+
if (num_considered <= accumulated_quota)
1958+
continue;
1959+
1960+
// Number of cookies that will be purged in this round.
1961+
size_t round_goal =
1962+
std::min(purge_goal, num_considered - accumulated_quota);
1963+
purge_goal -= round_goal;
1964+
1965+
SortLeastRecentlyAccessed(it_purge_begin, it_bdd[i + 1], round_goal);
1966+
// Cookies accessed on or after |safe_date| would have been safe from
1967+
// global purge, and we want to keep track of this.
1968+
CookieItVector::iterator it_purge_end = it_purge_begin + round_goal;
1969+
CookieItVector::iterator it_purge_middle =
1970+
LowerBoundAccessDate(it_purge_begin, it_purge_end, safe_date);
1971+
// Delete cookies accessed before |safe_date|.
19971972
num_deleted += GarbageCollectDeleteRange(
1998-
current, DELETE_COOKIE_NON_SECURE, it_bdd[0], it_bdd[3]);
1999-
num_deleted += GarbageCollectNumFromRangeWithQuota(
2000-
current, safe_date, purge_goal - num_deleted, it_bdd, GC_SECURE);
2001-
} else {
2002-
num_deleted += GarbageCollectNumFromRangeWithQuota(
2003-
current, safe_date, purge_goal, it_bdd, GC_NONSECURE);
1973+
current, DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE, it_purge_begin,
1974+
it_purge_middle);
1975+
// Delete cookies accessed on or after |safe_date|.
1976+
num_deleted += GarbageCollectDeleteRange(
1977+
current, DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE, it_purge_middle,
1978+
it_purge_end);
1979+
it_purge_begin = it_purge_end;
20041980
}
2005-
purge_goal -= num_deleted;
2006-
20071981
DCHECK_EQ(0U, purge_goal);
20081982
}
20091983
}
@@ -2051,70 +2025,6 @@ size_t CookieMonster::GarbageCollect(const Time& current,
20512025
return num_deleted;
20522026
}
20532027

2054-
size_t CookieMonster::GarbageCollectNumFromRangeWithQuota(
2055-
const Time& current,
2056-
const Time& safe_date,
2057-
size_t purge_goal,
2058-
CookieItVector::iterator* it_bdd,
2059-
GCType type) {
2060-
size_t num_deleted = 0;
2061-
size_t begin_partition = type == GC_SECURE ? 3 : 0;
2062-
size_t num_cookies = it_bdd[begin_partition + 3] - it_bdd[begin_partition];
2063-
size_t num_to_keep = num_cookies - purge_goal;
2064-
size_t quota[3] = {std::floor(num_to_keep * kDomainCookiesQuotaLow),
2065-
std::floor(num_to_keep * kDomainCookiesQuotaMedium),
2066-
std::floor(num_to_keep * kDomainCookiesQuotaHigh)};
2067-
2068-
// The quota calculation will be up to 3 fewer than the number of
2069-
// cookies we can keep. Bump up the numbers to get the right answer:
2070-
if (quota[0] + quota[1] + quota[2] < num_to_keep)
2071-
quota[2] += 1;
2072-
if (quota[0] + quota[1] + quota[2] < num_to_keep)
2073-
quota[1] += 1;
2074-
if (quota[0] + quota[1] + quota[2] < num_to_keep)
2075-
quota[0] += 1;
2076-
DCHECK_EQ(num_to_keep, quota[0] + quota[1] + quota[2]);
2077-
2078-
// Purge domain cookies in 3 rounds.
2079-
// Round 1: consider low-priority cookies only: evict least-recently
2080-
// accessed, while protecting quota[0] of these from deletion.
2081-
// Round 2: consider {low, medium}-priority cookies, evict least-recently
2082-
// accessed, while protecting quota[0] + quota[1].
2083-
// Round 3: consider all cookies, evict least-recently accessed.
2084-
size_t accumulated_quota = 0;
2085-
CookieItVector::iterator it_purge_begin = it_bdd[begin_partition];
2086-
for (size_t i = 0; i < arraysize(quota) && purge_goal > 0; ++i) {
2087-
accumulated_quota += quota[i];
2088-
2089-
size_t num_considered = it_bdd[i + begin_partition + 1] - it_purge_begin;
2090-
if (num_considered <= accumulated_quota)
2091-
continue;
2092-
2093-
// Number of cookies that will be purged in this round.
2094-
size_t round_goal =
2095-
std::min(purge_goal, num_considered - accumulated_quota);
2096-
purge_goal -= round_goal;
2097-
2098-
SortLeastRecentlyAccessed(it_purge_begin, it_bdd[i + begin_partition + 1],
2099-
round_goal);
2100-
// Cookies accessed on or after |safe_date| would have been safe from
2101-
// global purge, and we want to keep track of this.
2102-
CookieItVector::iterator it_purge_end = it_purge_begin + round_goal;
2103-
CookieItVector::iterator it_purge_middle =
2104-
LowerBoundAccessDate(it_purge_begin, it_purge_end, safe_date);
2105-
// Delete cookies accessed before |safe_date|.
2106-
num_deleted += GarbageCollectDeleteRange(
2107-
current, DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE, it_purge_begin,
2108-
it_purge_middle);
2109-
// Delete cookies accessed on or after |safe_date|.
2110-
num_deleted += GarbageCollectDeleteRange(
2111-
current, DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE, it_purge_middle,
2112-
it_purge_end);
2113-
it_purge_begin = it_purge_end;
2114-
}
2115-
return num_deleted;
2116-
}
2117-
21182028
size_t CookieMonster::GarbageCollectExpired(const Time& current,
21192029
const CookieMapItPair& itpair,
21202030
CookieItVector* cookie_its) {

net/cookies/cookie_monster.h

Lines changed: 3 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -129,11 +129,9 @@ class NET_EXPORT CookieMonster : public CookieStore {
129129
static const size_t kPurgeCookies;
130130

131131
// Quota for cookies with {low, medium, high} priorities within a domain.
132-
// The quota is specified as a percentage of the total number of cookies we
133-
// intend to retain for a domain.
134-
static const double kDomainCookiesQuotaLow;
135-
static const double kDomainCookiesQuotaMedium;
136-
static const double kDomainCookiesQuotaHigh;
132+
static const size_t kDomainCookiesQuotaLow;
133+
static const size_t kDomainCookiesQuotaMedium;
134+
static const size_t kDomainCookiesQuotaHigh;
137135

138136
// The store passed in should not have had Init() called on it yet. This
139137
// class will take care of initializing it. The backing store is NOT owned by
@@ -368,8 +366,6 @@ class NET_EXPORT CookieMonster : public CookieStore {
368366
COOKIE_DELETE_EQUIVALENT_LAST_ENTRY
369367
};
370368

371-
enum GCType { GC_NONSECURE, GC_SECURE };
372-
373369
// The strategy for fetching cookies. Controlled by Finch experiment.
374370
enum FetchStrategy {
375371
// Fetches all cookies only when they're needed.
@@ -584,27 +580,6 @@ class NET_EXPORT CookieMonster : public CookieStore {
584580
size_t purge_goal,
585581
CookieItVector cookie_its);
586582

587-
// Helper for GarbageCollect(). Deletes |purge_goal| cookies of type |type|
588-
// from the ranges specified in |it_bdd|. Returns the number of cookies
589-
// deleted.
590-
//
591-
// |it_bdd| is a bit of a complicated beast: it must be a 7-element array
592-
// of 'CookieItVector::Iterator' objects that demarcate the boundaries of
593-
// a list of cookies sorted first by secure/non-secure, and then by priority,
594-
// low to high. That is, it ought to look something like:
595-
//
596-
// LLLLMMMMHHHHHLLLLMMMMHHHH
597-
// ^ ^ ^ ^ ^ ^ ^
598-
// 0 1 2 3 4 5 6
599-
//
600-
// TODO(mkwst): This is super-complicated. We should determine whether we
601-
// can simplify our implementation of "priority".
602-
size_t GarbageCollectNumFromRangeWithQuota(const base::Time& current,
603-
const base::Time& safe_date,
604-
size_t purge_goal,
605-
CookieItVector::iterator* it_bdd,
606-
GCType type);
607-
608583
// Find the key (for lookup in cookies_) based on the given domain.
609584
// See comment on keys before the CookieMap typedef.
610585
std::string GetKey(const std::string& domain) const;

0 commit comments

Comments
 (0)