Add two new columns to cookies database schema

This adds two new columns, source_port and is_same_party, to the schema
for the SQLitePersistentCookieStore. These are not yet hooked up to
anything. The default values are -1 and false, respectively.

Bug: 1142606, 1141135
Change-Id: Id2774272b62c8070a5a2eab5a765937f124a764a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505468Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823245}

net/extras/sqlite/sqlite_persistent_cookie_store.cc

@@ -19,6 +19,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
 #include "base/sequenced_task_runner.h"
@@ -104,6 +105,10 @@ const int kLoadDelayMilliseconds = 0;
 const int kLoadDelayMilliseconds = 0;
 #endif
 
+// Port number to use for cookies whose source port is unknown at the time of
+// database migration to V13.
+const int kDefaultUnknownPort = -1;
+
 // A little helper to help us log (on client thread) if the background runner
 // gets stuck.
 class TimeoutTracker : public base::RefCountedThreadSafe<TimeoutTracker> {
@@ -148,6 +153,7 @@ namespace {
 
 // Version number of the database.
 //
+// Version 13 - 2020/10/28 - https://crrev.com/c/2505468
 // Version 12 - 2019/11/20 - https://crrev.com/c/1898301
 // Version 11 - 2019/04/17 - https://crrev.com/c/1570416
 // Version 10 - 2018/02/13 - https://crrev.com/c/906675
@@ -160,6 +166,12 @@ namespace {
 // Version 5  - 2011/12/05 - https://codereview.chromium.org/8533013
 // Version 4  - 2009/09/01 - https://codereview.chromium.org/183021
 //
+// Version 13 adds two new fields: "source_port" (the port number of the source
+// origin, and "same_party" (boolean indicating whether the cookie had a
+// SameParty attribute). In migrating, source_port defaults to -1 for old
+// entries for which the source port is unknown, and same_party defaults to
+// false.
+//
 // Version 12 adds a column for "source_scheme" to store whether the
 // cookie was set from a URL with a cryptographic scheme.
 //
@@ -211,8 +223,8 @@ namespace {
 // Version 3 updated the database to include the last access time, so we can
 // expire them in decreasing order of use when we've reached the maximum
 // number of cookies.
-const int kCurrentVersionNumber = 12;
-const int kCompatibleVersionNumber = 12;
+const int kCurrentVersionNumber = 13;
+const int kCompatibleVersionNumber = 13;
 
 }  // namespace
 
@@ -608,7 +620,7 @@ bool CreateV11Schema(sql::Database* db) {
 
 // Initializes the cookies table, returning true on success.
 // The table cannot exist when calling this function.
-bool CreateV12Schema(sql::Database* db) {
+bool CreateV13Schema(sql::Database* db) {
   DCHECK(!db->DoesTableExist("cookies"));
 
   std::string stmt(base::StringPrintf(
@@ -628,10 +640,12 @@ bool CreateV12Schema(sql::Database* db) {
       "encrypted_value BLOB DEFAULT '',"
       "samesite INTEGER NOT NULL DEFAULT %d,"
       "source_scheme INTEGER NOT NULL DEFAULT %d,"
+      "source_port INTEGER NOT NULL DEFAULT %d,"
+      "is_same_party INTEGER NOT NULL DEFAULT 0,"
       "UNIQUE (host_key, name, path))",
       CookiePriorityToDBCookiePriority(COOKIE_PRIORITY_DEFAULT),
       CookieSameSiteToDBCookieSameSite(CookieSameSite::UNSPECIFIED),
-      static_cast<int>(CookieSourceScheme::kUnset)));
+      static_cast<int>(CookieSourceScheme::kUnset), kDefaultUnknownPort));
   if (!db->Execute(stmt.c_str()))
     return false;
 
@@ -791,7 +805,7 @@ bool SQLitePersistentCookieStore::Backend::CreateDatabaseSchema() {
   if (db()->DoesTableExist("cookies"))
     return true;
 
-  return CreateV12Schema(db());
+  return CreateV13Schema(db());
 }
 
 bool SQLitePersistentCookieStore::Backend::DoInitializeDatabase() {
@@ -1019,7 +1033,7 @@ SQLitePersistentCookieStore::Backend::DoMigrateDatabaseSchema() {
     meta_table()->SetCompatibleVersionNumber(
         std::min(cur_version, kCompatibleVersionNumber));
     transaction.Commit();
-    UMA_HISTOGRAM_TIMES("Cookie.TimeDatabaseMigrationToV10",
+    base::UmaHistogramTimes("Cookie.TimeDatabaseMigrationToV10",
                             base::TimeTicks::Now() - start_time);
   }
 
@@ -1085,6 +1099,41 @@ SQLitePersistentCookieStore::Backend::DoMigrateDatabaseSchema() {
     transaction.Commit();
   }
 
+  if (cur_version == 12) {
+    const char kMigrationSuccessHistogram[] =
+        "Cookie.TimeDatabaseMigrationToV13Success";
+    const char kMigrationFailureHistogram[] =
+        "Cookie.TimeDatabaseMigrationToV13Failure";
+    const base::TimeTicks start_time = base::TimeTicks::Now();
+
+    sql::Transaction transaction(db());
+    if (!transaction.Begin()) {
+      base::UmaHistogramTimes(kMigrationFailureHistogram,
+                              base::TimeTicks::Now() - start_time);
+      return base::nullopt;
+    }
+
+    std::string update_stmt(
+        base::StringPrintf("ALTER TABLE cookies ADD COLUMN source_port "
+                           "INTEGER NOT NULL DEFAULT %d;"
+                           "ALTER TABLE cookies ADD COLUMN is_same_party "
+                           "INTEGER NOT NULL DEFAULT 0;",
+                           kDefaultUnknownPort));
+    if (!db()->Execute(update_stmt.c_str())) {
+      base::UmaHistogramTimes(kMigrationFailureHistogram,
+                              base::TimeTicks::Now() - start_time);
+      return base::nullopt;
+    }
+
+    ++cur_version;
+    meta_table()->SetVersionNumber(cur_version);
+    meta_table()->SetCompatibleVersionNumber(
+        std::min(cur_version, kCompatibleVersionNumber));
+    transaction.Commit();
+    base::UmaHistogramTimes(kMigrationSuccessHistogram,
+                            base::TimeTicks::Now() - start_time);
+  }
+
   // Put future migration cases here.
 
   return base::make_optional(cur_version);
@@ -1188,8 +1237,8 @@ void SQLitePersistentCookieStore::Backend::DoCommit() {
       "INSERT INTO cookies (creation_utc, host_key, name, value, "
       "encrypted_value, path, expires_utc, is_secure, is_httponly, "
       "samesite, last_access_utc, has_expires, is_persistent, priority,"
-      "source_scheme) "
-      "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)"));
+      "source_scheme, source_port, is_same_party) "
+      "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)"));
   if (!add_smt.is_valid())
     return;
 
@@ -1238,16 +1287,20 @@ void SQLitePersistentCookieStore::Backend::DoCommit() {
           }
           add_smt.BindString(5, po->cc().Path());
           add_smt.BindInt64(6, po->cc().ExpiryDate().ToInternalValue());
-          add_smt.BindInt(7, po->cc().IsSecure());
-          add_smt.BindInt(8, po->cc().IsHttpOnly());
+          add_smt.BindBool(7, po->cc().IsSecure());
+          add_smt.BindBool(8, po->cc().IsHttpOnly());
           add_smt.BindInt(
               9, CookieSameSiteToDBCookieSameSite(po->cc().SameSite()));
           add_smt.BindInt64(10, po->cc().LastAccessDate().ToInternalValue());
-          add_smt.BindInt(11, po->cc().IsPersistent());
-          add_smt.BindInt(12, po->cc().IsPersistent());
+          add_smt.BindBool(11, po->cc().IsPersistent());
+          add_smt.BindBool(12, po->cc().IsPersistent());
           add_smt.BindInt(
               13, CookiePriorityToDBCookiePriority(po->cc().Priority()));
           add_smt.BindInt(14, static_cast<int>(po->cc().SourceScheme()));
+          // TODO(crbug.com/1141135): Record port number of the cookie.
+          add_smt.BindInt(15, kDefaultUnknownPort);
+          // TODO(crbug.com/1142606): Record SameParty attribute of the cookie.
+          add_smt.BindBool(16, false);
           if (!add_smt.Run()) {
             DLOG(WARNING) << "Could not add a cookie to the DB.";
             RecordCookieCommitProblem(COOKIE_COMMIT_PROBLEM_ADD);

net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc

@@ -1659,11 +1659,39 @@ bool CreateV11Schema(sql::Database* db) {
   return true;
 }
 
-bool AddV11CookiesToDBImpl(sql::Database* db,
-                           const std::vector<CanonicalCookie>& cookies);
+bool CreateV12Schema(sql::Database* db) {
+  sql::MetaTable meta_table;
+  if (!meta_table.Init(db, /* version = */ 12,
+                       /* earliest compatible version = */ 12)) {
+    return false;
+  }
 
-// Add a selection of cookies to the DB.
-bool AddV11CookiesToDB(sql::Database* db) {
+  // Version 12 schema
+  std::string stmt(
+      "CREATE TABLE cookies("
+      "creation_utc INTEGER NOT NULL,"
+      "host_key TEXT NOT NULL,"
+      "name TEXT NOT NULL,"
+      "value TEXT NOT NULL,"
+      "path TEXT NOT NULL,"
+      "expires_utc INTEGER NOT NULL,"
+      "is_secure INTEGER NOT NULL,"
+      "is_httponly INTEGER NOT NULL,"
+      "last_access_utc INTEGER NOT NULL,"
+      "has_expires INTEGER NOT NULL DEFAULT 1,"
+      "is_persistent INTEGER NOT NULL DEFAULT 1,"
+      "priority INTEGER NOT NULL DEFAULT 1,"  // COOKIE_PRIORITY_DEFAULT
+      "encrypted_value BLOB DEFAULT '',"
+      "samesite INTEGER NOT NULL DEFAULT -1,"      // UNSPECIFIED
+      "source_scheme INTEGER NOT NULL DEFAULT 0,"  // CookieSourceScheme::kUnset
+      "UNIQUE (host_key, name, path))");
+  if (!db->Execute(stmt.c_str()))
+    return false;
+
+  return true;
+}
+
+std::vector<CanonicalCookie> CookiesForMigrationTest() {
   static base::Time now = base::Time::Now();
 
   std::vector<CanonicalCookie> cookies;
@@ -1694,11 +1722,11 @@ bool AddV11CookiesToDB(sql::Database* db) {
       CanonicalCookie("C", "B", "example.com", "/path", now, now, now,
                       false /* secure */, false /* httponly */,
                       CookieSameSite::UNSPECIFIED, COOKIE_PRIORITY_DEFAULT));
-  return AddV11CookiesToDBImpl(db, cookies);
+  return cookies;
 }
 
-bool AddV11CookiesToDBImpl(sql::Database* db,
-                           const std::vector<CanonicalCookie>& cookies) {
+bool AddV11CookiesToDB(sql::Database* db) {
+  std::vector<CanonicalCookie> cookies = CookiesForMigrationTest();
   sql::Statement add_smt(db->GetCachedStatement(
       SQL_FROM_HERE,
       "INSERT INTO cookies (creation_utc, host_key, name, value, "
@@ -1745,8 +1773,56 @@ bool AddV11CookiesToDBImpl(sql::Database* db,
   return true;
 }
 
+bool AddV12CookiesToDB(sql::Database* db) {
+  std::vector<CanonicalCookie> cookies = CookiesForMigrationTest();
+  sql::Statement add_smt(db->GetCachedStatement(
+      SQL_FROM_HERE,
+      "INSERT INTO cookies (creation_utc, host_key, name, value, "
+      "encrypted_value, path, expires_utc, is_secure, is_httponly, "
+      "samesite, last_access_utc, has_expires, is_persistent, priority, "
+      "source_scheme)"
+      "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)"));
+  if (!add_smt.is_valid())
+    return false;
+  sql::Transaction transaction(db);
+  transaction.Begin();
+  for (const CanonicalCookie& cookie : cookies) {
+    add_smt.Reset(true);
+    add_smt.BindInt64(
+        0, cookie.CreationDate().ToDeltaSinceWindowsEpoch().InMicroseconds());
+    add_smt.BindString(1, cookie.Domain());
+    add_smt.BindString(2, cookie.Name());
+    add_smt.BindString(3, cookie.Value());
+    add_smt.BindBlob(4, "", 0);  // encrypted_value
+    add_smt.BindString(5, cookie.Path());
+    add_smt.BindInt64(
+        6, cookie.ExpiryDate().ToDeltaSinceWindowsEpoch().InMicroseconds());
+    add_smt.BindInt(7, cookie.IsSecure());
+    add_smt.BindInt(8, cookie.IsHttpOnly());
+    // Note that this, Priority(), and SourceScheme() below nominally rely on
+    // the enums in sqlite_persistent_cookie_store.cc having the same values as
+    // the ones in ../../cookies/cookie_constants.h.  But nothing in this test
+    // relies on that equivalence, so it's not worth the hassle to guarantee
+    // that.
+    add_smt.BindInt(9, static_cast<int>(cookie.SameSite()));
+    add_smt.BindInt64(
+        10,
+        cookie.LastAccessDate().ToDeltaSinceWindowsEpoch().InMicroseconds());
+    add_smt.BindInt(11, cookie.IsPersistent());
+    add_smt.BindInt(12, cookie.IsPersistent());
+    add_smt.BindInt(13, static_cast<int>(cookie.Priority()));
+    add_smt.BindInt(14, static_cast<int>(cookie.SourceScheme()));
+    if (!add_smt.Run())
+      return false;
+  }
+  if (!transaction.Commit())
+    return false;
+
+  return true;
+}
+
 // Confirm the cookie list passed in has the above cookies in it.
-void ConfirmV11CookiesFromDB(
+void ConfirmCookiesAfterMigrationTest(
     std::vector<std::unique_ptr<CanonicalCookie>> read_in_cookies) {
   std::sort(read_in_cookies.begin(), read_in_cookies.end(), &CompareCookies);
   int i = 0;
@@ -1809,7 +1885,20 @@ TEST_F(SQLitePersistentCookieStoreTest, UpgradeToSchemaVersion12) {
 
   std::vector<std::unique_ptr<CanonicalCookie>> read_in_cookies;
   CreateAndLoad(false, false, &read_in_cookies);
-  ConfirmV11CookiesFromDB(std::move(read_in_cookies));
+  ConfirmCookiesAfterMigrationTest(std::move(read_in_cookies));
+}
+
+TEST_F(SQLitePersistentCookieStoreTest, UpgradeToSchemaVersion13) {
+  // Open db
+  sql::Database connection;
+  ASSERT_TRUE(connection.Open(temp_dir_.GetPath().Append(kCookieFilename)));
+  ASSERT_TRUE(CreateV12Schema(&connection));
+  ASSERT_TRUE(AddV12CookiesToDB(&connection));
+  connection.Close();
+
+  std::vector<std::unique_ptr<CanonicalCookie>> read_in_cookies;
+  CreateAndLoad(false, false, &read_in_cookies);
+  ConfirmCookiesAfterMigrationTest(std::move(read_in_cookies));
 }
 
 }  // namespace net

tools/metrics/histograms/histograms_xml/cookie/histograms.xml

@@ -370,6 +370,28 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="Cookie.TimeDatabaseMigrationToV13Failure" units="ms"
+    expires_after="2021-10-30">
+  <owner>chlily@chromium.org</owner>
+  <owner>morlovich@chromium.org</owner>
+  <summary>
+    The amount of time (ms) to migrate a v12 cookie database to v13, in the case
+    when migration fails. Migration occurs upon first startup of a browser
+    version with v13 database code.
+  </summary>
+</histogram>
+
+<histogram name="Cookie.TimeDatabaseMigrationToV13Success" units="ms"
+    expires_after="2021-10-30">
+  <owner>chlily@chromium.org</owner>
+  <owner>morlovich@chromium.org</owner>
+  <summary>
+    The amount of time (ms) to migrate a v12 cookie database to v13, in the case
+    when migration succeeds. Migration occurs upon first startup of a browser
+    version with v13 database code.
+  </summary>
+</histogram>
+
 <histogram name="Cookie.TimeDatabaseMigrationToV9" units="ms"
     expires_after="2018-08-30">
   <owner>erikchen@chromium.org</owner>