Add a method to override all the cookies in CookieMonster

net/cookies/cookie_monster.h

Index: net/cookies/cookie_monster.h
diff --git a/net/cookies/cookie_monster.h b/net/cookies/cookie_monster.h
index 79f1bbff6e07156912b6f31d556ca82a13346634..b0f76fceb5c7bcaa2a61c185ca0317e73068fb0f 100644
--- a/net/cookies/cookie_monster.h
+++ b/net/cookies/cookie_monster.h
@@ -234,6 +234,14 @@ class NET_EXPORT CookieMonster : public CookieStore {
   // to the thread you actually want to be notified on.
   void FlushStore(const base::Closure& callback);
 
+  // Replaces all the cookies by |list|.
+  // This method does not flush the backend, and does not update the creation
+  // time nor the last access time: if a cookie in |list| already exists in the
+  // cookie monster with a different time, the cookie is left untouched in the
+  // cookie monster.

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

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]

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]

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 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.

+  void SetAllCookiesAsync(const CookieList& list,
+                          const SetCookiesCallback& callback);
+
   // CookieStore implementation.
 
   // Sets the cookies specified by |cookie_list| returned from |url|
@@ -322,6 +330,7 @@ class NET_EXPORT CookieMonster : public CookieStore {
   class GetAllCookiesForURLWithOptionsTask;
   class GetAllCookiesTask;
   class GetCookiesWithOptionsTask;
+  class SetAllCookiesTask;
   class SetCookieWithDetailsTask;
   class SetCookieWithOptionsTask;
   class DeleteSessionCookiesTask;
@@ -350,6 +359,9 @@ class NET_EXPORT CookieMonster : public CookieStore {
   // For FindCookiesForKey.
   FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, ShortLivedSessionCookies);
 
+  // For ComputeCookieDiff.
+  FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, ComputeCookieDiff);
+
   // Internal reasons for deletion, used to populate informative histograms
   // and to provide a public cause for onCookieChange notifications.
   //
@@ -544,6 +556,9 @@ class NET_EXPORT CookieMonster : public CookieStore {
                           const base::Time& creation_time,
                           const CookieOptions& options);
 
+  // Helper function calling SetCanonicalCookie() for all cookies in |list|.
+  bool SetCanonicalCookies(const CookieList& list);
+
   void InternalUpdateCookieAccessTime(CanonicalCookie* cc,
                                       const base::Time& current_time);
 
@@ -608,6 +623,18 @@ class NET_EXPORT CookieMonster : public CookieStore {
   void DoCookieTaskForURL(const scoped_refptr<CookieMonsterTask>& task_item,
                           const GURL& url);
 
+  // Computes the difference between |old_cookies| and |new_cookies|, and writes
+  // the result in |cookies_to_add| and |cookies_to_delete|.
+  // This function has the side effect of changing the order of |old_cookies|
+  // and |new_cookies|. |cookies_to_add| and |cookies_to_delete| must be empty,
+  // and none of the arguments can be null.
+  // Creation time and last access time are ignored (cookies differing only by
+  // their time are considered identical).
+  void ComputeCookieDiff(CookieList* old_cookies,
+                         CookieList* new_cookies,
+                         CookieList* cookies_to_add,
+                         CookieList* cookies_to_delete);
+
   // Run all cookie changed callbacks that are monitoring |cookie|.
   // |removed| is true if the cookie was deleted.
   void RunCallbacks(const CanonicalCookie& cookie, bool removed);