Split initial load of cookies by domains

chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc

Index: chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc
===================================================================
--- chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc	(revision 103757)
+++ chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc	(working copy)
@@ -16,6 +16,7 @@
 #include "content/browser/browser_thread.h"
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
 
 class SQLitePersistentCookieStoreTest : public testing::Test {
  public:
@@ -23,25 +24,33 @@
       : ui_thread_(BrowserThread::UI),
         db_thread_(BrowserThread::DB),
         io_thread_(BrowserThread::IO),
-        event_(false, false) {
+        event_(false, false),
+        event_key_(false, false) {
   }
 
- protected:
   void OnLoaded(
       const std::vector<net::CookieMonster::CanonicalCookie*>& cookies) {
     cookies_ = cookies;
     event_.Signal();
   }
 
-  bool Load(std::vector<net::CookieMonster::CanonicalCookie*>* cookies) {
-    bool result =
-        store_->Load(base::Bind(&SQLitePersistentCookieStoreTest::OnLoaded,
+  void OnKeyLoaded(
+      const std::vector<net::CookieMonster::CanonicalCookie*>& cookies) {
+    cookies_ = cookies;
+    event_key_.Signal();
+  }
+
+  void Load(std::vector<net::CookieMonster::CanonicalCookie*>* cookies) {
+    store_->Load(base::Bind(&SQLitePersistentCookieStoreTest::OnLoaded,
                                 base::Unretained(this)));
     event_.Wait();
     *cookies = cookies_;
-    return result;
   }
 
+  void Sleep() {
+    base::PlatformThread::Sleep(1000);
+  }
+
   virtual void SetUp() {
     ui_thread_.Start();
     db_thread_.Start();
@@ -50,7 +59,7 @@
     store_ = new SQLitePersistentCookieStore(
         temp_dir_.path().Append(chrome::kCookieFilename));
     std::vector<net::CookieMonster::CanonicalCookie*> cookies;
-    ASSERT_TRUE(Load(&cookies));
+    Load(&cookies);
     ASSERT_EQ(0u, cookies.size());
     // Make sure the store gets written at least once.
     store_->AddCookie(
@@ -62,10 +71,12 @@
                                             false, false, true));
   }
 
+ protected:
   BrowserThread ui_thread_;
   BrowserThread db_thread_;
   BrowserThread io_thread_;
   base::WaitableEvent event_;
+  base::WaitableEvent event_key_;
   std::vector<net::CookieMonster::CanonicalCookie*> cookies_;
   ScopedTempDir temp_dir_;
   scoped_refptr<SQLitePersistentCookieStore> store_;
@@ -118,7 +129,7 @@
       temp_dir_.path().Append(chrome::kCookieFilename));
 
   // Reload and test for persistence
-  ASSERT_TRUE(Load(&cookies));
+  Load(&cookies);
   ASSERT_EQ(1U, cookies.size());
   ASSERT_STREQ("http://foo.bar", cookies[0]->Domain().c_str());
   ASSERT_STREQ("A", cookies[0]->Name().c_str());
@@ -135,10 +146,62 @@
       temp_dir_.path().Append(chrome::kCookieFilename));
 
   // Reload and check if the cookie has been removed.
-  ASSERT_TRUE(Load(&cookies));
+  Load(&cookies);
   ASSERT_EQ(0U, cookies.size());
 }
 
+// Test that priority load of cookies for a specfic domain key could be

[erikwright (departed), 2011/10/04 19:55:02]

This test is very clever and effective. In general, though, I'm afraid it may
some day end up flaky due to its use of Sleep. Chromium developers are trained
through much pain to never put "sleep" in a test except under extraordinary
conditions that protect the test from being flaky.

It would be pretty easy to modify it to not be flaky. Instead of posting a sleep
task to the DB thread, how about creating a second event object, and posting
tasks to Wait on the event. Then, you would have something like:

db_thread.Post(base::Bind(Event::Wait, db_event_));
store_->Load(base::Bind(OnLoaded, this));
store->LoadCookiesForKey("aaa.com", base::Bind(onKeyLoaded, this));
db_thread.Post(base::Bind(Event::Wait, db_event_));

// Now the DB-thread queue contains:
// (active:)
// 1. Wait (on db_event)
// (pending:)
// 2. "Init And Chain-Load First Domain"
// 3. Priority Load (aaa.com)
// 4. Wait (on db_event)

db_event->Signal();
io_event->Wait();  // will be signaled by the priority load's callback

// TODO(guohui): verify that the aaa.com cookies are there, but some other
// cookes are not.

// The next Chain-Load is blocked by a Wait on db_event_. Once we release it,
// the remaining chain loads will all occur, and the OnLoaded callback will
// fire (and signal io_event_).

db_event_->Signal();
io_event_->Wait();

// TODO(guohui): verify that all cookies have been loaded.

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

Done.

+// completed before the entire store is loaded
+TEST_F(SQLitePersistentCookieStoreTest, TestLoadCookiesForKey) {
+  std::vector<net::CookieMonster::CanonicalCookie*> cookies;
+  base::Time t = base::Time::Now() + base::TimeDelta::FromInternalValue(10);
+  store_->AddCookie(
+    net::CookieMonster::CanonicalCookie(GURL(), "A", "B",
+                                        "www.aaa.com", "/",

[erikwright (departed), 2011/10/04 19:55:02]

Add a note that "a foo.bar cookie was already added in setup."

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

Done.

+                                        std::string(), std::string(),
+                                        t, t, t, false, false, true));
+  t += base::TimeDelta::FromInternalValue(10);
+  store_->AddCookie(
+    net::CookieMonster::CanonicalCookie(GURL(), "A", "B",
+                                        "www.bbb.com", "/",
+                                        std::string(), std::string(),
+                                        t, t, t, false, false, true));
+  store_ = NULL;
+
+  scoped_refptr<base::ThreadTestHelper> helper(
+      new base::ThreadTestHelper(
+          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB)));
+  // Make sure we wait until the destructor has run.
+  ASSERT_TRUE(helper->Run());
+
+  store_ = new SQLitePersistentCookieStore(
+      temp_dir_.path().Append(chrome::kCookieFilename));
+  db_thread_.PostTask(BrowserThread::DB, FROM_HERE,
+    base::Bind(&SQLitePersistentCookieStoreTest::Sleep,
+               base::Unretained(this)));
+
+  store_->Load(base::Bind(&SQLitePersistentCookieStoreTest::OnLoaded,
+                          base::Unretained(this)));
+
+  store_->LoadCookiesForKey("aaa.com",
+    base::Bind(&SQLitePersistentCookieStoreTest::OnKeyLoaded,
+               base::Unretained(this)));
+  db_thread_.PostTask(BrowserThread::DB, FROM_HERE,
+    base::Bind(&SQLitePersistentCookieStoreTest::Sleep,
+               base::Unretained(this)));
+
+  event_key_.Wait();
+  ASSERT_EQ(event_.IsSignaled(), false);

[erikwright (departed), 2011/10/04 19:55:02]

If the order is guaranteed to be the order of creation for some reason besides a
side-effect that could reasonably change, please note that here and explain why.
Otherwise, consider creating a set of domains and comparing that, rather than
assuming the order.

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

Done.

+  ASSERT_EQ(2U, cookies_.size());
+  ASSERT_STREQ("http://foo.bar", cookies_[0]->Domain().c_str());
+  ASSERT_STREQ("www.aaa.com", cookies_[1]->Domain().c_str());
+
+  event_.Wait();
+  ASSERT_EQ(1U, cookies_.size());
+  ASSERT_STREQ("www.bbb.com", cookies_[0]->Domain().c_str());
+  // Reload and te

[erikwright (departed), 2011/10/04 19:55:02]

Remove or correct the comment on line 202.

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

Done.

+}
+
 // Test that we can force the database to be written by calling Flush().
 TEST_F(SQLitePersistentCookieStoreTest, TestFlush) {
   // File timestamps don't work well on all platforms, so we'll determine