[Offline Pages] Fix crash due to table inconsistency.

components/offline_pages/offline_page_metadata_store_impl_unittest.cc

Index: components/offline_pages/offline_page_metadata_store_impl_unittest.cc
diff --git a/components/offline_pages/offline_page_metadata_store_impl_unittest.cc b/components/offline_pages/offline_page_metadata_store_impl_unittest.cc
index 2ed53a23477395f3b7bbe4c459249fde93e00c2d..b26eeda7ac8d0f93131dd4fba8fdd6be3cb2715d 100644
--- a/components/offline_pages/offline_page_metadata_store_impl_unittest.cc
+++ b/components/offline_pages/offline_page_metadata_store_impl_unittest.cc
@@ -18,12 +18,15 @@
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_page_metadata_store_sql.h"
 #include "components/offline_pages/offline_page_model.h"
+#include "sql/connection.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 
 namespace {
 
+#define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
+
 const char kTestClientNamespace[] = "CLIENT_NAMESPACE";
 const char kTestURL[] = "https://example.com";
 const ClientId kTestClientId1(kTestClientNamespace, "1234");
@@ -32,9 +35,40 @@ const base::FilePath::CharType kFilePath[] =
     FILE_PATH_LITERAL("/offline_pages/example_com.mhtml");
 int64_t kFileSize = 234567;
 
+// Build a store with outdated schema to simulate the upgrading process.
+// TODO(romax): move it to sql_unittests.

[fgorski, 2016/06/03 18:17:16]

good point.

Another test that we should include in upgrades is working with existing data,
which should also find its way to extracted tests.

[romax, 2016/06/03 19:36:22]

i've combined this case with the newly added test.

+void BuildBadTable(const base::FilePath& file) {

[fgorski, 2016/06/03 18:17:16]

How about BuildStoreV1 (and then look at the comment below)

[romax, 2016/06/03 19:36:22]

Done.

+  sql::Connection connection;
+  ASSERT_TRUE(
+      connection.Open(file.Append(FILE_PATH_LITERAL("OfflinePages.db"))));
+  ASSERT_TRUE(connection.is_open());
+  ASSERT_TRUE(connection.BeginTransaction());
+  ASSERT_TRUE(connection.Execute("CREATE TABLE " OFFLINE_PAGES_TABLE_NAME
+                                 "(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,"
+                                 " user_initiated INTEGER,"
+                                 " 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"
+                                 ")"));
+  ASSERT_TRUE(connection.CommitTransaction());
+  ASSERT_TRUE(connection.DoesTableExist(OFFLINE_PAGES_TABLE_NAME));
+  ASSERT_FALSE(
+      connection.DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time"));
+}
+
 class OfflinePageMetadataStoreFactory {
  public:
   virtual OfflinePageMetadataStore* BuildStore(const base::FilePath& file) = 0;
+  virtual OfflinePageMetadataStore* BuildBadStore(

[fgorski, 2016/06/03 18:17:16]

Can we call it: BuildV1Store() or something indicating that it was valid at some
point but is no longer?

Actually it seems that you are doing the upgrade in this method. How about:
BuildStoreWithUpgrade()

[romax, 2016/06/03 19:36:22]

Done.

+      const base::FilePath& file) = 0;
 };
 
 class OfflinePageMetadataStoreSQLFactory
@@ -45,6 +79,13 @@ class OfflinePageMetadataStoreSQLFactory
         base::ThreadTaskRunnerHandle::Get(), file);
     return store;
   }
+
+  OfflinePageMetadataStore* BuildBadStore(const base::FilePath& file) override {
+    BuildBadTable(file);
+    OfflinePageMetadataStoreSQL* store = new OfflinePageMetadataStoreSQL(
+        base::ThreadTaskRunnerHandle::Get(), file);
+    return store;
+  }
 };
 
 enum CalledCallback { NONE, LOAD, ADD, REMOVE, DESTROY };
@@ -119,6 +160,7 @@ template <typename T>
 class OfflinePageMetadataStoreTest : public OfflinePageMetadataStoreTestBase {
  public:
   std::unique_ptr<OfflinePageMetadataStore> BuildStore();
+  std::unique_ptr<OfflinePageMetadataStore> BuildBadStore();
 
  protected:
   T factory_;
@@ -135,6 +177,17 @@ OfflinePageMetadataStoreTest<T>::BuildStore() {
   return store;
 }
 
+template <typename T>
+std::unique_ptr<OfflinePageMetadataStore>
+OfflinePageMetadataStoreTest<T>::BuildBadStore() {
+  std::unique_ptr<OfflinePageMetadataStore> store(
+      factory_.BuildBadStore(temp_directory_.path()));
+  store->Load(base::Bind(&OfflinePageMetadataStoreTestBase::LoadCallback,
+                         base::Unretained(this)));
+  PumpLoop();
+  return store;
+}
+
 typedef testing::Types<OfflinePageMetadataStoreSQLFactory> MyTypes;
 TYPED_TEST_CASE(OfflinePageMetadataStoreTest, MyTypes);
 
@@ -147,6 +200,26 @@ TYPED_TEST(OfflinePageMetadataStoreTest, LoadEmptyStore) {
   EXPECT_EQ(0U, this->offline_pages_.size());
 }
 
+// Loads a store which has an outdated schema.
+// This test case would crash if it's not handling correctly when we're loading
+// old version stores.
+// TODO(romax): Move this to sql_unittest.
+TYPED_TEST(OfflinePageMetadataStoreTest, LoadExistingBadStore) {
+  std::unique_ptr<OfflinePageMetadataStore> store(this->BuildBadStore());

[fgorski, 2016/06/03 18:17:16]

can you omit this->?

[romax, 2016/06/03 19:36:22]

no, since TYPED_TEST is a derived template class so accessing member should use
this-> explicitly.

[fgorski, 2016/06/03 20:04:17]

Acknowledged. Thanks for checking.

+  EXPECT_EQ(LOAD, this->last_called_callback_);
+  EXPECT_EQ(STATUS_TRUE, this->last_status_);
+  EXPECT_EQ(0U, this->offline_pages_.size());
+  OfflinePageItem offline_page(GURL(kTestURL), 1234LL, kTestClientId1,
+                               base::FilePath(kFilePath), kFileSize);
+  store->AddOrUpdateOfflinePage(
+      offline_page,
+      base::Bind(&OfflinePageMetadataStoreTestBase::UpdateCallback,
+                 base::Unretained(this), ADD));
+  this->PumpLoop();
+  EXPECT_EQ(ADD, this->last_called_callback_);
+  EXPECT_EQ(STATUS_TRUE, this->last_status_);
+}
+
 // Adds metadata of an offline page into a store and then opens the store
 // again to make sure that stored metadata survives store restarts.
 TYPED_TEST(OfflinePageMetadataStoreTest, AddOfflinePage) {