Expire appcaches that fail to update for "too long".

content/browser/appcache/appcache_database.cc

Index: content/browser/appcache/appcache_database.cc
diff --git a/content/browser/appcache/appcache_database.cc b/content/browser/appcache/appcache_database.cc
index 985173be166b49be5017b5ad8e6478488b5307bf..49c350a00523f5bc04f2fb443dbffa995ddbd980 100644
--- a/content/browser/appcache/appcache_database.cc
+++ b/content/browser/appcache/appcache_database.cc
@@ -23,13 +23,8 @@ namespace content {
 // Schema -------------------------------------------------------------------
 namespace {
 
-#if defined(APPCACHE_USE_SIMPLE_CACHE)
-const int kCurrentVersion = 6;
-const int kCompatibleVersion = 6;
-#else
-const int kCurrentVersion = 5;
-const int kCompatibleVersion = 5;
-#endif
+const int kCurrentVersion = 7;
+const int kCompatibleVersion = 7;
 
 // A mechanism to run experiments that may affect in data being persisted
 // in different ways such that when the experiment is toggled on/off via
@@ -63,7 +58,9 @@ const TableInfo kTables[] = {
     " origin TEXT,"
     " manifest_url TEXT,"
     " creation_time INTEGER,"
-    " last_access_time INTEGER)" },
+    " last_access_time INTEGER,"
+    " last_full_update_check_time INTEGER,"
+    " first_evictable_error_time INTEGER)" },
 
   { kCachesTable,
     "(cache_id INTEGER PRIMARY KEY,"
@@ -313,7 +310,9 @@ bool AppCacheDatabase::FindGroup(int64 group_id, GroupRecord* record) {
 
   const char* kSql =
       "SELECT group_id, origin, manifest_url,"
-      "       creation_time, last_access_time"
+      "       creation_time, last_access_time,"
+      "       last_full_update_check_time,"
+      "       first_evictable_error_time"
       "  FROM Groups WHERE group_id = ?";
 
   sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
@@ -335,7 +334,9 @@ bool AppCacheDatabase::FindGroupForManifestUrl(
 
   const char* kSql =
       "SELECT group_id, origin, manifest_url,"
-      "       creation_time, last_access_time"
+      "       creation_time, last_access_time,"
+      "       last_full_update_check_time,"
+      "       first_evictable_error_time"
       "  FROM Groups WHERE manifest_url = ?";
 
   sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
@@ -357,7 +358,9 @@ bool AppCacheDatabase::FindGroupsForOrigin(
 
   const char* kSql =
       "SELECT group_id, origin, manifest_url,"
-      "       creation_time, last_access_time"
+      "       creation_time, last_access_time,"
+      "       last_full_update_check_time,"
+      "       first_evictable_error_time"
       "   FROM Groups WHERE origin = ?";
 
   sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
@@ -379,7 +382,9 @@ bool AppCacheDatabase::FindGroupForCache(int64 cache_id, GroupRecord* record) {
 
   const char* kSql =
       "SELECT g.group_id, g.origin, g.manifest_url,"
-      "       g.creation_time, g.last_access_time"
+      "       g.creation_time, g.last_access_time,"
+      "       g.last_full_update_check_time,"
+      "       g.first_evictable_error_time"
       "  FROM Groups g, Caches c"
       "  WHERE c.cache_id = ? AND c.group_id = g.group_id";
 
@@ -395,17 +400,9 @@ bool AppCacheDatabase::FindGroupForCache(int64 cache_id, GroupRecord* record) {
 
 bool AppCacheDatabase::UpdateLastAccessTime(
     int64 group_id, base::Time time) {
-  if (!LazyOpen(true))
+  if (!LazyUpdateLastAccessTime(group_id, time))
     return false;
-
-  const char* kSql =
-      "UPDATE Groups SET last_access_time = ? WHERE group_id = ?";
-
-  sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
-  statement.BindInt64(0, time.ToInternalValue());
-  statement.BindInt64(1, group_id);
-
-  return statement.Run();
+  return CommitLazyLastAccessTimes();
 }
 
 bool AppCacheDatabase::LazyUpdateLastAccessTime(
@@ -425,20 +422,46 @@ bool AppCacheDatabase::CommitLazyLastAccessTimes() {
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin())
     return false;
-  for (const auto& pair : lazy_last_access_times_)
-    UpdateLastAccessTime(pair.first, pair.second);
+  for (const auto& pair : lazy_last_access_times_) {
+    const char* kSql =
+        "UPDATE Groups SET last_access_time = ? WHERE group_id = ?";
+    sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));

[jsbell, 2015/05/27 22:38:18]

Does this Statement need to be explicitly Run()?

(I'm a bit ignorant of the API here)

[michaeln, 2015/05/27 23:21:51]

Ooops, sure does need a call to Run.

+    statement.BindInt64(0, pair.second.ToInternalValue());  // time
+    statement.BindInt64(1, pair.first);  // group_id
+  }
   lazy_last_access_times_.clear();
   return transaction.Commit();
 }
 
+bool AppCacheDatabase::UpdateEvictionTimes(
+    int64 group_id,
+    base::Time last_full_update_check_time,
+    base::Time first_evictable_error_time) {
+  if (!LazyOpen(true))
+    return false;
+
+  const char* kSql =
+      "UPDATE Groups"
+      " SET last_full_update_check_time = ?, first_evictable_error_time = ?"
+      " WHERE group_id = ?";
+
+  sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, last_full_update_check_time.ToInternalValue());
+  statement.BindInt64(1, first_evictable_error_time.ToInternalValue());
+  statement.BindInt64(3, group_id);
+
+  return statement.Run();
+}
+
 bool AppCacheDatabase::InsertGroup(const GroupRecord* record) {
   if (!LazyOpen(true))
     return false;
 
   const char* kSql =
       "INSERT INTO Groups"
-      "  (group_id, origin, manifest_url, creation_time, last_access_time)"
-      "  VALUES(?, ?, ?, ?, ?)";
+      "  (group_id, origin, manifest_url, creation_time, last_access_time,"
+      "   last_full_update_check_time, first_evictable_error_time)"
+      "  VALUES(?, ?, ?, ?, ?, ?, ?)";
 
   sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, record->group_id);
@@ -446,6 +469,8 @@ bool AppCacheDatabase::InsertGroup(const GroupRecord* record) {
   statement.BindString(2, record->manifest_url.spec());
   statement.BindInt64(3, record->creation_time.ToInternalValue());
   statement.BindInt64(4, record->last_access_time.ToInternalValue());
+  statement.BindInt64(5, record->last_full_update_check_time.ToInternalValue());
+  statement.BindInt64(6, record->first_evictable_error_time.ToInternalValue());
 
   return statement.Run();
 }
@@ -956,6 +981,11 @@ void AppCacheDatabase::ReadGroupRecord(
     record->last_access_time =
         base::Time::FromInternalValue(statement.ColumnInt64(4));
   }
+
+  record->last_full_update_check_time =
+      base::Time::FromInternalValue(statement.ColumnInt64(5));
+  record->first_evictable_error_time =
+      base::Time::FromInternalValue(statement.ColumnInt64(6));
 }
 
 void AppCacheDatabase::ReadCacheRecord(
@@ -1134,8 +1164,9 @@ bool AppCacheDatabase::CreateSchema() {
 
 bool AppCacheDatabase::UpgradeSchema() {
 #if defined(APPCACHE_USE_SIMPLE_CACHE)
-  return DeleteExistingAndCreateNewDatabase();
-#else
+  if (meta_table_->GetVersionNumber() < 6)
+    return DeleteExistingAndCreateNewDatabase();
+#endif
   if (meta_table_->GetVersionNumber() == 3) {
     // version 3 was pre 12/17/2011
     DCHECK_EQ(strcmp(kNamespacesTable, kTables[3].table_name), 0);
@@ -1207,13 +1238,38 @@ bool AppCacheDatabase::UpgradeSchema() {
     }
     meta_table_->SetVersionNumber(5);
     meta_table_->SetCompatibleVersionNumber(5);
+    if (!transaction.Commit())
+      return false;
+  }
+
+#if defined(APPCACHE_USE_SIMPLE_CACHE)
+  // The schema version number was increased to 6 when we switched to the
+  // SimpleCache for Android, but the SQL part of the schema is identical
+  // to v5 on desktop chrome.
+  if (meta_table_->GetVersionNumber() == 6) {

[jsbell, 2015/05/27 22:38:18]

Could this just be < 7 in both SimpleCache and other cases?

(This looks fine/correct, just thinking of ways to simplify)

[michaeln, 2015/05/27 23:21:51]

That could work with an early return up top for < 2. Either way, I'd want to
leave the comment in about how the version upgrade mechanism was used (or
abused) on android for the version we're upgrading out of.

+#else
+  if (meta_table_->GetVersionNumber() == 5) {
+#endif
+    // version [5,6] was pre May 2015
+    // v7 Adds support for expiring caches that are failing to update.
+    sql::Transaction transaction(db_.get());
+    if (!transaction.Begin() ||
+        !db_->Execute(
+            "ALTER TABLE Groups ADD COLUMN"
+            " last_full_update_check_time INTEGER") ||
+        !db_->Execute(
+            "ALTER TABLE Groups ADD COLUMN"
+            " first_evictable_error_time INTEGER")) {
+      return false;
+    }
+    meta_table_->SetVersionNumber(7);
+    meta_table_->SetCompatibleVersionNumber(7);
     return transaction.Commit();
   }
 
   // If there is no upgrade path for the version on disk to the current
   // version, nuke everything and start over.
   return DeleteExistingAndCreateNewDatabase();
-#endif
 }
 
 void AppCacheDatabase::ResetConnectionAndTables() {