[Offline Pages] Fix crash due to table inconsistency.

components/offline_pages/offline_page_metadata_store_sql.cc

 // Copyright 2016 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.
 
 #include "components/offline_pages/offline_page_metadata_store_sql.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "sql/connection.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 
 namespace offline_pages {
 
 namespace {
 
 // This is a macro instead of a const so that
 // it can be used inline in other SQL statements below.
 #define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
 
 const char kOfflinePagesColumns[] =

[jianli, 2016/06/03 21:12:18]

Could you please comment that new column should be added to the end of the list
below in order to avoid complicated table upgrade?

[romax, 2016/06/03 23:11:39]

Done.

     "(offline_id INTEGER PRIMARY KEY NOT NULL,"
     " creation_time INTEGER NOT NULL,"
     " file_size INTEGER NOT NULL,"
     " version INTEGER NOT NULL,"
     " last_access_time INTEGER NOT NULL,"
     " access_count INTEGER NOT NULL,"
     " status INTEGER NOT NULL DEFAULT 0,"
     // A note on this field:  It will be NULL for now and is reserved for
     // later use.  We will treat NULL as "Unknown" in any subsequent queries
     // for user_initiated values.
     " user_initiated INTEGER,"  // this is actually a boolean
-    " expiration_time INTEGER NOT NULL,"
+    " expiration_time INTEGER NOT NULL DEFAULT 0,"
     " client_namespace VARCHAR NOT NULL,"
     " client_id VARCHAR NOT NULL,"
     " online_url VARCHAR NOT NULL,"
     " offline_url VARCHAR NOT NULL DEFAULT '',"
     " file_path VARCHAR NOT NULL"
     ")";
 
 // This is cloned from //content/browser/appcache/appcache_database.cc
 struct TableInfo {
   const char* table_name;
@@ skipping 22 matching lines @@
   OP_FILE_PATH,
 };
 
 bool CreateTable(sql::Connection* db, const TableInfo& table_info) {
   std::string sql("CREATE TABLE ");
   sql += table_info.table_name;
   sql += table_info.columns;
   return db->Execute(sql.c_str());
 }
 
+bool RefreshColumns(sql::Connection* db) {
+  if (!db->Execute("ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
+                   " RENAME TO temp_" OFFLINE_PAGES_TABLE_NAME)) {
+    return false;
+  }
+  if (!CreateTable(db, kOfflinePagesTable))
+    return false;
+  if (!db->Execute(
+          "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
+          " (offline_id, creation_time, file_size, version, last_access_time, "
+          "access_count, status, user_initiated, client_namespace, client_id, "
+          "online_url, offline_url, file_path) "
+          "SELECT offline_id, creation_time, file_size, version, "
+          "last_access_time, "
+          "access_count, status, user_initiated, client_namespace, client_id, "
+          "online_url, offline_url, file_path "
+          "FROM temp_" OFFLINE_PAGES_TABLE_NAME)) {
+    return false;
+  }
+  if (!db->Execute("DROP TABLE IF EXISTS temp_" OFFLINE_PAGES_TABLE_NAME))
+    return false;
+
+  return true;
+}
+
 bool CreateSchema(sql::Connection* db) {
   // If you create a transaction but don't Commit() it is automatically
   // rolled back by its destructor when it falls out of scope.
   sql::Transaction transaction(db);
   if (!transaction.Begin())
     return false;
-  if (db->DoesTableExist(kOfflinePagesTable.table_name))
+  if (db->DoesTableExist(kOfflinePagesTable.table_name)) {

[jianli, 2016/06/03 21:12:18]

I suggest to reorder all these if checks to make the code easier to understand
and get rid of 2nd call of Commit.
  if (!db->DoesTableExist(kOfflinePagesTable.table_name)) {
    if (!CreateTable(db, kOfflinePagesTable)
      return false;
  }

  if (db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time"))
    return true;

  if (!RefreshColumns(db))
    return false;

  return transaction.Commit();
}

[romax, 2016/06/03 23:11:39]

emm I dont think we can return true since it may come from the create table
above and it would rollback without committing the transaction. But i've changed
the order.
Thanks for the catch!

+    if (!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time")) {
+      if (!RefreshColumns(db))
+        return false;
+      return transaction.Commit();
+    }
     return true;
+  }
 
   if (!CreateTable(db, kOfflinePagesTable))
     return false;
 
   // TODO(bburns): Add indices here.
   return transaction.Commit();
 }
 
 bool DeleteByOfflineId(sql::Connection* db, int64_t offline_id) {
   static const char kSql[] =
@@ skipping 227 matching lines @@
 }
 
 void OfflinePageMetadataStoreSQL::Reset(const ResetCallback& callback) {
   background_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&OfflinePageMetadataStoreSQL::ResetSync, base::Passed(&db_),
                  base::ThreadTaskRunnerHandle::Get(), callback));
 }
 
 }  // namespace offline_pages