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..c2b4878c21429fc79f9919289fd169d5ec318995 100644
--- a/chrome/browser/history/download_database.cc
+++ b/chrome/browser/history/download_database.cc
@@ -8,9 +8,11 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/debug/alias.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
@@ -20,7 +22,6 @@
 #include "chrome/browser/history/download_row.h"
 #include "chrome/browser/history/history_types.h"
 #include "content/public/browser/download_interrupt_reasons.h"
-#include "content/public/browser/download_item.h"
 #include "sql/statement.h"
 
 using content::DownloadItem;
@@ -30,6 +31,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 +87,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 +116,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 +196,8 @@ DownloadDatabase::DownloadDatabase()
     : owning_thread_set_(false),
       owning_thread_(0),
       next_id_(0),
-      next_db_handle_(0) {
+      next_db_handle_(0),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
 }
 
 DownloadDatabase::~DownloadDatabase() {
@@ -296,6 +298,7 @@ void DownloadDatabase::QueryDownloads(
       "FROM downloads "
       "ORDER BY start_time"));
 
+  bool initiate_db_cleanup(false);
   while (statement_main.Step()) {
     scoped_ptr<DownloadRow> info(new DownloadRow());
     int column = 0;
@@ -343,6 +346,15 @@ void DownloadDatabase::QueryDownloads(
       continue;
     }
 
+    // If the record is the result of a crash during an in-progress download,
+    // clean that record up and arrange for a separate task to be spawned to
+    // clean up the database.
+    if (content::DownloadItem::IN_PROGRESS == info->state) {
+      info->state = content::DownloadItem::INTERRUPTED;
+      info->interrupt_reason = content::DOWNLOAD_INTERRUPT_REASON_CRASH;
+      initiate_db_cleanup = true;
+    }
+
     DCHECK(!ContainsKey(info_map, info->db_handle));
     info_map[db_handle] = info.release();
   }
@@ -391,6 +403,13 @@ void DownloadDatabase::QueryDownloads(
     delete it->second;
     it->second = NULL;
   }
+
+  if (initiate_db_cleanup) {
+    // Don't delay the return of the data.
+    MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(&DownloadDatabase::CleanUpInProgressEntries,
+                              weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
@@ -426,13 +445,14 @@ bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
   return statement.Run();
 }
 
-bool DownloadDatabase::CleanUpInProgressEntries() {
+void DownloadDatabase::CleanUpInProgressEntries() {
   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();

[benjhayden, 2013/03/27 19:38:14]

Do you want to call HistoryBackend::ScheduleCommit?

[Randy Smith (Not in Mondays), 2013/03/27 19:40:00]

I can't call that from inside DownloadDatabase.  Are you suggesting that I move
the spawn-off point up into HistoryBackend?  That would require me to scan the
list of returned downloads, instead of being able to set a bool "for free" when
I need to cleanup the entry.

 }
 
 int64 DownloadDatabase::CreateDownload(