Add a method to override all the cookies in CookieMonster

Add a method to override all the cookies in CookieMonster

This method is required in order to upstream CookieStoreIOS.

CookieStoreIOS can periodically synchronize the iOS system
cookie store (NSHTTPCookieStorage) to CookieMonster.

It is not possible to simply delete all the cookies and re-add
the new version of the cookies, because this can lead to cookie
loss if the application crashes right after deleting all cookies.
For this reason, this CL introduces a new SetAllCookies method
that computes the exact difference between the exiting cookies and
the new cookies, and does only the minimum of operations needed.

[droger, 2015-03-12 15:23:45]

Patchset #1 (id:1) has been deleted

[droger, 2015-03-12 15:33:22]

Patchset #1 (id:20001) has been deleted

[droger, 2015-03-12 15:50:29]

droger@chromium.org changed reviewers:
+ erikwright@chromium.org

[droger, 2015-03-12 15:50:29]

fyi: this code has been used in production on iOS for ~1 year. This CL is an
attempt to upstream it.

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.h (right):

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.h:241: // cookie monster.
I don't know if this behavior w.r.t. creation time and last access time makes
sense.
If you think it does not, I can check if it can be removed, I think it is here
mainly for performance reasons (to minimize the updates on the backend).

[erikwright (departed), 2015-03-13 13:59:01]

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2357: // It is OK to call set_difference using
PartialDiffCookieSorter even though
// A set ordered by FullDiffCookieSorter is also ordered by
PartialDiffCookieSorter.

Would it be a terrible idea to do:

DCHECK(std::is_sorted(old_cookies->begin(), old_cookies->end(),
PartialDiffCookieSorter));
... also for new_cookies ...

?

Especially where those functions are buried in the anonymous namespace and not
tested. Alternatively, pull them out somewhere and test them.

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2359: std::set_difference(
How about the following comment.

// Select any old cookie for deletion if no new cookie has the same name,
domain, and path

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2366: std::set_difference(new_cookies->begin(),
new_cookies->end(),
// Select any new cookie for addition (or update) if no old cookie is exactly
equivalent (excluding last access and creation times).

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.h (right):

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.h:241: // cookie monster.
On 2015/03/12 15:50:29, droger wrote:
> I don't know if this behavior w.r.t. creation time and last access time makes
> sense.
> If you think it does not, I can check if it can be removed, I think it is here
> mainly for performance reasons (to minimize the updates on the backend).

Presumably the rationale for not needing to update the last_access_time is that
expiry is being handled by the iOS cookie store anyways? Or because we are
handling the network requests and iOS is only handling the JS cookie APIs, and
thus we assume that anything JS is accessing has already been touched recently
by the network stack?

I'm not sure, off the top of my head, what creation_time is used for.

I can see an edge case where this matters. If JS in a page is setting many
cookies, we might not evict the right ones, as the last access ordering will not
be meaningful.

At a minimum I'd like more information about why this was considered "safe" and
whether there is any justification for why it is "necessary".

[erikwright (departed), 2015-03-13 13:59:02]

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2357: // It is OK to call set_difference using
PartialDiffCookieSorter even though
// A set ordered by FullDiffCookieSorter is also ordered by
PartialDiffCookieSorter.

Would it be a terrible idea to do:

DCHECK(std::is_sorted(old_cookies->begin(), old_cookies->end(),
PartialDiffCookieSorter));
... also for new_cookies ...

?

Especially where those functions are buried in the anonymous namespace and not
tested. Alternatively, pull them out somewhere and test them.

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2359: std::set_difference(
How about the following comment.

// Select any old cookie for deletion if no new cookie has the same name,
domain, and path

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2366: std::set_difference(new_cookies->begin(),
new_cookies->end(),
// Select any new cookie for addition (or update) if no old cookie is exactly
equivalent (excluding last access and creation times).

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.h (right):

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.h:241: // cookie monster.
On 2015/03/12 15:50:29, droger wrote:
> I don't know if this behavior w.r.t. creation time and last access time makes
> sense.
> If you think it does not, I can check if it can be removed, I think it is here
> mainly for performance reasons (to minimize the updates on the backend).

Presumably the rationale for not needing to update the last_access_time is that
expiry is being handled by the iOS cookie store anyways? Or because we are
handling the network requests and iOS is only handling the JS cookie APIs, and
thus we assume that anything JS is accessing has already been touched recently
by the network stack?

I'm not sure, off the top of my head, what creation_time is used for.

I can see an edge case where this matters. If JS in a page is setting many
cookies, we might not evict the right ones, as the last access ordering will not
be meaningful.

At a minimum I'd like more information about why this was considered "safe" and
whether there is any justification for why it is "necessary".

[droger, 2015-03-13 15:10:47]

Thanks for the comments, this is interesting.

It seems that at least the last access date is important.
Even if we don't/can't support them correctly on iOS, maybe this function on
CookieMonster should support it (because CookieMonster is not iOS-specific).

I don't really know about the creation date.

I'll experiment some more, and see if I can just drop that specific behavior
with respect to these creation and last access times.

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.h (right):

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.h:241: // cookie monster.
On 2015/03/13 13:59:01, erikwright wrote:
> Presumably the rationale for not needing to update the last_access_time is
that
> expiry is being handled by the iOS cookie store anyways? Or because we are
> handling the network requests and iOS is only handling the JS cookie APIs, and
> thus we assume that anything JS is accessing has already been touched recently
> by the network stack?

Some context: on iOS when a WebView is attached to a request context, we switch
to the system cookie store as a source of truth (both for the net stack and for
javascript). When this happen, we periodically copy all the cookies to cookie
monster (so that cookies can be persisted on disk), using that new
SetAllCookiesAsync() function.

When web views are detached from the request context, the source of truth is
moved back to cookie monster again.

Now, to answer your question: the system store does not have a notion of "last
access", and so we just don't bother with them at all. When we convert NSHTTP
cookies to canonical cookies, we use an invalid time base::Time().

So far it seems that it's been working, although now that I read the code, I see
that it is used to evict cookies when a domain has too many. This means that the
cookie eviction is probably broken (depending on how the system store handles
eviction by itself). It is not clear if/how it can be fixed though:

it would be possible to keep track of the last access time from the network
stack, because we see the calls in CookieStoreIOS, but we can't do anything for
javascript access.

> 
> I'm not sure, off the top of my head, what creation_time is used for.

I don't exactly know either. It is at least used to uniquely identify cookies.
For example DeleteCanonicalCookie is based on that, and also the SQL database
completely crashes if two cookies share the same creation time.
It seemed that updating the creation time as part of this new
SetAllCookiesAsync() was not very useful, and it is unclear if we can support
that or not.

> 
> I can see an edge case where this matters. If JS in a page is setting many
> cookies, we might not evict the right ones, as the last access ordering will
not
> be meaningful.
> 
> At a minimum I'd like more information about why this was considered "safe"
and
> whether there is any justification for why it is "necessary".

[droger, 2015-03-16 13:00:55]

Patchset #2 (id:60001) has been deleted

[droger, 2015-03-16 13:03:50]

I removed the specific treatment for creation time and last access time, as it
seems to make little sense given the earlier comments in this review.

I did some tests, and it does not seem to add significant churn on iOS, very few
additional cookies get updated.

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2357: // It is OK to call set_difference using
PartialDiffCookieSorter even though
On 2015/03/13 13:59:01, erikwright wrote:
> // A set ordered by FullDiffCookieSorter is also ordered by
> PartialDiffCookieSorter.
> 
> Would it be a terrible idea to do:
> 
> DCHECK(std::is_sorted(old_cookies->begin(), old_cookies->end(),
> PartialDiffCookieSorter));
> ... also for new_cookies ...
> 
> ?
> 
> Especially where those functions are buried in the anonymous namespace and not
> tested. Alternatively, pull them out somewhere and test them.

Comment updated.
Unfortunately it seems that std::is_sorted is in the C++11 stdlib only.
I have moved these methods to the CanonicalCookie class and added unittests.

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2359: std::set_difference(
On 2015/03/13 13:59:01, erikwright wrote:
> How about the following comment.
> 
> // Select any old cookie for deletion if no new cookie has the same name,
> domain, and path

Done.

https://codereview.chromium.org/1000103002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2366: std::set_difference(new_cookies->begin(),
new_cookies->end(),
On 2015/03/13 13:59:01, erikwright wrote:
> // Select any new cookie for addition (or update) if no old cookie is exactly
> equivalent (excluding last access and creation times).

Done.

[erikwright (departed), 2015-03-17 17:46:16]

LGTM

https://codereview.chromium.org/1000103002/diff/80001/net/cookies/cookie_mons...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1000103002/diff/80001/net/cookies/cookie_mons...
net/cookies/cookie_monster_unittest.cc:2186: // SetAllCookies must not flush.
It should, presumably, still update the backend, no?

[droger, 2015-03-17 17:48:57]

https://codereview.chromium.org/1000103002/diff/80001/net/cookies/cookie_mons...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1000103002/diff/80001/net/cookies/cookie_mons...
net/cookies/cookie_monster_unittest.cc:2186: // SetAllCookies must not flush.
On 2015/03/17 17:46:16, erikwright wrote:
> It should, presumably, still update the backend, no?

Yes.

[droger, 2015-03-17 18:09:48]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-03-17 18:10:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1000103002/80001

[commit-bot: I haz the power, 2015-03-17 19:22:48]

Patchset 2 (id:??) landed as
https://crrev.com/d5d1278c27a9964ace098aa3717a84a301bb19c7
Cr-Commit-Position: refs/heads/master@{#320954}

[commit-bot: I haz the power, 2015-03-17 19:24:12]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-03-17 19:26:37]

Committed patchset #2 (id:80001)