Fix downloads db state=3 corruption using version=23

chrome/browser/history/download_database.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.
 
 #include "chrome/browser/history/download_database.h"
 
 #include <limits>
 #include <string>
 #include <vector>
 
@@ skipping 20 matching lines @@
   "id INTEGER PRIMARY KEY,"           // SQLite-generated primary key.
   "full_path LONGVARCHAR NOT NULL,"   // Location of the download on disk.
   "url LONGVARCHAR NOT NULL,"         // URL of the downloaded file.
   "start_time INTEGER NOT NULL,"      // When the download was started.
   "received_bytes INTEGER NOT NULL,"  // Total size downloaded.
   "total_bytes INTEGER NOT NULL,"     // Total size of the download.
   "state INTEGER NOT NULL,"           // 1=complete, 2=cancelled, 4=interrupted
   "end_time INTEGER NOT NULL,"        // When the download completed.
   "opened INTEGER NOT NULL)";         // 1 if it has ever been opened else 0
 
+// These constants and next two functions are used to allow
+// DownloadItem::DownloadState 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 DownloadPersistentStoreInfos 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;
+
+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::REMOVING: return kStateInvalid;
+    case DownloadItem::INTERRUPTED: return kStateInterrupted;
+    case DownloadItem::MAX_DOWNLOAD_STATE: return kStateInvalid;
+    default: return kStateInvalid;
+  }
+}
+
+DownloadItem::DownloadState IntToState(int state) {
+  switch (state) {
+    case kStateInProgress: return DownloadItem::IN_PROGRESS;
+    case kStateComplete: return DownloadItem::COMPLETE;
+    case kStateCancelled: return DownloadItem::CANCELLED;
+    // We should not need kStateBug140687 here because MigrateDownloadState()
+    // is called in HistoryDatabase::Init().
+    case kStateInterrupted: return DownloadItem::INTERRUPTED;
+    default: return DownloadItem::MAX_DOWNLOAD_STATE;
+  }
+}
+
 #if defined(OS_POSIX)
 
 // Binds/reads the given file path to the given column of the given statement.
 void BindFilePath(sql::Statement& statement, const FilePath& path, int col) {
   statement.BindString(col, path.value());
 }
 FilePath ColumnFilePath(sql::Statement& statement, int col) {
   return FilePath(statement.ColumnString(col));
 }
 
@@ skipping 34 matching lines @@
   }
 }
 
 bool DownloadDatabase::EnsureColumnExists(
     const std::string& name, const std::string& type) {
   std::string add_col = "ALTER TABLE downloads ADD COLUMN " + name + " " + type;
   return GetDB().DoesColumnExist("downloads", name.c_str()) ||
          GetDB().Execute(add_col.c_str());
 }
 
+bool DownloadDatabase::MigrateDownloadsState() {
+  sql::Statement statement(GetDB().GetUniqueStatement(
+        "UPDATE downloads SET state=? WHERE state=?"));
+  statement.BindInt(0, kStateInterrupted);
+  statement.BindInt(1, kStateBug140687);
+  return statement.Run();
+}
+
 bool DownloadDatabase::InitDownloadTable() {
   CheckThread();
   GetMetaTable().GetValue(kNextDownloadId, &next_id_);
   if (GetDB().DoesTableExist("downloads")) {
     return EnsureColumnExists("end_time", "INTEGER NOT NULL DEFAULT 0") &&
            EnsureColumnExists("opened", "INTEGER NOT NULL DEFAULT 0");
   } else {
     return GetDB().Execute(kSchema);
   }
 }
@@ skipping 18 matching lines @@
       "ORDER BY start_time"));
 
   while (statement.Step()) {
     DownloadPersistentStoreInfo info;
     info.db_handle = statement.ColumnInt64(0);
     info.path = ColumnFilePath(statement, 1);
     info.url = GURL(statement.ColumnString(2));
     info.start_time = base::Time::FromTimeT(statement.ColumnInt64(3));
     info.received_bytes = statement.ColumnInt64(4);
     info.total_bytes = statement.ColumnInt64(5);
-    info.state = statement.ColumnInt(6);
+    info.state = IntToState(statement.ColumnInt(6));
     info.end_time = base::Time::FromTimeT(statement.ColumnInt64(7));
     info.opened = statement.ColumnInt(8) != 0;
+    if (info.state == DownloadItem::MAX_DOWNLOAD_STATE) {
+      continue;

[Randy Smith (Not in Mondays), 2012/08/07 20:36:16]

UMA?  (You're welcome to say not in this CL, but I do want it, because I don't
want us to just ignore problems like the one that just happened.)

[sky, 2012/08/07 20:54:17]

Why add this? At best we do a DCHECK for this sort of thing.

[asanka, 2012/08/07 20:56:14]

If you skip this early, wouldn't you risk not updating next_db_handle_ ? Or is
that okay since this entry is considered invalid?

[Randy Smith (Not in Mondays), 2012/08/07 21:01:35]

The basic problem is that almost any sanitization would have meant we didn't
notice the bug that started all this, which showed up as a crash on canary.  So
I want something that has some chance of letting us know if there's a lot of DB
corruption out in the wild, so that we can respond to it and make sure it's not
the result of a bug in the code.  Is there some better pattern?

[benjhayden, 2012/08/08 01:11:52]

This layer of abstraction separates the compiler-generated enum values for
DownloadState from the db schema values for |state| in order to protect against
the kind of change to DownloadState that caused this mess.

[benjhayden, 2012/08/08 01:11:52]

Done.

[benjhayden, 2012/08/08 01:11:52]

Done.

[benjhayden, 2012/08/08 01:11:52]

This isn't just sanitization. It's a mapping, a layer of abstraction to protect
the schema from changes to DownloadState.
I also added the UMA.

[Randy Smith (Not in Mondays), 2012/08/08 01:52:25]

Good point.  Ok.

+    }
     results->push_back(info);
     if (info.db_handle >= next_db_handle_)
       next_db_handle_ = info.db_handle + 1;
     if (!db_handles.insert(info.db_handle).second) {
       // info.db_handle was already in db_handles. The database is corrupt.
       base::debug::Alias(&info.db_handle);
       DCHECK(false);
     }
   }
 }
 
 bool DownloadDatabase::UpdateDownload(const DownloadPersistentStoreInfo& data) {
   CheckThread();
   DCHECK(data.db_handle > 0);
+  int state = StateToInt(data.state);
+  if (state == kStateInvalid)
+    return false;

[Randy Smith (Not in Mondays), 2012/08/07 20:36:16]

DCHECK?  If we get an invalid state here, it means something in the rest of the
program has done something wrong.

[benjhayden, 2012/08/08 01:11:52]

Is it possible for UpdateDownload to see a download in the REMOVING state?
How sure are you?
I'll put in a todo.

[Randy Smith (Not in Mondays), 2012/08/08 01:52:25]

Not sure enough for this CL :-} :-|.
Sounds good.

   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE downloads "
       "SET received_bytes=?, state=?, end_time=?, opened=? WHERE id=?"));
   statement.BindInt64(0, data.received_bytes);
-  statement.BindInt(1, data.state);
+  statement.BindInt(1, state);
   statement.BindInt64(2, data.end_time.ToTimeT());
   statement.BindInt(3, (data.opened ? 1 : 0));
   statement.BindInt64(4, data.db_handle);
 
   return statement.Run();
 }
 
 bool DownloadDatabase::UpdateDownloadPath(const FilePath& path,
                                           DownloadID db_handle) {
   CheckThread();
   DCHECK(db_handle > 0);
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE downloads SET full_path=? WHERE id=?"));
   BindFilePath(statement, path, 0);
   statement.BindInt64(1, db_handle);
 
   return statement.Run();
 }
 
 bool DownloadDatabase::CleanUpInProgressEntries() {
   CheckThread();
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE downloads SET state=? WHERE state=?"));
-  statement.BindInt(0, DownloadItem::CANCELLED);
-  statement.BindInt(1, DownloadItem::IN_PROGRESS);
+  statement.BindInt(0, kStateCancelled);
+  statement.BindInt(1, kStateInProgress);
 
   return statement.Run();
 }
 
 int64 DownloadDatabase::CreateDownload(
     const DownloadPersistentStoreInfo& info) {
   CheckThread();
 
   if (next_db_handle_ == 0) {
     // This is unlikely. All current known tests and users already call
     // QueryDownloads() before CreateDownload().
     std::vector<DownloadPersistentStoreInfo> results;
     QueryDownloads(&results);
     CHECK_NE(0, next_db_handle_);
   }
 
+  int state = StateToInt(info.state);
+  if (state == kStateInvalid)
+    return false;
+
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO downloads "
       "(id, full_path, url, start_time, received_bytes, total_bytes, state, "
       "end_time, opened) "
       "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)"));
 
   int db_handle = next_db_handle_++;
 
   statement.BindInt64(0, db_handle);
   BindFilePath(statement, info.path, 1);
   statement.BindString(2, info.url.spec());
   statement.BindInt64(3, info.start_time.ToTimeT());
   statement.BindInt64(4, info.received_bytes);
   statement.BindInt64(5, info.total_bytes);
-  statement.BindInt(6, info.state);
+  statement.BindInt(6, state);
   statement.BindInt64(7, info.end_time.ToTimeT());
   statement.BindInt(8, info.opened ? 1 : 0);
 
   if (statement.Run()) {
     // TODO(benjhayden) if(info.id>next_id_){setvalue;next_id_=info.id;}
     GetMetaTable().SetValue(kNextDownloadId, ++next_id_);
 
     return db_handle;
   }
   return 0;
@@ skipping 17 matching lines @@
 
   int num_downloads_deleted = -1;
   {
     sql::Statement count(GetDB().GetCachedStatement(SQL_FROM_HERE,
         "SELECT count(*) FROM downloads WHERE start_time >= ? "
         "AND start_time < ? AND (State = ? OR State = ? OR State = ?)"));
     count.BindInt64(0, start_time);
     count.BindInt64(
         1,
         end_time ? end_time : std::numeric_limits<int64>::max());
-    count.BindInt(2, DownloadItem::COMPLETE);
-    count.BindInt(3, DownloadItem::CANCELLED);
-    count.BindInt(4, DownloadItem::INTERRUPTED);
+    count.BindInt(2, kStateComplete);
+    count.BindInt(3, kStateCancelled);
+    count.BindInt(4, kStateInterrupted);
     if (count.Step())
       num_downloads_deleted = count.ColumnInt(0);
   }
 
 
   bool success = false;
   base::TimeTicks started_removing = base::TimeTicks::Now();
   {
     // This does not use an index. We currently aren't likely to have enough
     // downloads where an index by time will give us a lot of benefit.
     sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
         "DELETE FROM downloads WHERE start_time >= ? AND start_time < ? "
         "AND (State = ? OR State = ? OR State = ?)"));
     statement.BindInt64(0, start_time);
     statement.BindInt64(
         1,
         end_time ? end_time : std::numeric_limits<int64>::max());
-    statement.BindInt(2, DownloadItem::COMPLETE);
-    statement.BindInt(3, DownloadItem::CANCELLED);
-    statement.BindInt(4, DownloadItem::INTERRUPTED);
+    statement.BindInt(2, kStateComplete);
+    statement.BindInt(3, kStateCancelled);
+    statement.BindInt(4, kStateInterrupted);
 
     success = statement.Run();
   }
 
   base::TimeTicks finished_removing = base::TimeTicks::Now();
 
   if (num_downloads_deleted >= 0) {
     UMA_HISTOGRAM_COUNTS("Download.DatabaseRemoveDownloadsCount",
                          num_downloads_deleted);
     base::TimeDelta micros = (1000 * (finished_removing - started_removing));
     UMA_HISTOGRAM_TIMES("Download.DatabaseRemoveDownloadsTime", micros);
     if (num_downloads_deleted > 0) {
       UMA_HISTOGRAM_TIMES("Download.DatabaseRemoveDownloadsTimePerRecord",
                           (1000 * micros) / num_downloads_deleted);
     }
   }
 
   return success;
 }
 
 }  // namespace history