Clean up entries left by crashes in the DownloadDB.

chrome/browser/history/download_database.cc

Index: chrome/browser/history/download_database.cc
diff --git a/chrome/browser/history/download_database.cc b/chrome/browser/history/download_database.cc
index 0b91d4d52b880c66b91b20ce2b3015532604bb5e..8bbe2ddd23f4e725c59166969cc89e9742184c31 100644
--- a/chrome/browser/history/download_database.cc
+++ b/chrome/browser/history/download_database.cc
@@ -30,6 +30,30 @@ namespace history {
 // 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().
+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;
+
 namespace {
 
 // Reason for dropping a particular record.
@@ -62,52 +86,28 @@ static const char kUrlChainSchema[] =
     "url LONGVARCHAR NOT NULL, "     // URL.
     "PRIMARY KEY (id, chain_index) )";
 
-// These constants and next two functions 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) {
   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) {
   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;
   }
 }
@@ -115,46 +115,46 @@ DownloadItem::DownloadState IntToState(int state) {
 int 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) {
   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;
@@ -195,7 +195,8 @@ 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,9 @@ bool DownloadDatabase::DropDownloadTable() {
 
 void DownloadDatabase::QueryDownloads(
     std::vector<DownloadRow>* results) {
+  if (!in_progress_entry_cleanup_completed_)
+    CleanUpInProgressEntries();
+
   results->clear();
   if (next_db_handle_ < 1)
     next_db_handle_ = 1;
@@ -394,6 +398,9 @@ void DownloadDatabase::QueryDownloads(
 }
 
 bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
+  if (!in_progress_entry_cleanup_completed_)
+    CleanUpInProgressEntries();
+
   DCHECK(data.db_handle > 0);
   int state = StateToInt(data.state);
   if (state == kStateInvalid) {
@@ -426,17 +433,22 @@ bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
   return statement.Run();
 }
 
-bool DownloadDatabase::CleanUpInProgressEntries() {
+void DownloadDatabase::CleanUpInProgressEntries() {

[brettw, 2013/04/01 23:52:51]

I sort of prefer the scheme where the function is
EnsureInProgressEntriesCleanedUp() and it checks the flag and returns early,
which saves you the bool check in every caller. You can keep it like this if you
prefer, though.

[Randy Smith (Not in Mondays), 2013/04/02 18:58:52]

Done.

   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) {
+  if (!in_progress_entry_cleanup_completed_)
+    CleanUpInProgressEntries();
+
   if (next_db_handle_ == 0) {
     // This is unlikely. All current known tests and users already call
     // QueryDownloads() before CreateDownload().
@@ -505,6 +517,9 @@ int64 DownloadDatabase::CreateDownload(
 }
 
 void DownloadDatabase::RemoveDownload(int64 handle) {
+  if (!in_progress_entry_cleanup_completed_)
+    CleanUpInProgressEntries();
+
   sql::Statement downloads_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM downloads WHERE id=?"));
   downloads_statement.BindInt64(0, handle);
@@ -517,6 +532,9 @@ void DownloadDatabase::RemoveDownload(int64 handle) {
 }
 
 int DownloadDatabase::CountDownloads() {
+  if (!in_progress_entry_cleanup_completed_)
+    CleanUpInProgressEntries();
+
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT count(*) from downloads"));
   statement.Step();