Persist download interrupt reason, both target and current paths, and url_chain.

chrome/browser/history/history_unittest.cc

 // Copyright (c) 2012 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.
 
 // History unit tests come in two flavors:
 //
 // 1. The more complicated style is that the unit test creates a full history
 //    service. This spawns a background thread for the history backend, and
 //    all communication is asynchronous. This is useful for testing more
 //    complicated things or end-to-end behavior.
@@ skipping 19 matching lines @@
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
+#include "base/stringprintf.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/history/download_row.h"
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/history/history_database.h"
 #include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/history_service.h"
+#include "chrome/browser/history/history_unittest_base.h"
 #include "chrome/browser/history/in_memory_database.h"
 #include "chrome/browser/history/in_memory_history_backend.h"
 #include "chrome/browser/history/page_usage_data.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/thumbnail_score.h"
 #include "chrome/tools/profiles/thumbnail-inl.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
@@ skipping 35 matching lines @@
   virtual void DBLoaded(int backend_id) OVERRIDE {}
   virtual void StartTopSitesMigration(int backend_id) OVERRIDE {}
   virtual void NotifyVisitDBObserversOnAddVisit(
       const BriefVisitInfo& info) OVERRIDE {}
  private:
   HistoryBackendDBTest* history_test_;
 };
 
 // This must be outside the anonymous namespace for the friend statement in
 // HistoryBackend to work.
-class HistoryBackendDBTest : public testing::Test {
+class HistoryBackendDBTest : public HistoryUnitTestBase {
  public:
   HistoryBackendDBTest() : db_(NULL) {
   }
 
   ~HistoryBackendDBTest() {
   }
 
  protected:
   friend class BackendDelegate;
 
   // Creates the HistoryBackend and HistoryDatabase on the current thread,
   // assigning the values to backend_ and db_.
   void CreateBackendAndDatabase() {
     backend_ = new HistoryBackend(history_dir_, 0, new BackendDelegate(this),
                                   NULL);
     backend_->Init(std::string(), false);
     db_ = backend_->db_.get();
     DCHECK(in_mem_backend_.get()) << "Mem backend should have been set by "
         "HistoryBackend::Init";
   }
 
+  void CreateDBVersion(int version) {

[sky, 2013/01/31 01:28:49]

Wrap all calls to this in ASSERT_NO_FATAL_FAILURE.

[Randy Smith (Not in Mondays), 2013/01/31 20:11:28]

Done, but could you clue me as to what the goal is?  I don't see any way that a
fatal failure could occur outside of the internal ASSERT_NO_FATAL_FAILURE.

+    FilePath data_path;
+    ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &data_path));
+    data_path = data_path.AppendASCII("History");
+    data_path = data_path.Append(StringPrintf("history.%d.sql", version));
+    ASSERT_NO_FATAL_FAILURE(
+        ExecuteSQLScript(data_path, history_dir_.Append(
+            chrome::kHistoryFilename)));
+  }
+
   // testing::Test
   virtual void SetUp() {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     history_dir_ = temp_dir_.path().AppendASCII("HistoryBackendDBTest");
     ASSERT_TRUE(file_util::CreateDirectory(history_dir_));
   }
 
   void DeleteBackend() {
     if (backend_) {
       backend_->Closing();
       backend_ = NULL;
     }
   }
 
   virtual void TearDown() {
     DeleteBackend();
 
     // Make sure we don't have any event pending that could disrupt the next
     // test.
     MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure());
     MessageLoop::current()->Run();
   }
 
   int64 AddDownload(DownloadItem::DownloadState state, const Time& time) {
+    std::vector<GURL> url_chain;
+    url_chain.push_back(GURL("foo-url"));
+
     DownloadRow download(
         FilePath(FILE_PATH_LITERAL("foo-path")),
-        GURL("foo-url"),
+        FilePath(FILE_PATH_LITERAL("foo-path")),
+        url_chain,
         GURL(""),
         time,
         time,
         0,
         512,
         state,
+        content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+        content::DOWNLOAD_INTERRUPT_REASON_NONE,
         0,
         0);
     return db_->CreateDownload(download);
   }
 
   base::ScopedTempDir temp_dir_;
 
   MessageLoopForUI message_loop_;
 
   // names of the database files
@@ skipping 39 matching lines @@
   DownloadID handle;
   EXPECT_NE(0, handle = AddDownload(DownloadItem::COMPLETE, Time()));
   db_->QueryDownloads(&downloads);
   EXPECT_EQ(1U, downloads.size());
   db_->RemoveDownload(handle);
   db_->QueryDownloads(&downloads);
   EXPECT_EQ(0U, downloads.size());
 }
 
 TEST_F(HistoryBackendDBTest, MigrateDownloadsState) {
-  // Create the db and close it so that we can reopen it directly.
-  CreateBackendAndDatabase();
-  DeleteBackend();
+  // Create the db we want.
+  CreateDBVersion(22);
   {
-    // Re-open the db for manual manipulation.
+    // Open the db for manual manipulation.
     sql::Connection db;
     ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
-    {
-      // Manually force the version to 22.
-      sql::Statement version22(db.GetUniqueStatement(
-            "UPDATE meta SET value=22 WHERE key='version'"));
-      ASSERT_TRUE(version22.Run());
-    }
+
     // Manually insert corrupted rows; there's infrastructure in place now to
     // make this impossible, at least according to the test above.
     for (int state = 0; state < 5; ++state) {
       sql::Statement s(db.GetUniqueStatement(
             "INSERT INTO downloads (id, full_path, url, start_time, "
             "received_bytes, total_bytes, state, end_time, opened) VALUES "
             "(?, ?, ?, ?, ?, ?, ?, ?, ?)"));
       s.BindInt64(0, 1 + state);
       s.BindString(1, "path");
       s.BindString(2, "url");
       s.BindInt64(3, base::Time::Now().ToTimeT());
       s.BindInt64(4, 100);
       s.BindInt64(5, 100);
       s.BindInt(6, state);
       s.BindInt64(7, base::Time::Now().ToTimeT());
       s.BindInt(8, state % 2);
       ASSERT_TRUE(s.Run());
     }
   }
 
   // Re-open the db using the HistoryDatabase, which should migrate from version
-  // 22 to 23, fixing just the row whose state was 3. Then close the db so that
-  // we can re-open it directly.
+  // 22 to the current version, fixing just the row whose state was 3.
+  // Then close the db so that we can re-open it directly.
   CreateBackendAndDatabase();
   DeleteBackend();
   {
     // Re-open the db for manual manipulation.
     sql::Connection db;
     ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
     {
       // The version should have been updated.
       int cur_version = HistoryDatabase::GetCurrentVersion();
       ASSERT_LT(22, cur_version);
@@ skipping 14 matching lines @@
         // 4.
         EXPECT_EQ(((counter == 3) ? 4 : counter), statement.ColumnInt(1));
         EXPECT_EQ(counter % 2, statement.ColumnInt(2));
         ++counter;
       }
       EXPECT_EQ(5, counter);
     }
   }
 }
 
+TEST_F(HistoryBackendDBTest, MigrateDownloadsReasonPathsAndDangerType) {
+  Time now(base::Time::Now());
+
+  // Create the db we want.  The schema didn't change from 22->23, so just
+  // re-use the v22 file.
+  CreateDBVersion(22);
+  {
+    // Re-open the db for manual manipulation.
+    sql::Connection db;
+    ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
+
+    // Manually insert some rows.
+    sql::Statement s(db.GetUniqueStatement(
+        "INSERT INTO downloads (id, full_path, url, start_time, "
+        "received_bytes, total_bytes, state, end_time, opened) VALUES "
+        "(?, ?, ?, ?, ?, ?, ?, ?, ?)"));
+
+    int64 db_handle = 0;

[sky, 2013/01/31 01:28:49]

THis code isn't particularly readable. Can you create a dump of this and load it
too?

[Randy Smith (Not in Mondays), 2013/01/31 20:11:28]

I can, and if it's important to you I will.  But I find the idiom fairly
readable (it's parallel to, e.g. DownloadDatabase::UpdateDownload()) and if I
turn it into a script I'm going to need to manage the relationship between a
time value specified inside the script and a different time constant outside the
script, which is fairly simple now, but wouldn't be then.  So I'm going to ask
you to tell me twice :-}.

(If I do do the script I'll want to execute a string straight from this file
rather than create a separate file, but I doubt you care about that.)

+    // Null path.
+    s.BindInt64(0, ++db_handle);
+    s.BindString(1, "");
+    s.BindString(2, "http://whatever.com/index.html");
+    s.BindInt64(3, now.ToTimeT());
+    s.BindInt64(4, 100);
+    s.BindInt64(5, 100);
+    s.BindInt(6, 1);
+    s.BindInt64(7, now.ToTimeT());
+    s.BindInt(8, 1);
+    ASSERT_TRUE(s.Run());
+    s.Reset(true);
+
+    // Non-null path.
+    s.BindInt64(0, ++db_handle);
+    s.BindString(1, "/path/to/some/file");
+    s.BindString(2, "http://whatever.com/index1.html");
+    s.BindInt64(3, now.ToTimeT());
+    s.BindInt64(4, 100);
+    s.BindInt64(5, 100);
+    s.BindInt(6, 1);
+    s.BindInt64(7, now.ToTimeT());
+    s.BindInt(8, 1);
+    ASSERT_TRUE(s.Run());
+  }
+
+  // Re-open the db using the HistoryDatabase, which should migrate from version
+  // 23 to 24, creating the new tables and creating the new path, reason,
+  // and danger columns.
+  CreateBackendAndDatabase();
+  DeleteBackend();
+  {
+    // Re-open the db for manual manipulation.
+    sql::Connection db;
+    ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
+    {
+      // The version should have been updated.
+      int cur_version = HistoryDatabase::GetCurrentVersion();
+      ASSERT_LT(23, cur_version);
+      sql::Statement s(db.GetUniqueStatement(
+          "SELECT value FROM meta WHERE key = 'version'"));
+      EXPECT_TRUE(s.Step());
+      EXPECT_EQ(cur_version, s.ColumnInt(0));
+    }
+    {
+      base::Time nowish(base::Time::FromTimeT(now.ToTimeT()));
+
+      // Confirm downloads table is valid.
+      sql::Statement statement(db.GetUniqueStatement(
+          "SELECT id, interrupt_reason, current_path, target_path, "
+          "       danger_type, start_time, end_time "
+          "FROM downloads ORDER BY id"));
+      EXPECT_TRUE(statement.Step());
+      EXPECT_EQ(1, statement.ColumnInt64(0));
+      EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
+                statement.ColumnInt(1));
+      EXPECT_EQ("", statement.ColumnString(2));
+      EXPECT_EQ("", statement.ColumnString(3));
+      // Implicit dependence on value of kDangerTypeNotDangerous from
+      // download_database.cc.
+      EXPECT_EQ(0, statement.ColumnInt(4));
+      EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(5));
+      EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(6));
+
+      EXPECT_TRUE(statement.Step());
+      EXPECT_EQ(2, statement.ColumnInt64(0));
+      EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
+                statement.ColumnInt(1));
+      EXPECT_EQ("/path/to/some/file", statement.ColumnString(2));
+      EXPECT_EQ("/path/to/some/file", statement.ColumnString(3));
+      EXPECT_EQ(0, statement.ColumnInt(4));
+      EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(5));
+      EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(6));
+
+      EXPECT_FALSE(statement.Step());
+    }
+    {
+      // Confirm downloads_url_chains table is valid.
+      sql::Statement statement(db.GetUniqueStatement(
+          "SELECT id, chain_index, url FROM downloads_url_chains "
+          " ORDER BY id, chain_index"));
+      EXPECT_TRUE(statement.Step());
+      EXPECT_EQ(1, statement.ColumnInt64(0));
+      EXPECT_EQ(0, statement.ColumnInt(1));
+      EXPECT_EQ("http://whatever.com/index.html", statement.ColumnString(2));
+
+      EXPECT_TRUE(statement.Step());
+      EXPECT_EQ(2, statement.ColumnInt64(0));
+      EXPECT_EQ(0, statement.ColumnInt(1));
+      EXPECT_EQ("http://whatever.com/index1.html", statement.ColumnString(2));
+
+      EXPECT_FALSE(statement.Step());
+    }
+  }
+}
+
+TEST_F(HistoryBackendDBTest, ConfirmDownloadRowCreateAndDelete) {
+  // Create the DB.
+  CreateBackendAndDatabase();
+
+  base::Time now(base::Time::Now());
+
+  // Add some downloads.
+  AddDownload(DownloadItem::COMPLETE, now);
+  int64 did2 = AddDownload(DownloadItem::COMPLETE, now +
+                           base::TimeDelta::FromDays(2));
+  int64 did3 = AddDownload(DownloadItem::COMPLETE, now -
+                           base::TimeDelta::FromDays(2));
+
+  // Confirm that resulted in the correct number of rows in the DB.
+  DeleteBackend();
+  {
+    sql::Connection db;
+    ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
+    sql::Statement statement(db.GetUniqueStatement(
+        "Select Count(*) from downloads"));
+    EXPECT_TRUE(statement.Step());
+    EXPECT_EQ(3, statement.ColumnInt(0));
+
+    sql::Statement statement1(db.GetUniqueStatement(
+        "Select Count(*) from downloads_url_chains"));
+    EXPECT_TRUE(statement1.Step());
+    EXPECT_EQ(3, statement1.ColumnInt(0));
+  }
+
+  // Delete some rows and make sure the results are still correct.
+  CreateBackendAndDatabase();
+  db_->RemoveDownload(did2);
+  db_->RemoveDownload(did3);
+  DeleteBackend();
+  {
+    sql::Connection db;
+    ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
+    sql::Statement statement(db.GetUniqueStatement(
+        "Select Count(*) from downloads"));
+    EXPECT_TRUE(statement.Step());
+    EXPECT_EQ(1, statement.ColumnInt(0));
+
+    sql::Statement statement1(db.GetUniqueStatement(
+        "Select Count(*) from downloads_url_chains"));
+    EXPECT_TRUE(statement1.Step());
+    EXPECT_EQ(1, statement1.ColumnInt(0));
+  }
+}
+
+struct InterruptReasonAssociation {
+  std::string name;
+  int value;
+};
+
+// Test is dependent on interrupt reasons being listed in header file
+// in order.
+const InterruptReasonAssociation current_reasons[] = {
+#define INTERRUPT_REASON(a, b) { #a, b },
+#include "content/public/browser/download_interrupt_reason_values.h"
+#undef INTERRUPT_REASON
+};
+
+// This represents a list of all reasons we've previously used;
+// Do Not Remove Any Entries From This List.
+const InterruptReasonAssociation historical_reasons[] = {
+  {"FILE_FAILED",  1},
+  {"FILE_ACCESS_DENIED",  2},
+  {"FILE_NO_SPACE",  3},
+  {"FILE_NAME_TOO_LONG",  5},
+  {"FILE_TOO_LARGE",  6},
+  {"FILE_VIRUS_INFECTED",  7},
+  {"FILE_TRANSIENT_ERROR",  10},
+  {"FILE_BLOCKED",  11},
+  {"FILE_SECURITY_CHECK_FAILED",  12},
+  {"FILE_TOO_SHORT", 13},
+  {"NETWORK_FAILED",  20},
+  {"NETWORK_TIMEOUT",  21},
+  {"NETWORK_DISCONNECTED",  22},
+  {"NETWORK_SERVER_DOWN",  23},
+  {"SERVER_FAILED",  30},
+  {"SERVER_NO_RANGE",  31},
+  {"SERVER_PRECONDITION",  32},
+  {"SERVER_BAD_CONTENT",  33},
+  {"USER_CANCELED",  40},
+  {"USER_SHUTDOWN",  41},
+  {"CRASH",  50},
+};
+
+// Make sure no one has changed a DownloadInterruptReason we've previously
+// persisted.
+TEST_F(HistoryBackendDBTest,
+       ConfirmDownloadInterruptReasonBackwardsCompatible) {
+  // Are there any cases in which a historical number has been repurposed
+  // for an error other than it's original?
+  for (size_t i = 0; i < arraysize(current_reasons); i++) {
+    const InterruptReasonAssociation& cur_reason(current_reasons[i]);
+    bool found = false;
+
+    for (size_t j = 0; j < arraysize(historical_reasons); ++j) {
+      const InterruptReasonAssociation& hist_reason(historical_reasons[j]);
+
+      if (hist_reason.value == cur_reason.value) {
+        EXPECT_EQ(cur_reason.name, hist_reason.name)
+            << "Same integer value used for old error \""
+            << hist_reason.name
+            << "\" as for new error \""
+            << cur_reason.name
+            << "\"." << std::endl
+            << "**This will cause database conflicts with persisted values**"
+            << std::endl
+            << "Please assign a new, non-conflicting value for the new error.";
+      }
+
+      if (hist_reason.name == cur_reason.name) {
+        EXPECT_EQ(cur_reason.value, hist_reason.value)
+            << "Same name (\"" << hist_reason.name
+            << "\") maps to a different value historically ("
+            << hist_reason.value << ") and currently ("
+            << cur_reason.value << ")" << std::endl
+            << "This may cause database conflicts with persisted values"
+            << std::endl
+            << "If this error is the same as the old one, you should"
+            << std::endl
+            << "use the old value, and if it is different, you should"
+            << std::endl
+            << "use a new name.";
+
+        found = true;
+      }
+    }
+
+    EXPECT_TRUE(found)
+        << "Error \"" << cur_reason.name << "\" not found in historical list."
+        << std::endl
+        << "Please add it.";
+  }
+}
+
 // The tracker uses RenderProcessHost pointers for scoping but never
 // dereferences them. We use ints because it's easier. This function converts
 // between the two.
 static void* MakeFakeHost(int id) {
   void* host = 0;
   memcpy(&host, &id, sizeof(id));
   return host;
 }
 
 class HistoryTest : public testing::Test {
@@ skipping 879 matching lines @@
 
   history_service_->StopSyncing(syncer::HISTORY_DELETE_DIRECTIVES);
   EXPECT_TRUE(
       history_service_->ProcessLocalDeleteDirective(delete_directive).IsSet());
   EXPECT_EQ(1u, change_processor.GetChanges().size());
 }
 
 }  // namespace
 
 }  // namespace history