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

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>
 
 #include "base/debug/alias.h"
 #include "base/file_path.h"
 #include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_persistent_store_info.h"
+#include "content/public/browser/download_interrupt_reasons.h"
 #include "sql/statement.h"
 
 using content::DownloadItem;
 using content::DownloadPersistentStoreInfo;
 
 namespace history {
 
 namespace {
 
 static const char kSchema[] =
   "CREATE TABLE downloads ("
-  "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.
+  "id INTEGER PRIMARY KEY,"           // Primary key.
+  "target_path LONGVARCHAR NOT NULL," // Final location of the download on disk.
+  "current_path LONGVARCHAR NOT NULL," // Current location of the download
+                                       // on disk.
   "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
+  "state INTEGER NOT NULL,"           // 1=complete, 4=interrupted
+  "interrupt_reason INTEGER NOT NULL,"// Reason the download was interrupted.
   "end_time INTEGER NOT NULL,"        // When the download completed.
   "opened INTEGER NOT NULL)";         // 1 if it has ever been opened else 0
 
+static const char kUrlChainSchema[] =
+    "CREATE TABLE downloads_url_chains ("
+    "id INTEGER NOT NULL,"           // SQLite-generated primary key.

[sky, 2012/11/14 00:34:31]

Your comment is a bit misleading here. Don't you mean that its 'downloads.id' ?

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

Right.  Copy and paste error.  Done.

+    "chain_index INTEGER NOT NULL,"  // Index of url in chain
+                                     // 0 is initial target,
+                                     // MAX is target after redirects.
+    "url LONGVARCHAR NOT NULL, "     // URL.
+    "PRIMARY KEY (id, chain_index) )";
+
 // 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::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()
+    // We should not need kStateBug140687 here because MigrateDownloadsState()
     // 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) {
@@ skipping 48 matching lines @@
 }
 
 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::MigrateReasonAndPaths() {
+  // We need to rename the table and copy back from it because SQLite
+  // provides no way to rename or delete a column.
+  if (!GetDB().Execute("ALTER TABLE downloads RENAME TO downloads_tmp"))
+    return false;
+
+  // Recreate main table and populate it.
+  if (!GetDB().Execute(kSchema))
+    return false;
+
+  sql::Statement statement_populate(GetDB().GetUniqueStatement(
+    "INSERT INTO downloads "
+    "( id, target_path, current_path, start_time, received_bytes, total_bytes, "
+    "  state, interrupt_reason, end_time, opened ) "
+    "SELECT id, full_path, full_path, start_time, received_bytes, total_bytes, "
+    "       state, ?, end_time, opened "
+    "FROM downloads_tmp"));
+  statement_populate.BindInt(0, content::DOWNLOAD_INTERRUPT_REASON_NONE);

[sky, 2012/11/14 00:34:31]

I get nervous when we start persisting enums from different parts of the code
base, especially when the enum does not mention this at all.

[benjhayden, 2012/11/14 19:43:22]

I agree with Scott. This is how we ran into trouble with the state.
A mapping layer like we have with state would be ideal.
There are currently about 20 interrupt_reasons, which seems feasible. If there
were >100, I'd start thinking about testing the immutability of the
interrupt_reason set.

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

Fair enough.  There are enough errors in the enum that I'm reluctant to
construct a mapping function in this file, but I did put a note in
download_interrupt_reason_values.h indicating that the values may be persisted
and shouldn't be changed.  WDYT?

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

Still pushing back, Milo--keeping 20 interrupt reasons in sync with the original
location strikes me as a fairly high maintenance burden.  I have marked the
original location as "don't change; you can add, but reserve values once they're
in place".  Willing to let me get away with that?

[benjhayden, 2012/11/15 15:57:41]

Comments don't do anything for me or the compiler.
I don't know about Scott, but I'd settle for a test that makes it extremely
clear that it's not the kind of test that one simply walks into and updates to
reflect changes to the xmacros file.

TEST(ProtectDownloadDatabaseFromCarelessInterruptReasonSchemaChange) {
  static const char kMessage[] = "\n\nYOU BROKE THE DownloadDatabase
SCHEMA!\n\n"
    "If interrupt reason values are changed, then many users' downloads
databases will be corrupted.\n"
    "New values may be assigned safely.\n"
    "If a value is removed from download_interrupt_reason_values.h, it MUST
NEVER be removed from kHistoricalValues, and it MUST NEVER be reassigned to a
new reason.\n";

  static const struct {int value; const char* name;} kHistoricalValues[] = {{1,
"FILE_FAILED"}, ...};

#define INTERRUPT_REASON(name, value) EXPECT_STREQ(name,
lookup(kHistoricalValues, value)) << kMessage;
#include "download_interrupt_reason_values.h"
#undef INTERRUPT_REASON
}

The goal is not to increase maintenance burden any more than necessary, but to
provide just enough of a speed bump at just the right place to prevent a
maintainer from accidentally speeding through the stop sign comment.

+  if (!statement_populate.Run())
+    return false;
+
+  // Create new chain table and populate it.
+  if (!GetDB().Execute(kUrlChainSchema))
+    return false;
+
+  if (!GetDB().Execute("INSERT INTO downloads_url_chains "
+                       "  ( id, chain_index, url) "
+                       "  SELECT id, 0, url from downloads_tmp"))
+    return false;
+
+  // Get rid of temporary table.
+  if (!GetDB().Execute("DROP TABLE downloads_tmp"))
+    return false;
+
+  return true;
+}
+
 bool DownloadDatabase::InitDownloadTable() {
   CheckThread();
   GetMetaTable().GetValue(kNextDownloadId, &next_id_);
   if (GetDB().DoesTableExist("downloads")) {

[benjhayden, 2012/11/14 19:43:22]

&& DoesTableExist("downloads_url_chains")?

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

Done, though I'm a little uncomfortable that I don't understand the "threat
model" that this function is supposed to be protecting against.  The obvious one
is database version skew, but we're handling that elsewhere. 

Scott, care to comment?  What kinds of gotchas am I looking to remedy here; only
a clean DB?  If so, why the EnsureColumnExists() statements?

[benjhayden, 2012/11/15 15:57:41]

The schema change represented by these EnsureColumnExists calls did not
correspond with a database version bump. The primary threat model guarded by
this method is when the table[s] don't exist at all. The EnsureColumnExists
parts guard against the secondary threat of schema skew because that wasn't
handled with a version bump.

Since you are bumping the version, you might be able to rewrite this function as
return ((DoesTableExist("downloads") || Execute(kSchema)) &&
        (DoesTableExist("downloads_url_chains") || Execute(kUrlChainSchema));

Scott might have a better idea.

     return EnsureColumnExists("end_time", "INTEGER NOT NULL DEFAULT 0") &&
            EnsureColumnExists("opened", "INTEGER NOT NULL DEFAULT 0");
   } else {
-    return GetDB().Execute(kSchema);
+    return GetDB().Execute(kSchema) && GetDB().Execute(kUrlChainSchema);
   }
 }
 
 bool DownloadDatabase::DropDownloadTable() {
   CheckThread();
   return GetDB().Execute("DROP TABLE downloads");
 }
 
 void DownloadDatabase::QueryDownloads(
     std::vector<DownloadPersistentStoreInfo>* results) {
   CheckThread();
   results->clear();
   if (next_db_handle_ < 1)
     next_db_handle_ = 1;
   std::set<DownloadID> db_handles;
 
-  sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
-      "SELECT id, full_path, url, start_time, received_bytes, "
-        "total_bytes, state, end_time, opened "
+  std::map<DownloadID, DownloadPersistentStoreInfo*> info_map;
+
+  sql::Statement statement_main(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "SELECT id, target_path, current_path, start_time, received_bytes, "
+        "total_bytes, state, interrupt_reason, end_time, opened "
       "FROM downloads "
       "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);
-    int state = statement.ColumnInt(6);
-    info.state = IntToState(state);
-    info.end_time = base::Time::FromTimeT(statement.ColumnInt64(7));
-    info.opened = statement.ColumnInt(8) != 0;
-    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);
+  while (statement_main.Step()) {
+    DownloadPersistentStoreInfo* info(new DownloadPersistentStoreInfo);
+    int column = 0;

[benjhayden, 2012/11/14 19:43:22]

I've also been very tempted to do this, and it is clever, but it also makes it
very easy to accidentally mismatch columns, and hard to notice when that
happens, so it's very fragile code.
Do you want to define kConstants for the column indices instead? They'd be used
both here and in the INSERT statement.

Bonus points if you want to split the DPSI reading/writing out to co-located
static functions alongside these kConstantColumnIndices and a string "id,
target_path, ... opened".

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

Does that make it different than what it replaces?  I see this as being very
slightly easier to figure out if you've got a match (the algorithm is to compare
against the previous column in lines of code and in the SQL, versus counting up
a number that's an absolute location), and noticeably easier to modify if you
need to.  Doesn't mean it's not fragile, but I think it's still an improvement. 
That doesn't make the code easy to modify the way this does.
You say it like it's a good thing :-}.  More specifically: The column numbers
are very much SQL statement dependent.  Sharing constants between different SQL
statements doesn't strike me as a win; then you've got a dependency between
those constants and the contents of the SQL statements.  
That would work, but seems like real overkill for this problem.

[benjhayden, 2012/11/15 15:57:41]

I think there's a philosophical subtlety here. Let's chat offline.

+
+    info->db_handle = statement_main.ColumnInt64(column++);
+    info->target_path = ColumnFilePath(statement_main, column++);
+    info->current_path = ColumnFilePath(statement_main, column++);
+    info->start_time = base::Time::FromTimeT(
+        statement_main.ColumnInt64(column++));
+    info->received_bytes = statement_main.ColumnInt64(column++);
+    info->total_bytes = statement_main.ColumnInt64(column++);
+    int state = statement_main.ColumnInt(column++);
+    info->state = IntToState(state);
+    info->interrupt_reason = static_cast<content::DownloadInterruptReason>(
+        statement_main.ColumnInt(column++));
+    info->end_time = base::Time::FromTimeT(
+        statement_main.ColumnInt64(column++));
+    info->opened = statement_main.ColumnInt(column++) != 0;
+    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);
     }
-    if (info.state == DownloadItem::MAX_DOWNLOAD_STATE) {
+    if (info->state == DownloadItem::MAX_DOWNLOAD_STATE) {
       UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
       continue;
     }
-    results->push_back(info);
+    DCHECK(!ContainsKey(info_map, info->db_handle));
+    info_map[info->db_handle] = info;
+  }
+
+  sql::Statement statement_chain(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT id, chain_index, url FROM downloads_url_chains "
+      "ORDER BY id, chain_index"));
+
+  while (statement_chain.Step()) {
+    int64 db_handle = statement_chain.ColumnInt64(0);
+    int chain_index = statement_chain.ColumnInt(1);
+
+    // Note that these DCHECKs may trip as a result of corrupted databases.
+    // We have them because in debug builds the chances are higher there's
+    // an actual bug than that the database is corrupt, but we handle the
+    // DB corruption case in production code.
+
+    // Confirm the handle has already been seen--if it hasn't, discard the
+    // record.
+    DCHECK(ContainsKey(info_map, db_handle));
+    if (!ContainsKey(info_map, db_handle))
+      continue;
+
+    // Confirm all previous URLs in the chain have already been seen;
+    // if not, fill in with null or discard record.
+    int current_chain_size = info_map[db_handle]->url_chain.size();
+    std::vector<GURL>* url_chain(&info_map[db_handle]->url_chain);
+    DCHECK_EQ(chain_index, current_chain_size);
+    while (current_chain_size < chain_index) {
+      url_chain->push_back(GURL());
+      current_chain_size++;
+    }
+    if (current_chain_size > chain_index)
+      continue;

[benjhayden, 2012/11/14 19:43:22]

Why not overwrite?

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

I flipped a coin :-}.  Do you have some reason to think that overwrite might be
better?  I don't have a dog in the race.

[benjhayden, 2012/11/15 15:57:41]

Ah, I missed the ORDER BY clause and thought that you might see urls out of
order.
Nevermind. :-)

+
+    // Save the record.
+    url_chain->push_back(GURL(statement_chain.ColumnString(2)));
+  }
+
+  for (std::map<DownloadID, DownloadPersistentStoreInfo*>::iterator
+           it = info_map.begin(); it != info_map.end(); ++it) {
+    results->push_back(*it->second);
+    delete it->second;
+    it->second = NULL;
   }
 }
 
 bool DownloadDatabase::UpdateDownload(const DownloadPersistentStoreInfo& data) {
   CheckThread();
   DCHECK(data.db_handle > 0);
   int state = StateToInt(data.state);
   if (state == kStateInvalid) {
     // TODO(benjhayden) [D]CHECK instead.
     return false;
   }
   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, state);
-  statement.BindInt64(2, data.end_time.ToTimeT());
-  statement.BindInt(3, (data.opened ? 1 : 0));
-  statement.BindInt64(4, data.db_handle);
+      "SET received_bytes=?, state=?, interrupt_reason=?, end_time=?, "
+      " opened=? WHERE id=?"));
+
+  int column = 0;
+  statement.BindInt64(column++, data.received_bytes);
+  statement.BindInt(column++, state);
+  statement.BindInt(column++, data.interrupt_reason);
+  statement.BindInt64(column++, data.end_time.ToTimeT());
+  statement.BindInt(column++, (data.opened ? 1 : 0));
+  statement.BindInt64(column++, data.db_handle);
 
   return statement.Run();
 }
 
-bool DownloadDatabase::UpdateDownloadPath(const FilePath& path,
-                                          DownloadID db_handle) {
+bool DownloadDatabase::UpdateDownloadPath(
+    const FilePath& target_path, const FilePath& current_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);
+      "UPDATE downloads SET target_path=?, current_path=? WHERE id=?"));
+  BindFilePath(statement, target_path, 0);
+  BindFilePath(statement, current_path, 0);

[benjhayden, 2012/11/14 19:43:22]

Check the column index here.

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

Whoops.  Good catch.  Done.  I'll see if I can write a test for this, as well;
it should have been caught by testing.

   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, kStateCancelled);
@@ skipping 11 matching lines @@
     // 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());

[sky, 2012/11/14 00:34:31]

Since you're writing migration code, could you migrate this to use
ToInternalValue like we do every where else for times?

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

Done.

-  statement.BindInt64(4, info.received_bytes);
-  statement.BindInt64(5, info.total_bytes);
-  statement.BindInt(6, state);
-  statement.BindInt64(7, info.end_time.ToTimeT());
-  statement.BindInt(8, info.opened ? 1 : 0);
+  {
+    sql::Statement statement_insert(GetDB().GetCachedStatement(
+        SQL_FROM_HERE,
+        "INSERT INTO downloads "
+        "(id, target_path, current_path, start_time, "
+        " received_bytes, total_bytes, state, interrupt_reason, "
+        " end_time, opened) "
+        "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
 
-  if (statement.Run()) {
-    // TODO(benjhayden) if(info.id>next_id_){setvalue;next_id_=info.id;}
-    GetMetaTable().SetValue(kNextDownloadId, ++next_id_);
+    int column = 0;
+    statement_insert.BindInt64(column++, db_handle);
+    BindFilePath(statement_insert, info.target_path, column++);
+    BindFilePath(statement_insert, info.current_path, column++);
+    statement_insert.BindInt64(column++, info.start_time.ToTimeT());
+    statement_insert.BindInt64(column++, info.received_bytes);
+    statement_insert.BindInt64(column++, info.total_bytes);
+    statement_insert.BindInt(column++, state);
+    statement_insert.BindInt(column++, content::DOWNLOAD_INTERRUPT_REASON_NONE);
+    statement_insert.BindInt64(column++, info.end_time.ToTimeT());
+    statement_insert.BindInt(column++, info.opened ? 1 : 0);
+    if (!statement_insert.Run()) {
+      LOG(WARNING) << "Main insertion for download create failed.";
+      return 0;

[benjhayden, 2012/11/14 19:43:22]

When you merge with my DownloadHistory, please be sure that this and the other
'return 0' change to returning kUninitializedHandle.

[Randy Smith (Not in Mondays), 2012/11/14 22:47:34]

Will do.

+    }
+  }
 
-    return db_handle;
+  sql::Statement statement_insert_chain(
+      GetDB().GetCachedStatement(SQL_FROM_HERE,
+                                 "INSERT INTO downloads_url_chains "
+                                 "(id, chain_index, url) "
+                                 "VALUES (?, ?, ?)"));
+  for (size_t i = 0; i < info.url_chain.size(); ++i) {
+    statement_insert_chain.BindInt64(0, db_handle);
+    statement_insert_chain.BindInt(1, i);
+    statement_insert_chain.BindString(2, info.url_chain[i].spec());
+    if (!statement_insert_chain.Run()) {
+      LOG(WARNING) << "Url insertion for download create failed.";
+      return 0;
+    }
+    statement_insert_chain.Reset(true);
   }
-  return 0;
+
+  // TODO(benjhayden) if(info.id>next_id_){setvalue;next_id_=info.id;}
+  GetMetaTable().SetValue(kNextDownloadId, ++next_id_);
+
+  return db_handle;
 }
 
 void DownloadDatabase::RemoveDownload(DownloadID db_handle) {
   CheckThread();
 
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM downloads WHERE id=?"));
   statement.BindInt64(0, db_handle);
 
   statement.Run();
@@ skipping 51 matching lines @@
     if (num_downloads_deleted > 0) {
       UMA_HISTOGRAM_TIMES("Download.DatabaseRemoveDownloadsTimePerRecord",
                           (1000 * micros) / num_downloads_deleted);
     }
   }
 
   return success;
 }
 
 }  // namespace history