Split initial load of cookies by domains

net/base/cookie_monster.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Brought to you by the letter D and the number 2.
 
 #ifndef NET_BASE_COOKIE_MONSTER_H_
 #define NET_BASE_COOKIE_MONSTER_H_
 #pragma once
 
 #include <map>
 #include <queue>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/synchronization/lock.h"
 #include "base/task.h"
 #include "base/time.h"
 #include "net/base/cookie_store.h"
 #include "net/base/net_export.h"
 
+class Closure;

[erikwright (departed), 2011/09/20 19:00:44]

This should go in the 'namespace base' block. You have just declared a new (and
unused) class named ::Closure, in addition to the base::Closure referenced
below.

[guohui, 2011/10/06 15:40:00]

Done.

 class GURL;
 
 namespace base {
 class Histogram;
 class TimeTicks;
-}
+}  // namespace base
 
 namespace net {
 
 class CookieList;
 
 // The cookie monster is the system for storing and retrieving cookies. It has
 // an in-memory list of all cookies, and synchronizes non-session cookies to an
 // optional permanent storage that implements the PersistentCookieStore
 // interface.
 //
 // This class IS thread-safe. Normally, it is only used on the I/O thread, but
 // is also accessed directly through Automation for UI testing.
 //
-// Several methods exist in asynchronous forms. Calls may be deferred if all
-// affected cookies are not yet loaded from the backing store. Otherwise, the
-// callback may be invoked immediately (prior to return of the asynchronous
+// All cookie tasks are handled asynchronously. Tasks may be deferred if
+// all affected cookies, are not yet loaded from the backing store. Otherwise,

[erikwright (departed), 2011/09/20 19:00:44]

nit: this comma ("all affected cookies, ") should be removed.

[guohui, 2011/10/06 15:40:00]

Done.

+// the callback may be invoked immediately (prior to return of the asynchronous
 // function).
 //
+// A cookie task is either pending loading of the entire cookie store, or
+// loading of cookies for a specfic domain key(eTLD+1). In the former case, the
+// cookie task will be queued in queue_ while PersistentCookieStore chain loads
+// the cookie store on DB thread. In the later case, the cookie task will be

[erikwright (departed), 2011/09/20 19:00:44]

nit: later -> latter

[guohui, 2011/10/06 15:40:00]

Done.

+// passed as a callback to PermanentCookieStore:LoadCookiesForKey() which
+// triggers loading of cookies for the specified domain key(eTLD+1) on DB
+// thread. Please refer to the link below for detailed design documents.
+// http://www.chromium.org/developers/design-documents/cookie-split-loading

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

Have you brought the design docs up to date?

I'd recommend not relying on design docs for details; they're less likely to be
kept in sync with the code than comments in the code.  Relying on them for a
high level overview is fine, but if there are details you want to communicate,
it should be in the comments in the code.

[guohui, 2011/10/06 15:40:00]

Done.

+//
 // Callbacks are guaranteed to be invoked on the calling thread.
 //
 // TODO(deanm) Implement CookieMonster, the cookie database.
 //  - Verify that our domain enforcement and non-dotted handling is correct
 class NET_EXPORT CookieMonster : public CookieStore {
  public:
   class CanonicalCookie;
   class Delegate;
   class ParsedCookie;
   class PersistentCookieStore;
@@ skipping 382 matching lines @@
     }
   }
 
   // Initializes the backing store and reads existing cookies from it.
   // Should only be called by InitIfNecessary().
   void InitStore();
 
   // Stores cookies loaded from the backing store and invokes any deferred
   // calls. |beginning_time| should be the moment PersistentCookieStore::Load
   // was invoked and is used for reporting histogram_time_load_.
+  // See PersistentCookieStore::Load for details on the contents of cookies.
   void OnLoaded(base::TimeTicks beginning_time,
                 const std::vector<CanonicalCookie*>& cookies);
 
+  // Stores cookies loaded from the backing store and invokes the deferred
+  // CookieMonsterTask pending loading of cookies associated with the domain key

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

The interface contract are these sentences with "pending loading of cookies
associated with the domain key" elided; the nature of the passed in task isn't
under control of this routine.  If you want to say that the request_task should
be the one that's associated with the domain key handed to the persistent store
from (I don't remember the source), that's fine, but it's not part of the
interface contract and I think the comment here will be better if that's clear.

[guohui, 2011/10/06 15:40:00]

Done.

+  // (eTLD+1). Called when all cookies for the domain key(eTLD+1) have been
+  // loaded from DB. See PersistentCookieStore::Load for details on the contents
+  // of cookies.
+  void CookieMonster::OnKeyLoaded(
+    const base::Closure& request_task, const std::string& key,
+    const std::vector<CanonicalCookie*>& cookies);
+
   // Stores the loaded cookies.
   void StoreLoadedCookies(const std::vector<CanonicalCookie*>& cookies);
 
   // Invokes deferred calls.
   void InvokeQueue();
 
   // Checks that |cookies_| matches our invariants, and tries to repair any
   // inconsistencies. (In other words, it does not have duplicate cookies).
   void EnsureCookiesMapIsValid();
 
@@ skipping 95 matching lines @@
   void RecordPeriodicStats(const base::Time& current_time);
 
   // Initialize the above variables; should only be called from
   // the constructor.
   void InitializeHistograms();
 
   // The resolution of our time isn't enough, so we do something
   // ugly and increment when we've seen the same time twice.
   base::Time CurrentTime();
 
-  // Run the cookie request task if cookie loaded, otherwise added the task
-  // to task queue.
+  // Runs the task if, or defers the task until, the full cookie database is
+  // loaded.
   void DoCookieTask(const scoped_refptr<CookieMonsterTask>& task_item);
 
+  // Runs the task if, or defers the task until, the cookies for the given URL
+  // are loaded.
+  void DoCookieTaskForURL(const scoped_refptr<CookieMonsterTask>& task_item,
+    const GURL& url);
+
   // Histogram variables; see CookieMonster::InitializeHistograms() in
   // cookie_monster.cc for details.
   base::Histogram* histogram_expiration_duration_minutes_;
   base::Histogram* histogram_between_access_interval_minutes_;
   base::Histogram* histogram_evicted_last_access_minutes_;
   base::Histogram* histogram_count_;
   base::Histogram* histogram_domain_count_;
   base::Histogram* histogram_etldp1_count_;
   base::Histogram* histogram_domain_per_etldp1_count_;
   base::Histogram* histogram_number_duplicate_db_cookies_;
   base::Histogram* histogram_cookie_deletion_cause_;
   base::Histogram* histogram_time_get_;
   base::Histogram* histogram_time_mac_;
   base::Histogram* histogram_time_load_;
 
   CookieMap cookies_;
 
   // Indicates whether the cookie store has been initialized. This happens
   // lazily in InitStoreIfNecessary().
   bool initialized_;
 
   // Indicates whether loading from the backend store is completed and
   // calls may be immediately processed.
   bool loaded_;
 
+  // List of domain keys that have been loaded from DB
+  std::set<std::string> keys_loaded_;
+
   // Queues calls to CookieMonster until loading from the backend store is
   // completed.
   std::queue<scoped_refptr<CookieMonsterTask> > queue_;
 
   // Indicates whether this cookie monster uses the new effective domain
   // key scheme or not.
   ExpiryAndKeyScheme expiry_and_key_scheme_;
 
   scoped_refptr<PersistentCookieStore> store_;
 
   base::Time last_time_seen_;
 
   // Minimum delay after updating a cookie's LastAccessDate before we will
   // update it again.
   const base::TimeDelta last_access_threshold_;
 
   // Approximate date of access time of least recently accessed cookie
   // in |cookies_|.  Note that this is not guaranteed to be accurate, only a)
   // to be before or equal to the actual time, and b) to be accurate
   // immediately after a garbage collection that scans through all the cookies.
   // This value is used to determine whether global garbage collection might
   // find cookies to purge.
   // Note: The default Time() constructor will create a value that compares
-  // earlier than any other time value, which is is wanted.  Thus this
+  // earlier than any other time value, which is wanted.  Thus this
   // value is not initialized.
   base::Time earliest_access_time_;
 
+  // During loading, holds the set of all loaded cookie creation times. Used to
+  // avoid ever letting cookies with duplicate creation times into the store;
+  // that way we don't have to worry about what sections of code are safe
+  // to call while it's in that state.
+  std::set<int64> creation_times_;
+
   std::vector<std::string> cookieable_schemes_;
 
   scoped_refptr<Delegate> delegate_;
 
   // Lock for thread-safety
   base::Lock lock_;
 
   base::Time last_statistic_record_time_;
 
   bool keep_expired_cookies_;
@@ skipping 275 matching lines @@
 
 class CookieMonster::PersistentCookieStore
     : public RefcountedPersistentCookieStore {
  public:
   virtual ~PersistentCookieStore() {}
 
   typedef base::Callback<void(const std::vector<
       CookieMonster::CanonicalCookie*>&)> LoadedCallback;
 
   // Initializes the store and retrieves the existing cookies. This will be
-  // called only once at startup.
-  virtual bool Load(const LoadedCallback& loaded_callback) = 0;
+  // called only once at startup. The callback will return all the cookies

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

General note: If you want to refer to arguments to a function you're commented
(or code in general) the convention is to do so by surrounding the code fragment
with vertical bars (e.g. |loaded_callback|).  I haven't seen places where you
literally refer to arguments without doing this convention, so there's no
request for change here, but there's enough conceptual references that I wanted
to make sure you had this tool in your toolbox.

[guohui, 2011/10/06 15:40:00]

Thanks for sharing the information, I was not aware of this convention ^_^

On 2011/09/21 20:52:39, rdsmith wrote:

+  // that are not yet returned to CookieMonster by previous priority loads.

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

You use the phrase "priority loads" but don't define it (both here and in the
.cc file).  It's pretty clear what you mean (any cookies loaded through
LoadCookiesForKey count as priority loads) but that clarity has come after
reading the code pretty carefully, so I'd like to surface it better in the
comments.  I'd suggest you just change the description below to be "Do a
priority load of all cookies for the domain key (eTLD+1)."

[guohui, 2011/10/06 15:40:00]

Done.

+  virtual void Load(const LoadedCallback& loaded_callback) = 0;
+
+  // Loads all cookies for the domain key(eTLD+1). The callback will return all
+  // the cookies that are not yet returned by previous loads, which includes
+  // cookies for the requested domain key if they are not already returned, plus
+  // all cookies that are chain-loaded and not yet returned to CookieMonster.
+  virtual void LoadCookiesForKey(const std::string& key,
+    const LoadedCallback& loaded_callback) = 0;
 
   virtual void AddCookie(const CanonicalCookie& cc) = 0;
   virtual void UpdateCookieAccessTime(const CanonicalCookie& cc) = 0;
   virtual void DeleteCookie(const CanonicalCookie& cc) = 0;
 
   // Sets the value of the user preference whether the persistent storage
   // must be deleted upon destruction.
   virtual void SetClearLocalStateOnExit(bool clear_local_state) = 0;
 
   // Flush the store and post the given Task when complete.
   virtual void Flush(Task* completion_task) = 0;
 
  protected:
   PersistentCookieStore() {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PersistentCookieStore);
 };
 
 class CookieList : public std::vector<CookieMonster::CanonicalCookie> {
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_COOKIE_MONSTER_H_