SQLitePersistentCookieStore: recover from uniqueness violation on V9->V10 migration

We can do better than just dropping the DB, by dropping just the older
versions of the duplicate cookie --- this is how M65 would behave when
loading the store, so it preserves old behavior.

This is probably too late for Chrome, but may help other embedders.

Bug: 835990
Change-Id: I25bf87d46574346a47c05b35152fa3c42f9aa7a9
Reviewed-on: https://chromium-review.googlesource.com/1097204Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567018}

net/extras/sqlite/sqlite_persistent_cookie_store.cc

@@ -1178,26 +1178,18 @@ bool SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
       return false;
     }
     // If any cookies violate the new uniqueness constraints (no two
-    // cookies with the same (name, domain, path)) erase the cookie store.
-    // That "shouldn't happen", which means probably not too many users'
-    // cookie stores will have it.
-    // The choice to drop rather than pick one of the cookies randomly is
-    // because it is expected that servers will be able to deal with a known
-    // state (no cookies == first visit), and there may be cookie values they
-    // may not be able to deal with.
+    // cookies with the same (name, domain, path)), pick the newer version,
+    // since that's what CookieMonster would do anyway.
     if (!db_->Execute(
-            "INSERT OR FAIL INTO cookies "
+            "INSERT OR REPLACE INTO cookies "
             "(creation_utc, host_key, name, value, path, expires_utc, "
             "is_secure, is_httponly, last_access_utc, has_expires, "
             "is_persistent, priority, encrypted_value, firstpartyonly) "
             "SELECT creation_utc, host_key, name, value, path, expires_utc, "
             "       secure, httponly, last_access_utc, has_expires, "
             "       persistent, priority, encrypted_value, firstpartyonly "
-            "FROM cookies_old")) {
-      // The old database had duplicate cookies in a way that violates
-      // the spec.  Treat that as DB corruption and start with a clean slate.
-      if (!db_->Execute("DELETE FROM COOKIES;"))
-        return false;
+            "FROM cookies_old ORDER BY creation_utc ASC")) {
+      return false;
     }
     if (!db_->Execute("DROP TABLE cookies_old"))
       return false;

net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc

@@ -963,6 +963,9 @@ bool CreateV9Schema(sql::Connection* db) {
   return true;
 }
 
+bool AddV9CookiesToDBImpl(sql::Connection* db,
+                          const std::vector<CanonicalCookie>& cookies);
+
 // Add a selection of cookies to the DB.
 bool AddV9CookiesToDB(sql::Connection* db) {
   static base::Time cookie_time(base::Time::Now());
@@ -992,7 +995,11 @@ bool AddV9CookiesToDB(sql::Connection* db) {
       "C", "B", "example.com", "/path", cookie_time, cookie_time, cookie_time,
       false, false, CookieSameSite::DEFAULT_MODE, COOKIE_PRIORITY_DEFAULT));
   cookie_time += base::TimeDelta::FromMicroseconds(1);
+  return AddV9CookiesToDBImpl(db, cookies);
+}
 
+bool AddV9CookiesToDBImpl(sql::Connection* db,
+                          const std::vector<CanonicalCookie>& cookies) {
   sql::Statement add_smt(db->GetCachedStatement(
       SQL_FROM_HERE,
       "INSERT INTO cookies (creation_utc, host_key, name, value, "
@@ -1045,31 +1052,37 @@ void ConfirmV9CookiesFromDB(
   std::sort(read_in_cookies.begin(), read_in_cookies.end(), &CompareCookies);
   int i = 0;
   EXPECT_EQ("A", read_in_cookies[i]->Name());
+  EXPECT_EQ("B", read_in_cookies[i]->Value());
   EXPECT_EQ("example.com", read_in_cookies[i]->Domain());
   EXPECT_EQ("/", read_in_cookies[i]->Path());
 
   i++;
   EXPECT_EQ("A", read_in_cookies[i]->Name());
+  EXPECT_EQ("B", read_in_cookies[i]->Value());
   EXPECT_EQ("example.com", read_in_cookies[i]->Domain());
   EXPECT_EQ("/path", read_in_cookies[i]->Path());
 
   i++;
   EXPECT_EQ("A", read_in_cookies[i]->Name());
+  EXPECT_EQ("B", read_in_cookies[i]->Value());
   EXPECT_EQ("example2.com", read_in_cookies[i]->Domain());
   EXPECT_EQ("/", read_in_cookies[i]->Path());
 
   i++;
   EXPECT_EQ("C", read_in_cookies[i]->Name());
+  EXPECT_EQ("B", read_in_cookies[i]->Value());
   EXPECT_EQ("example.com", read_in_cookies[i]->Domain());
   EXPECT_EQ("/", read_in_cookies[i]->Path());
 
   i++;
   EXPECT_EQ("C", read_in_cookies[i]->Name());
+  EXPECT_EQ("B", read_in_cookies[i]->Value());
   EXPECT_EQ("example.com", read_in_cookies[i]->Domain());
   EXPECT_EQ("/path", read_in_cookies[i]->Path());
 
   i++;
   EXPECT_EQ("C", read_in_cookies[i]->Name());
+  EXPECT_EQ("B", read_in_cookies[i]->Value());
   EXPECT_EQ("example2.com", read_in_cookies[i]->Domain());
   EXPECT_EQ("/", read_in_cookies[i]->Path());
 }
@@ -1100,16 +1113,30 @@ TEST_F(SQLitePersistentCookieStoreTest, UpgradeToSchemaVersion10Corrupted) {
 
   ASSERT_TRUE(CreateV9Schema(&connection));
 
+  base::Time old_time = base::Time::Now() - base::TimeDelta::FromMinutes(90);
+  base::Time old_time2 = base::Time::Now() - base::TimeDelta::FromMinutes(91);
+  CanonicalCookie old_cookie1(
+      "A", "old_value", "example.com", "/", old_time, old_time, old_time, false,
+      false, CookieSameSite::DEFAULT_MODE, COOKIE_PRIORITY_DEFAULT);
+  AddV9CookiesToDBImpl(&connection, {old_cookie1});
+
   // Add the same set of cookies twice to create duplicates.
   ASSERT_TRUE(AddV9CookiesToDB(&connection));
   ASSERT_TRUE(AddV9CookiesToDB(&connection));
 
+  // Add some others as well.
+  CanonicalCookie old_cookie2(
+      "A", "old_value", "example.com", "/path", old_time2, old_time2, old_time2,
+      false, false, CookieSameSite::DEFAULT_MODE, COOKIE_PRIORITY_DEFAULT);
+  AddV9CookiesToDBImpl(&connection, {old_cookie2});
+
   connection.Close();
 
   std::vector<std::unique_ptr<CanonicalCookie>> read_in_cookies;
   CreateAndLoad(false, false, &read_in_cookies);
-  // Finding failures of the uniqueness constraint should result in a nuked DB.
-  EXPECT_EQ(0u, read_in_cookies.size());
+  // Finding failures of the uniqueness constraint should resolve them by
+  // timestamp.
+  ConfirmV9CookiesFromDB(std::move(read_in_cookies));
 }
 
 // Confirm the store can handle having cookies with identical creation