Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Adds non-secure cookie eviction to strict secure cookies experiment.
Browse files Browse the repository at this point in the history 
Per the draft modification to the Cookie RFC
(https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone),
this adds support for the eviction of non-secure cookies immediately
after eviction of expired cookies and before any other evictions.

This is a follow up to https://codereview.chromium.org/1420333002/.

BUG=546820
[email protected]

Review URL: https://codereview.chromium.org/1470653002

Cr-Commit-Position: refs/heads/master@{#361706}
  • Loading branch information
@joelweinberger
joelweinberger authored and Commit bot committed Nov 25, 2015
1 parent 3eefbb9 commit 82d99c1
Show file tree
Hide file tree
Showing 6 changed files with 378 additions and 63 deletions.
144 changes: 114 additions & 30 deletions net/cookies/cookie_monster.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,23 @@ void SortLeastRecentlyAccessed(CookieMonster::CookieItVector::iterator it_begin,
std::partial_sort(it_begin, it_begin + num_sort + 1, it_end, LRACookieSorter);
}

// Given a single cookie vector |cookie_its|, pushs all of the secure cookies in
// |cookie_its| into |secure_cookie_its| and all of the non-secure cookies into
// |non_secure_cookie_its|. Both |secure_cookie_its| and |non_secure_cookie_its|
// must be non-NULL.
void SplitCookieVectorIntoSecureAndNonSecure(
const CookieMonster::CookieItVector& cookie_its,
CookieMonster::CookieItVector* secure_cookie_its,
CookieMonster::CookieItVector* non_secure_cookie_its) {
DCHECK(secure_cookie_its && non_secure_cookie_its);
for (const auto& curit : cookie_its) {
if (curit->second->IsSecure())
secure_cookie_its->push_back(curit);
else
non_secure_cookie_its->push_back(curit);
}
}

// Predicate to support PartitionCookieByPriority().
struct CookiePriorityEqualsTo
: std::unary_function<const CookieMonster::CookieMap::iterator, bool> {
Expand Down Expand Up @@ -306,6 +323,8 @@ ChangeCausePair ChangeCauseMapping[] = {
{CookieMonsterDelegate::CHANGE_COOKIE_EXPIRED_OVERWRITE, true},
// DELETE_COOKIE_CONTROL_CHAR
{CookieMonsterDelegate::CHANGE_COOKIE_EVICTED, true},
// DELETE_COOKIE_NON_SECURE
{CookieMonsterDelegate::CHANGE_COOKIE_EVICTED, true},
// DELETE_COOKIE_LAST_ENTRY
{CookieMonsterDelegate::CHANGE_COOKIE_EXPLICIT, false}};

Expand Down Expand Up @@ -1956,7 +1975,7 @@ bool CookieMonster::SetCanonicalCookie(scoped_ptr<CanonicalCookie>* cc,
// make sure that we garbage collect... We can also make the assumption that
// if a cookie was set, in the common case it will be used soon after,
// and we will purge the expired cookies in GetCookies().
GarbageCollect(creation_time, key);
GarbageCollect(creation_time, key, options.enforce_strict_secure());

return true;
}
Expand Down Expand Up @@ -2028,34 +2047,69 @@ void CookieMonster::InternalDeleteCookie(CookieMap::iterator it,
delete cc;
}

size_t CookieMonster::GarbageCollectLeastRecentlyAccessed(
const base::Time& current,
const base::Time& safe_date,
size_t purge_goal,
CookieItVector cookie_its) {
// Sorts up to *and including* |cookie_its[purge_goal]|, so
// |earliest_access_time| will be properly assigned even if
// |global_purge_it| == |cookie_its.begin() + purge_goal|.
SortLeastRecentlyAccessed(cookie_its.begin(), cookie_its.end(), purge_goal);
// Find boundary to cookies older than safe_date.
CookieItVector::iterator global_purge_it = LowerBoundAccessDate(
cookie_its.begin(), cookie_its.begin() + purge_goal, safe_date);
// Only delete the old cookies, and if strict secure is enabled, delete
// non-secure ones first.
size_t num_deleted =
GarbageCollectDeleteRange(current, DELETE_COOKIE_EVICTED_GLOBAL,
cookie_its.begin(), global_purge_it);
// Set access day to the oldest cookie that wasn't deleted.
earliest_access_time_ = (*global_purge_it)->second->LastAccessDate();
return num_deleted;
}

// Domain expiry behavior is unchanged by key/expiry scheme (the
// meaning of the key is different, but that's not visible to this routine).
int CookieMonster::GarbageCollect(const Time& current, const std::string& key) {
size_t CookieMonster::GarbageCollect(const Time& current,
const std::string& key,
bool enforce_strict_secure) {
lock_.AssertAcquired();

int num_deleted = 0;
size_t num_deleted = 0;
Time safe_date(Time::Now() - TimeDelta::FromDays(kSafeFromGlobalPurgeDays));

// Collect garbage for this key, minding cookie priorities.
if (cookies_.count(key) > kDomainMaxCookies) {
VLOG(kVlogGarbageCollection) << "GarbageCollect() key: " << key;

CookieItVector cookie_its;
CookieItVector* cookie_its;

CookieItVector non_expired_cookie_its;
cookie_its = &non_expired_cookie_its;
num_deleted +=
GarbageCollectExpired(current, cookies_.equal_range(key), &cookie_its);
GarbageCollectExpired(current, cookies_.equal_range(key), cookie_its);

if (cookie_its.size() > kDomainMaxCookies) {
CookieItVector secure_cookie_its;
if (enforce_strict_secure && cookie_its->size() > kDomainMaxCookies) {
VLOG(kVlogGarbageCollection) << "Garbage collecting non-Secure cookies.";
num_deleted +=
GarbageCollectNonSecure(non_expired_cookie_its, &secure_cookie_its);
cookie_its = &secure_cookie_its;
}

if (cookie_its->size() > kDomainMaxCookies) {
VLOG(kVlogGarbageCollection) << "Deep Garbage Collect domain.";
size_t purge_goal =
cookie_its.size() - (kDomainMaxCookies - kDomainPurgeCookies);
cookie_its->size() - (kDomainMaxCookies - kDomainPurgeCookies);
DCHECK(purge_goal > kDomainPurgeCookies);

// Boundary iterators into |cookie_its| for different priorities.
CookieItVector::iterator it_bdd[4];
// Intialize |it_bdd| while sorting |cookie_its| by priorities.
// Schematic: [MLLHMHHLMM] => [LLL|MMMM|HHH], with 4 boundaries.
it_bdd[0] = cookie_its.begin();
it_bdd[3] = cookie_its.end();
it_bdd[0] = cookie_its->begin();
it_bdd[3] = cookie_its->end();
it_bdd[1] =
PartitionCookieByPriority(it_bdd[0], it_bdd[3], COOKIE_PRIORITY_LOW);
it_bdd[2] = PartitionCookieByPriority(it_bdd[1], it_bdd[3],
Expand Down Expand Up @@ -2109,36 +2163,47 @@ int CookieMonster::GarbageCollect(const Time& current, const std::string& key) {
if (cookies_.size() > kMaxCookies && earliest_access_time_ < safe_date) {
VLOG(kVlogGarbageCollection) << "GarbageCollect() everything";
CookieItVector cookie_its;

num_deleted += GarbageCollectExpired(
current, CookieMapItPair(cookies_.begin(), cookies_.end()),
&cookie_its);

if (cookie_its.size() > kMaxCookies) {
VLOG(kVlogGarbageCollection) << "Deep Garbage Collect everything.";
size_t purge_goal = cookie_its.size() - (kMaxCookies - kPurgeCookies);
DCHECK(purge_goal > kPurgeCookies);
// Sorts up to *and including* |cookie_its[purge_goal]|, so
// |earliest_access_time| will be properly assigned even if
// |global_purge_it| == |cookie_its.begin() + purge_goal|.
SortLeastRecentlyAccessed(cookie_its.begin(), cookie_its.end(),
purge_goal);
// Find boundary to cookies older than safe_date.
CookieItVector::iterator global_purge_it = LowerBoundAccessDate(
cookie_its.begin(), cookie_its.begin() + purge_goal, safe_date);
// Only delete the old cookies.
num_deleted +=
GarbageCollectDeleteRange(current, DELETE_COOKIE_EVICTED_GLOBAL,
cookie_its.begin(), global_purge_it);
// Set access day to the oldest cookie that wasn't deleted.
earliest_access_time_ = (*global_purge_it)->second->LastAccessDate();

if (enforce_strict_secure) {
CookieItVector secure_cookie_its;
CookieItVector non_secure_cookie_its;
SplitCookieVectorIntoSecureAndNonSecure(cookie_its, &secure_cookie_its,
&non_secure_cookie_its);
size_t non_secure_purge_goal =
std::min<size_t>(purge_goal, non_secure_cookie_its.size() - 1);

size_t just_deleted = GarbageCollectLeastRecentlyAccessed(
current, safe_date, non_secure_purge_goal, non_secure_cookie_its);
num_deleted += just_deleted;

if (just_deleted < purge_goal) {
size_t secure_purge_goal = std::min<size_t>(
purge_goal - just_deleted, secure_cookie_its.size() - 1);
num_deleted += GarbageCollectLeastRecentlyAccessed(
current, safe_date, secure_purge_goal, secure_cookie_its);
}
} else {
num_deleted += GarbageCollectLeastRecentlyAccessed(
current, safe_date, purge_goal, cookie_its);
}
}
}

return num_deleted;
}

int CookieMonster::GarbageCollectExpired(const Time& current,
const CookieMapItPair& itpair,
CookieItVector* cookie_its) {
size_t CookieMonster::GarbageCollectExpired(const Time& current,
const CookieMapItPair& itpair,
CookieItVector* cookie_its) {
if (keep_expired_cookies_)
return 0;

Expand All @@ -2160,10 +2225,29 @@ int CookieMonster::GarbageCollectExpired(const Time& current,
return num_deleted;
}

int CookieMonster::GarbageCollectDeleteRange(const Time& current,
DeletionCause cause,
CookieItVector::iterator it_begin,
CookieItVector::iterator it_end) {
size_t CookieMonster::GarbageCollectNonSecure(
const CookieItVector& valid_cookies,
CookieItVector* cookie_its) {
lock_.AssertAcquired();

size_t num_deleted = 0;
for (const auto& curr_cookie_it : valid_cookies) {
if (!curr_cookie_it->second->IsSecure()) {
InternalDeleteCookie(curr_cookie_it, true, DELETE_COOKIE_NON_SECURE);
++num_deleted;
} else if (cookie_its) {
cookie_its->push_back(curr_cookie_it);
}
}

return num_deleted;
}

size_t CookieMonster::GarbageCollectDeleteRange(
const Time& current,
DeletionCause cause,
CookieItVector::iterator it_begin,
CookieItVector::iterator it_end) {
for (CookieItVector::iterator it = it_begin; it != it_end; it++) {
histogram_evicted_last_access_minutes_->Add(
(current - (*it)->second->LastAccessDate()).InMinutes());
Expand Down
48 changes: 37 additions & 11 deletions net/cookies/cookie_monster.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,10 @@ class NET_EXPORT CookieMonster : public CookieStore {
// For CookieSource histogram enum.
FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, CookieSourceHistogram);

// For kSafeFromGlobalPurgeDays in CookieStore
FRIEND_TEST_ALL_PREFIXES(CookieMonsterSecureCookiesRequireSecureSchemeTest,
EvictSecureCookies);

// Internal reasons for deletion, used to populate informative histograms
// and to provide a public cause for onCookieChange notifications.
//
Expand Down Expand Up @@ -400,6 +404,10 @@ class NET_EXPORT CookieMonster : public CookieStore {
// cookies as we load them. See http://crbug.com/238041.
DELETE_COOKIE_CONTROL_CHAR,

// When strict secure cookies is enabled, non-secure cookies are evicted
// right after expired cookies.
DELETE_COOKIE_NON_SECURE,

DELETE_COOKIE_LAST_ENTRY
};

Expand Down Expand Up @@ -617,23 +625,41 @@ class NET_EXPORT CookieMonster : public CookieStore {
// constants for details.
//
// Returns the number of cookies deleted (useful for debugging).
int GarbageCollect(const base::Time& current, const std::string& key);
size_t GarbageCollect(const base::Time& current,
const std::string& key,
bool enforce_strict_secure);

// Helper for GarbageCollect(); can be called directly as well. Deletes
// all expired cookies in |itpair|. If |cookie_its| is non-NULL, it is
// populated with all the non-expired cookies from |itpair|.
// Helper for GarbageCollect(); can be called directly as well. Deletes all
// expired cookies in |itpair|. If |cookie_its| is non-NULL, all the
// non-expired cookies from |itpair| are appended to |cookie_its|.
//
// Returns the number of cookies deleted.
int GarbageCollectExpired(const base::Time& current,
const CookieMapItPair& itpair,
std::vector<CookieMap::iterator>* cookie_its);
size_t GarbageCollectExpired(const base::Time& current,
const CookieMapItPair& itpair,
CookieItVector* cookie_its);

// Helper for GarbageCollect(). Deletes all cookies not marked Secure in
// |valid_cookies_its|. If |cookie_its| is non-NULL, all the Secure cookies
// from |itpair| are appended to |cookie_its|.
//
// Returns the numeber of cookies deleted.
size_t GarbageCollectNonSecure(const CookieItVector& valid_cookies,
CookieItVector* cookie_its);

// Helper for GarbageCollect(). Deletes all cookies in the range specified by
// [|it_begin|, |it_end|). Returns the number of cookies deleted.
int GarbageCollectDeleteRange(const base::Time& current,
DeletionCause cause,
CookieItVector::iterator cookie_its_begin,
CookieItVector::iterator cookie_its_end);
size_t GarbageCollectDeleteRange(const base::Time& current,
DeletionCause cause,
CookieItVector::iterator cookie_its_begin,
CookieItVector::iterator cookie_its_end);

// Helper for GarbageCollect(). Deletes cookies in |cookie_its| from least to
// most recently used, but only before |safe_date|. Also will stop deleting
// when the number of remaining cookies hits |purge_goal|.
size_t GarbageCollectLeastRecentlyAccessed(const base::Time& current,
const base::Time& safe_date,
size_t purge_goal,
CookieItVector cookie_its);

// Find the key (for lookup in cookies_) based on the given domain.
// See comment on keys before the CookieMap typedef.
Expand Down
2 changes: 1 addition & 1 deletion net/cookies/cookie_monster_perftest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ TEST_F(CookieMonsterTest, TestGCTimes) {
for (int ci = 0; ci < static_cast<int>(arraysize(test_cases)); ++ci) {
const TestCase& test_case(test_cases[ci]);
scoped_refptr<CookieMonster> cm(CreateMonsterFromStoreForGC(
test_case.num_cookies, test_case.num_old_cookies,
test_case.num_cookies, test_case.num_old_cookies, 0, 0,
CookieMonster::kSafeFromGlobalPurgeDays * 2));

GURL gurl("http://google.com");
Expand Down
27 changes: 21 additions & 6 deletions net/cookies/cookie_monster_store_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,25 +196,40 @@ void MockSimplePersistentCookieStore::Flush(const base::Closure& callback) {
void MockSimplePersistentCookieStore::SetForceKeepSessionState() {
}

CookieMonster* CreateMonsterFromStoreForGC(int num_cookies,
int num_old_cookies,
CookieMonster* CreateMonsterFromStoreForGC(int num_secure_cookies,
int num_old_secure_cookies,
int num_non_secure_cookies,
int num_old_non_secure_cookies,
int days_old) {
base::Time current(base::Time::Now());
base::Time past_creation(base::Time::Now() - base::TimeDelta::FromDays(1000));
scoped_refptr<MockSimplePersistentCookieStore> store(
new MockSimplePersistentCookieStore);
int total_cookies = num_secure_cookies + num_non_secure_cookies;
int base = 0;
// Must expire to be persistent
for (int i = 0; i < num_cookies; i++) {
for (int i = 0; i < total_cookies; i++) {
int num_old_cookies;
bool secure;
if (i < num_secure_cookies) {
num_old_cookies = num_old_secure_cookies;
secure = true;
} else {
base = num_secure_cookies;
num_old_cookies = num_old_non_secure_cookies;
secure = false;
}
base::Time creation_time =
past_creation + base::TimeDelta::FromMicroseconds(i);
base::Time expiration_time = current + base::TimeDelta::FromDays(30);
base::Time last_access_time =
(i < num_old_cookies) ? current - base::TimeDelta::FromDays(days_old)
: current;
((i - base) < num_old_cookies)
? current - base::TimeDelta::FromDays(days_old)
: current;

CanonicalCookie cc(GURL(), "a", "1", base::StringPrintf("h%05d.izzle", i),
"/path", creation_time, expiration_time,
last_access_time, false, false, false,
last_access_time, secure, false, false,
COOKIE_PRIORITY_DEFAULT);
store->AddCookie(cc);
}
Expand Down
17 changes: 10 additions & 7 deletions net/cookies/cookie_monster_store_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,16 @@ class MockSimplePersistentCookieStore
// Helper function for creating a CookieMonster backed by a
// MockSimplePersistentCookieStore for garbage collection testing.
//
// Fill the store through import with |num_cookies| cookies, |num_old_cookies|
// with access time Now()-days_old, the rest with access time Now().
// Do two SetCookies(). Return whether each of the two SetCookies() took
// longer than |gc_perf_micros| to complete, and how many cookie were
// left in the store afterwards.
CookieMonster* CreateMonsterFromStoreForGC(int num_cookies,
int num_old_cookies,
// Fill the store through import with |num_*_cookies| cookies,
// |num_old_*_cookies| with access time Now()-days_old, the rest with access
// time Now(). Cookies made by |num_secure_cookies| and |num_non_secure_cookies|
// will be marked secure and non-secure, respectively. Do two SetCookies().
// Return whether each of the two SetCookies() took longer than |gc_perf_micros|
// to complete, and how many cookie were left in the store afterwards.
CookieMonster* CreateMonsterFromStoreForGC(int num_secure_cookies,
int num_old_secure_cookies,
int num_non_secure_cookies,
int num_old_non_secure_cookies,
int days_old);

} // namespace net
Expand Down
Loading

0 comments on commit 82d99c1

Please sign in to comment.