Clean up entries left by crashes in the DownloadDB.

BUG=224385
R=benjhayden@chromium.org



Review URL: https://chromiumcodereview.appspot.com/13044019

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@192879 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 209 insertions, 122 deletions

diff --git a/chrome/browser/history/download_database.cc b/chrome/browser/history/download_database.cc
index 003ba6f..4a88e3a 100644
--- a/chrome/browser/history/download_database.cc
+++ b/chrome/browser/history/download_database.cc
@@ -27,9 +27,6 @@ using content::DownloadItem;
 
 namespace history {
 
-// static
-const int64 DownloadDatabase::kUninitializedHandle = -1;
-
 namespace {
 
 // Reason for dropping a particular record.
@@ -62,140 +59,144 @@ static const char kUrlChainSchema[] =
     "url LONGVARCHAR NOT NULL, "     // URL.
     "PRIMARY KEY (id, chain_index) )";
 
-// These constants and next two functions are used to allow
+#if defined(OS_POSIX)
+
+// Binds/reads the given file path to the given column of the given statement.
+void BindFilePath(sql::Statement& statement, const base::FilePath& path,
+                  int col) {
+  statement.BindString(col, path.value());
+}
+base::FilePath ColumnFilePath(sql::Statement& statement, int col) {
+  return base::FilePath(statement.ColumnString(col));
+}
+
+#else
+
+// See above.
+void BindFilePath(sql::Statement& statement, const base::FilePath& path,
+                  int col) {
+  statement.BindString16(col, path.value());
+}
+base::FilePath ColumnFilePath(sql::Statement& statement, int col) {
+  return base::FilePath(statement.ColumnString16(col));
+}
+
+#endif
+
+// Key in the meta_table containing the next id to use for a new download in
+// this profile.
+static const char kNextDownloadId[] = "next_download_id";
+
+}  // namespace
+
+// static
+const int64 DownloadDatabase::kUninitializedHandle = -1;
+
+// These constants and the transformation functions below are used to allow
 // DownloadItem::DownloadState and DownloadDangerType to change without
 // breaking the database schema.
 // They guarantee that the values of the |state| field in the database are one
 // of the values returned by StateToInt, and that the values of the |state|
 // field of the DownloadRows returned by QueryDownloads() are one of the values
 // returned by IntToState().
-static const int kStateInvalid = -1;
-static const int kStateInProgress = 0;
-static const int kStateComplete = 1;
-static const int kStateCancelled = 2;
-static const int kStateBug140687 = 3;
-static const int kStateInterrupted = 4;
-
-static const int kDangerTypeInvalid = -1;
-static const int kDangerTypeNotDangerous = 0;
-static const int kDangerTypeDangerousFile = 1;
-static const int kDangerTypeDangerousUrl = 2;
-static const int kDangerTypeDangerousContent = 3;
-static const int kDangerTypeMaybeDangerousContent = 4;
-static const int kDangerTypeUncommonContent = 5;
-static const int kDangerTypeUserValidated = 6;
-static const int kDangerTypeDangerousHost = 7;
-
-int StateToInt(DownloadItem::DownloadState state) {
+const int DownloadDatabase::kStateInvalid = -1;
+const int DownloadDatabase::kStateInProgress = 0;
+const int DownloadDatabase::kStateComplete = 1;
+const int DownloadDatabase::kStateCancelled = 2;
+const int DownloadDatabase::kStateBug140687 = 3;
+const int DownloadDatabase::kStateInterrupted = 4;
+
+const int DownloadDatabase::kDangerTypeInvalid = -1;
+const int DownloadDatabase::kDangerTypeNotDangerous = 0;
+const int DownloadDatabase::kDangerTypeDangerousFile = 1;
+const int DownloadDatabase::kDangerTypeDangerousUrl = 2;
+const int DownloadDatabase::kDangerTypeDangerousContent = 3;
+const int DownloadDatabase::kDangerTypeMaybeDangerousContent = 4;
+const int DownloadDatabase::kDangerTypeUncommonContent = 5;
+const int DownloadDatabase::kDangerTypeUserValidated = 6;
+const int DownloadDatabase::kDangerTypeDangerousHost = 7;
+
+int DownloadDatabase::StateToInt(DownloadItem::DownloadState state) {
   switch (state) {
-    case DownloadItem::IN_PROGRESS: return kStateInProgress;
-    case DownloadItem::COMPLETE: return kStateComplete;
-    case DownloadItem::CANCELLED: return kStateCancelled;
-    case DownloadItem::INTERRUPTED: return kStateInterrupted;
+    case DownloadItem::IN_PROGRESS: return DownloadDatabase::kStateInProgress;
+    case DownloadItem::COMPLETE: return DownloadDatabase::kStateComplete;
+    case DownloadItem::CANCELLED: return DownloadDatabase::kStateCancelled;
+    case DownloadItem::INTERRUPTED: return DownloadDatabase::kStateInterrupted;
     case DownloadItem::MAX_DOWNLOAD_STATE:
       NOTREACHED();
-      return kStateInvalid;
+      return DownloadDatabase::kStateInvalid;
   }
   NOTREACHED();
-  return kStateInvalid;
+  return DownloadDatabase::kStateInvalid;
 }
 
-DownloadItem::DownloadState IntToState(int state) {
+DownloadItem::DownloadState DownloadDatabase::IntToState(int state) {
   switch (state) {
-    case kStateInProgress: return DownloadItem::IN_PROGRESS;
-    case kStateComplete: return DownloadItem::COMPLETE;
-    case kStateCancelled: return DownloadItem::CANCELLED;
+    case DownloadDatabase::kStateInProgress: return DownloadItem::IN_PROGRESS;
+    case DownloadDatabase::kStateComplete: return DownloadItem::COMPLETE;
+    case DownloadDatabase::kStateCancelled: return DownloadItem::CANCELLED;
     // We should not need kStateBug140687 here because MigrateDownloadsState()
     // is called in HistoryDatabase::Init().
-    case kStateInterrupted: return DownloadItem::INTERRUPTED;
+    case DownloadDatabase::kStateInterrupted: return DownloadItem::INTERRUPTED;
     default: return DownloadItem::MAX_DOWNLOAD_STATE;
   }
 }
 
-int DangerTypeToInt(content::DownloadDangerType danger_type) {
+int DownloadDatabase::DangerTypeToInt(content::DownloadDangerType danger_type) {
   switch (danger_type) {
     case content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS:
-      return kDangerTypeNotDangerous;
+      return DownloadDatabase::kDangerTypeNotDangerous;
     case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE:
-      return kDangerTypeDangerousFile;
+      return DownloadDatabase::kDangerTypeDangerousFile;
     case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL:
-      return kDangerTypeDangerousUrl;
+      return DownloadDatabase::kDangerTypeDangerousUrl;
     case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT:
-      return kDangerTypeDangerousContent;
+      return DownloadDatabase::kDangerTypeDangerousContent;
     case content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
-      return kDangerTypeMaybeDangerousContent;
+      return DownloadDatabase::kDangerTypeMaybeDangerousContent;
     case content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT:
-      return kDangerTypeUncommonContent;
+      return DownloadDatabase::kDangerTypeUncommonContent;
     case content::DOWNLOAD_DANGER_TYPE_USER_VALIDATED:
-      return kDangerTypeUserValidated;
+      return DownloadDatabase::kDangerTypeUserValidated;
     case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST:
-      return kDangerTypeDangerousHost;
+      return DownloadDatabase::kDangerTypeDangerousHost;
     case content::DOWNLOAD_DANGER_TYPE_MAX:
       NOTREACHED();
-      return kDangerTypeInvalid;
+      return DownloadDatabase::kDangerTypeInvalid;
   }
   NOTREACHED();
-  return kDangerTypeInvalid;
+  return DownloadDatabase::kDangerTypeInvalid;
 }
 
-content::DownloadDangerType IntToDangerType(int danger_type) {
+content::DownloadDangerType DownloadDatabase::IntToDangerType(int danger_type) {
   switch (danger_type) {
-    case kDangerTypeNotDangerous:
+    case DownloadDatabase::kDangerTypeNotDangerous:
       return content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS;
-    case kDangerTypeDangerousFile:
+    case DownloadDatabase::kDangerTypeDangerousFile:
       return content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE;
-    case kDangerTypeDangerousUrl:
+    case DownloadDatabase::kDangerTypeDangerousUrl:
       return content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL;
-    case kDangerTypeDangerousContent:
+    case DownloadDatabase::kDangerTypeDangerousContent:
       return content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT;
-    case kDangerTypeMaybeDangerousContent:
+    case DownloadDatabase::kDangerTypeMaybeDangerousContent:
       return content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT;
-    case kDangerTypeUncommonContent:
+    case DownloadDatabase::kDangerTypeUncommonContent:
       return content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT;
-    case kDangerTypeUserValidated:
+    case DownloadDatabase::kDangerTypeUserValidated:
       return content::DOWNLOAD_DANGER_TYPE_USER_VALIDATED;
-    case kDangerTypeDangerousHost:
+    case DownloadDatabase::kDangerTypeDangerousHost:
       return content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST;
     default:
       return content::DOWNLOAD_DANGER_TYPE_MAX;
   }
 }
 
-#if defined(OS_POSIX)
-
-// Binds/reads the given file path to the given column of the given statement.
-void BindFilePath(sql::Statement& statement, const base::FilePath& path,
-                  int col) {
-  statement.BindString(col, path.value());
-}
-base::FilePath ColumnFilePath(sql::Statement& statement, int col) {
-  return base::FilePath(statement.ColumnString(col));
-}
-
-#else
-
-// See above.
-void BindFilePath(sql::Statement& statement, const base::FilePath& path,
-                  int col) {
-  statement.BindString16(col, path.value());
-}
-base::FilePath ColumnFilePath(sql::Statement& statement, int col) {
-  return base::FilePath(statement.ColumnString16(col));
-}
-
-#endif
-
-// Key in the meta_table containing the next id to use for a new download in
-// this profile.
-static const char kNextDownloadId[] = "next_download_id";
-
-}  // namespace
-
 DownloadDatabase::DownloadDatabase()
     : owning_thread_set_(false),
       owning_thread_(0),
       next_id_(0),
-      next_db_handle_(0) {
+      next_db_handle_(0),
+      in_progress_entry_cleanup_completed_(false) {
 }
 
 DownloadDatabase::~DownloadDatabase() {
@@ -283,6 +284,8 @@ bool DownloadDatabase::DropDownloadTable() {
 
 void DownloadDatabase::QueryDownloads(
     std::vector<DownloadRow>* results) {
+  EnsureInProgressEntriesCleanedUp();
+
   results->clear();
   if (next_db_handle_ < 1)
     next_db_handle_ = 1;
@@ -401,6 +404,8 @@ void DownloadDatabase::QueryDownloads(
 }
 
 bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
+  EnsureInProgressEntriesCleanedUp();
+
   DCHECK(data.db_handle > 0);
   int state = StateToInt(data.state);
   if (state == kStateInvalid) {
@@ -433,16 +438,23 @@ bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
   return statement.Run();
 }
 
-bool DownloadDatabase::CleanUpInProgressEntries() {
+void DownloadDatabase::EnsureInProgressEntriesCleanedUp() {
+  if (in_progress_entry_cleanup_completed_)
+    return;
+
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
-      "UPDATE downloads SET state=? WHERE state=?"));
-  statement.BindInt(0, kStateCancelled);
-  statement.BindInt(1, kStateInProgress);
+      "UPDATE downloads SET state=?, interrupt_reason=? WHERE state=?"));
+  statement.BindInt(0, kStateInterrupted);
+  statement.BindInt(1, content::DOWNLOAD_INTERRUPT_REASON_CRASH);
+  statement.BindInt(2, kStateInProgress);
 
-  return statement.Run();
+  statement.Run();
+  in_progress_entry_cleanup_completed_ = true;
 }
 
 int64 DownloadDatabase::CreateDownload(const DownloadRow& info) {
+  EnsureInProgressEntriesCleanedUp();
+
   if (next_db_handle_ == 0) {
     // This is unlikely. All current known tests and users already call
     // QueryDownloads() before CreateDownload().
@@ -537,6 +549,8 @@ int64 DownloadDatabase::CreateDownload(const DownloadRow& info) {
 }
 
 void DownloadDatabase::RemoveDownload(int64 handle) {
+  EnsureInProgressEntriesCleanedUp();
+
   sql::Statement downloads_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM downloads WHERE id=?"));
   downloads_statement.BindInt64(0, handle);
@@ -559,6 +573,8 @@ void DownloadDatabase::RemoveDownloadURLs(int64 handle) {
 }
 
 int DownloadDatabase::CountDownloads() {
+  EnsureInProgressEntriesCleanedUp();
+
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT count(*) from downloads"));
   statement.Step();
diff --git a/chrome/browser/history/download_database.h b/chrome/browser/history/download_database.h
index 533af26..afb5fd0 100644
--- a/chrome/browser/history/download_database.h
+++ b/chrome/browser/history/download_database.h
@@ -8,7 +8,9 @@
 #include <string>
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/threading/platform_thread.h"
+#include "content/public/browser/download_item.h"
 #include "sql/meta_table.h"
 
 namespace sql {
@@ -41,12 +43,6 @@ class DownloadDatabase {
   // to select the row in the database table to update.
   bool UpdateDownload(const DownloadRow& data);
 
-  // Fixes state of the download entries. Sometimes entries with IN_PROGRESS
-  // state are not updated during browser shutdown (particularly when crashing).
-  // On the next start such entries are considered canceled. This functions
-  // fixes such entries.
-  bool CleanUpInProgressEntries();
-
   // Create a new database entry for one download and return its primary db id.
   int64 CreateDownload(const DownloadRow& info);
 
@@ -79,16 +75,59 @@ class DownloadDatabase {
   bool DropDownloadTable();
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(
+      HistoryBackendDBTest, ConfirmDownloadInProgressCleanup);
+
+  // Values used in the database for DownloadItem::State.
+  static const int kStateInvalid;
+  static const int kStateInProgress;
+  static const int kStateComplete;
+  static const int kStateCancelled;
+  static const int kStateBug140687;
+  static const int kStateInterrupted;
+
+  // Values used in the database for DownloadItem::DangerType
+  static const int kDangerTypeInvalid;
+  static const int kDangerTypeNotDangerous;
+  static const int kDangerTypeDangerousFile;
+  static const int kDangerTypeDangerousUrl;
+  static const int kDangerTypeDangerousContent;
+  static const int kDangerTypeMaybeDangerousContent;
+  static const int kDangerTypeUncommonContent;
+  static const int kDangerTypeUserValidated;
+  static const int kDangerTypeDangerousHost;
+
+  // Fixes state of the download entries. Sometimes entries with IN_PROGRESS
+  // state are not updated during browser shutdown (particularly when crashing).
+  // On the next start such entries are considered interrupted with
+  // interrupt reason |DOWNLOAD_INTERRUPT_REASON_CRASH|.  This function
+  // fixes such entries.
+  void EnsureInProgressEntriesCleanedUp();
+
   bool EnsureColumnExists(const std::string& name, const std::string& type);
 
   void RemoveDownloadURLs(int64 handle);
 
+  // Utility functions for conversion between DownloadItem types
+  // and DownloadDatabase constants.
+  static int StateToInt(content::DownloadItem::DownloadState state);
+  static content::DownloadItem::DownloadState IntToState(int state);
+  static int DangerTypeToInt(content::DownloadDangerType danger_type);
+  static content::DownloadDangerType IntToDangerType(int danger_type);
+
   bool owning_thread_set_;
   base::PlatformThreadId owning_thread_;
 
   int next_id_;
   int next_db_handle_;
 
+  // Initialized to false on construction, and checked in all functional
+  // routines post-migration in the database for a possible call to
+  // CleanUpInProgressEntries().  This allows us to avoid
+  // doing the cleanup until after any DB migration and unless we are
+  // actually use the downloads database.
+  bool in_progress_entry_cleanup_completed_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadDatabase);
 };
 
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 4ac89d1..327896b 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -1312,16 +1312,6 @@ void HistoryBackend::QueryDownloads(std::vector<DownloadRow>* rows) {
     db_->QueryDownloads(rows);
 }
 
-// Clean up entries that has been corrupted (because of the crash, for example).
-void HistoryBackend::CleanUpInProgressEntries() {
-  // If some "in progress" entries were not updated when Chrome exited, they
-  // need to be cleaned up.
-  if (!db_.get())
-    return;
-  db_->CleanUpInProgressEntries();
-  ScheduleCommit();
-}
-
 // Update a particular download entry.
 void HistoryBackend::UpdateDownload(const history::DownloadRow& data) {
   if (!db_.get())
diff --git a/chrome/browser/history/history_backend.h b/chrome/browser/history/history_backend.h
index facd6e2..e5a7530 100644
--- a/chrome/browser/history/history_backend.h
+++ b/chrome/browser/history/history_backend.h
@@ -308,7 +308,6 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
 
   void GetNextDownloadId(int* id);
   void QueryDownloads(std::vector<DownloadRow>* rows);
-  void CleanUpInProgressEntries();
   void UpdateDownload(const DownloadRow& data);
   void CreateDownload(const history::DownloadRow& history_info,
                       int64* db_handle);
diff --git a/chrome/browser/history/history_service.cc b/chrome/browser/history/history_service.cc
index eb49e9a..e38cbcf 100644
--- a/chrome/browser/history/history_service.cc
+++ b/chrome/browser/history/history_service.cc
@@ -804,14 +804,6 @@ void HistoryService::QueryDownloads(
       base::Bind(callback, base::Passed(&scoped_rows)));
 }
 
-// Changes all IN_PROGRESS in the database entries to CANCELED.
-// IN_PROGRESS entries are the corrupted entries, not updated by next function
-// because of the crash or some other extremal exit.
-void HistoryService::CleanUpInProgressEntries() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  ScheduleAndForget(PRIORITY_NORMAL, &HistoryBackend::CleanUpInProgressEntries);
-}
-
 // Handle updates for a particular download. This is a 'fire and forget'
 // operation, so we don't need to be called back.
 void HistoryService::UpdateDownload(const history::DownloadRow& data) {
diff --git a/chrome/browser/history/history_service.h b/chrome/browser/history/history_service.h
index d2c0102..b4f35e7 100644
--- a/chrome/browser/history/history_service.h
+++ b/chrome/browser/history/history_service.h
@@ -464,10 +464,6 @@ class HistoryService : public CancelableRequestProvider,
   // download. The callback is called on the thread that calls QueryDownloads().
   void QueryDownloads(const DownloadQueryCallback& callback);
 
-  // Begins a request to clean up entries that has been corrupted (because of
-  // the crash, for example).
-  void CleanUpInProgressEntries();
-
   // Called to update the history service about the current state of a download.
   // This is a 'fire and forget' query, so just pass the relevant state info to
   // the database with no need for a callback.
diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc
index 209421c..e465a80 100644
--- a/chrome/browser/history/history_unittest.cc
+++ b/chrome/browser/history/history_unittest.cc
@@ -213,8 +213,6 @@ void BackendDelegate::BroadcastNotifications(int type,
   delete details;
 }
 
-namespace {
-
 TEST_F(HistoryBackendDBTest, ClearBrowsingData_Downloads) {
   CreateBackendAndDatabase();
 
@@ -516,6 +514,65 @@ TEST_F(HistoryBackendDBTest, DownloadNukeRecordsMissingURLs) {
   }
 }
 
+TEST_F(HistoryBackendDBTest, ConfirmDownloadInProgressCleanup) {
+  // Create the DB.
+  CreateBackendAndDatabase();
+
+  base::Time now(base::Time::Now());
+
+  // Put an IN_PROGRESS download in the DB.
+  AddDownload(DownloadItem::IN_PROGRESS, now);
+
+  // Confirm that they made it into the DB unchanged.
+  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 state, interrupt_reason from downloads"));
+    EXPECT_TRUE(statement1.Step());
+    EXPECT_EQ(DownloadDatabase::kStateInProgress, statement1.ColumnInt(0));
+    EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, statement1.ColumnInt(1));
+    EXPECT_FALSE(statement1.Step());
+  }
+
+  // Read in the DB through query downloads, then test that the
+  // right transformation was returned.
+  CreateBackendAndDatabase();
+  std::vector<DownloadRow> results;
+  db_->QueryDownloads(&results);
+  ASSERT_EQ(1u, results.size());
+  EXPECT_EQ(content::DownloadItem::INTERRUPTED, results[0].state);
+  EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_CRASH,
+            results[0].interrupt_reason);
+
+  // Allow the update to propagate, shut down the DB, and confirm that
+  // the query updated the on disk database as well.
+  MessageLoop::current()->RunUntilIdle();
+  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 state, interrupt_reason from downloads"));
+    EXPECT_TRUE(statement1.Step());
+    EXPECT_EQ(DownloadDatabase::kStateInterrupted, statement1.ColumnInt(0));
+    EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_CRASH,
+              statement1.ColumnInt(1));
+    EXPECT_FALSE(statement1.Step());
+  }
+}
+
 struct InterruptReasonAssociation {
   std::string name;
   int value;
@@ -1635,6 +1692,4 @@ TEST_F(HistoryBackendDBTest, MigratePresentations) {
   STLDeleteElements(&results);
 }
 
-}  // namespace
-
 }  // namespace history