Put history insertion for downloads processing inline.

This change shifts standard download processing to wait on history
insertion (note that actual data download does not wait).  This makes the
download process less racy and is expected to make the download tests less
flaky.

Note that it also brings the rename to the download intermediate file rename
inline into standard download processing; i.e. now we always rename to the
intermediate file name and then to the final file name.  This again reduces
raciness and possibly tset flakiness.

BUG=63237
TEST=All current download tests.

Review URL: http://codereview.chromium.org/6096003

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

6 files changed, 167 insertions, 106 deletions

diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index cb753e8..620828b 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -27,6 +27,23 @@
 #include "chrome/common/pref_names.h"
 #include "ui/base/l10n/l10n_util.h"
 
+// A DownloadItem normally goes through the following states:
+//      * Created (when download starts)
+//      * Made visible to consumers (e.g. Javascript) after the
+//        destination file has been determined.
+//      * Entered into the history database.
+//      * Made visible in the download shelf.
+//      * All data is received.  Note that the actual data download occurs
+//        in parallel with the above steps, but until those steps are
+//        complete, completion of the data download will be ignored.
+//      * Download file is renamed to its final name, and possibly
+//        auto-opened.
+// TODO(rdsmith): This progress should be reflected in
+// DownloadItem::DownloadState and a state transition table/state diagram.
+//
+// TODO(rdsmith): This description should be updated to reflect the cancel
+// pathways.
+
 namespace {
 
 // Update frequency (milliseconds).
@@ -102,9 +119,12 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
       is_extension_install_(info.is_extension_install),
       name_finalized_(false),
       is_temporary_(false),
+      all_data_saved_(false),
       opened_(false) {
   if (state_ == IN_PROGRESS)
     state_ = CANCELLED;
+  if (state_ == COMPLETE)
+    all_data_saved_ = true;
   Init(false /* don't start progress timer */);
 }
 
@@ -139,6 +159,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
       is_extension_install_(info.is_extension_install),
       name_finalized_(false),
       is_temporary_(!info.save_info.file_path.empty()),
+      all_data_saved_(false),
       opened_(false) {
   Init(true /* start progress timer */);
 }
@@ -174,6 +195,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
       is_extension_install_(false),
       name_finalized_(false),
       is_temporary_(false),
+      all_data_saved_(false),
       opened_(false) {
   Init(true /* start progress timer */);
 }
@@ -299,8 +321,14 @@ void DownloadItem::Cancel(bool update_history) {
     download_manager_->DownloadCancelled(id_);
 }
 
-void DownloadItem::OnAllDataSaved(int64 size) {
+void DownloadItem::MarkAsComplete() {
+  DCHECK(all_data_saved_);
   state_ = COMPLETE;
+}
+
+void DownloadItem::OnAllDataSaved(int64 size) {
+  DCHECK(!all_data_saved_);
+  all_data_saved_ = true;
   UpdateSize(size);
   StopProgressTimer();
 }
@@ -324,7 +352,7 @@ void DownloadItem::Finished() {
     auto_opened_ = true;
   }
 
-  // Notify our observers that we are complete (the call to OnAllDataSaved()
+  // Notify our observers that we are complete (the call to MarkAsComplete()
   // set the state to complete but did not notify).
   UpdateObservers();
 
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index bc32f63..eaa9a55 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -40,8 +40,19 @@ class DownloadItem {
  public:
   enum DownloadState {
     IN_PROGRESS,
+
+    // Note that COMPLETE indicates that the download has gotten all of its
+    // data, has figured out its final destination file, has been entered
+    // into the history store, and has been shown in the UI.  The only
+    // operations remaining are acceptance by the user of
+    // a dangerous download (if dangerous), renaming the file
+    // to the final name, and auto-opening it (if it auto-opens).
     COMPLETE,
+
     CANCELLED,
+
+    // This state indicates that the download item is about to be destroyed,
+    // and observers seeing this state should release all references.
     REMOVING
   };
 
@@ -128,9 +139,13 @@ class DownloadItem {
   // when resuming a download (assuming the server supports byte ranges).
   void Cancel(bool update_history);
 
-  // Called when all data has been saved.
+  // Called when all data has been saved.  Only has display effects.
   void OnAllDataSaved(int64 size);
 
+  // Called when ready to consider the data received and move on to the
+  // next stage.
+  void MarkAsComplete();
+
   // Called when the entire download operation (including renaming etc)
   // is finished.
   void Finished();
@@ -152,6 +167,9 @@ class DownloadItem {
   // total size).
   int PercentComplete() const;
 
+  // Whether or not this download has saved all of its data.
+  bool all_data_saved() const { return all_data_saved_; }
+
   // Update the fields that may have changed in DownloadCreateInfo as a
   // result of analyzing the file and figuring out its type, location, etc.
   // May only be called once.
@@ -340,6 +358,9 @@ class DownloadItem {
   // True if the item was downloaded temporarily.
   bool is_temporary_;
 
+  // True if we've saved all the data for the download.
+  bool all_data_saved_;
+
   // Did the user open the item either directly or indirectly (such as by
   // setting always open files of this type)? The shelf also sets this field
   // when the user closes the shelf before the item has been opened but should
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 740177e..7e66e50 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -240,7 +240,7 @@ bool DownloadManager::Init(Profile* profile) {
 // history system of a new download. Since this method can be called while the
 // history service thread is still reading the persistent state, we do not
 // insert the new DownloadItem into 'history_downloads_' or inform our
-// observers at this point. OnCreateDatabaseEntryComplete() handles that
+// observers at this point. OnCreateDownloadEntryComplete() handles that
 // finalization of the the download creation as a callback from the
 // history thread.
 void DownloadManager::StartDownload(DownloadCreateInfo* info) {
@@ -438,6 +438,8 @@ void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
 
 void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
                                          const FilePath& target_path) {
+  VLOG(20) << __FUNCTION__ << "()" << " info = " << info->DebugString();
+
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   scoped_ptr<DownloadCreateInfo> infop(info);
@@ -458,29 +460,25 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
                                 info->is_extension_install,
                                 info->original_name);
   in_progress_[info->download_id] = download;
+  UpdateAppIcon();  // Reflect entry into in_progress_.
 
-  bool download_finished = ContainsKey(pending_finished_downloads_,
-                                       info->download_id);
-
-  VLOG(20) << __FUNCTION__ << "()"
-           << " target_path = \"" << target_path.value() << "\""
-           << " download_finished = " << download_finished
-           << " info = " << info->DebugString()
-           << " download = " << download->DebugString(true);
-
-  if (download_finished || info->is_dangerous) {
-    // The download has already finished or the download is not safe.
-    // We can now rename the file to its final name (or its tentative name
-    // in dangerous download cases).
+  // Rename to intermediate name.
+  if (info->is_dangerous) {
+    // The download is not safe.  We can now rename the file to its
+    // tentative name using OnFinalDownloadName (the actual final
+    // name after user confirmation will be set in
+    // ProceedWithFinishedDangerousDownload).
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(
             file_manager_, &DownloadFileManager::OnFinalDownloadName,
-            download->id(), target_path, !info->is_dangerous,
+            download->id(), target_path, false,
             make_scoped_refptr(this)));
   } else {
-    // The download hasn't finished and it is a safe download.  We need to
-    // rename it to its intermediate '.crdownload' path.
+    // The download is a safe download.  We need to
+    // rename it to its intermediate '.crdownload' path.  The final
+    // name after user confirmation will be set from
+    // DownloadItem::OnSafeDownloadFinished.
     FilePath download_path = download_util::GetCrDownloadPath(target_path);
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
@@ -490,17 +488,8 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
     download->Rename(download_path);
   }
 
-  if (download_finished) {
-    // If the download already completed by the time we reached this point, then
-    // notify observers that it did.
-    OnAllDataSaved(info->download_id,
-                   pending_finished_downloads_[info->download_id]);
-  }
-
   download_history_->AddEntry(*info, download,
       NewCallback(this, &DownloadManager::OnCreateDownloadEntryComplete));
-
-  UpdateAppIcon();
 }
 
 void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
@@ -510,73 +499,93 @@ void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
     DownloadItem* download = it->second;
     if (download->state() == DownloadItem::IN_PROGRESS) {
       download->Update(size);
+      UpdateAppIcon();  // Reflect size updates.
       download_history_->UpdateEntry(download);
     }
   }
-  UpdateAppIcon();
 }
 
 void DownloadManager::OnAllDataSaved(int32 download_id, int64 size) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " size = " << size;
-  DownloadMap::iterator it = in_progress_.find(download_id);
-  if (it == in_progress_.end()) {
-    // The download is done, but the user hasn't selected a final location for
-    // it yet (the Save As dialog box is probably still showing), so just keep
-    // track of the fact that this download id is complete, when the
-    // DownloadItem is constructed later we'll notify its completion then.
-    DCHECK(!ContainsKey(pending_finished_downloads_, download_id));
-    pending_finished_downloads_[download_id] = size;
-    VLOG(20) << __FUNCTION__ << "()" << " Added download_id = " << download_id
-             << " to pending_finished_downloads_";
-    return;
-  }
 
-  // Remove the id from the list of pending ids.
-  PendingFinishedMap::iterator erase_it =
-      pending_finished_downloads_.find(download_id);
-  if (erase_it != pending_finished_downloads_.end()) {
-    pending_finished_downloads_.erase(erase_it);
-    VLOG(20) << __FUNCTION__ << "()" << " Removed download_id = " << download_id
-             << " from pending_finished_downloads_";
-  }
+  DCHECK_EQ(1U, active_downloads_.count(download_id));
+  DownloadItem* download = active_downloads_[download_id];
+  download->OnAllDataSaved(size);
 
-  DownloadItem* download = it->second;
+  MaybeCompleteDownload(download);
+}
 
-  VLOG(20) << __FUNCTION__ << "()"
-           << " download = " << download->DebugString(true);
+bool DownloadManager::IsDownloadReadyForCompletion(DownloadItem* download) {
+  // If we don't have all the data, the download is not ready for
+  // completion.
+  if (!download->all_data_saved())
+    return false;
 
-  download->OnAllDataSaved(size);
+  // If the download isn't active (e.g. has been cancelled) it's not
+  // ready for completion.
+  if (active_downloads_.count(download->id()) == 0)
+    return false;
 
-  // Clean up will happen when the history system create callback runs if we
-  // don't have a valid db_handle yet.
-  if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
-    in_progress_.erase(it);
-    download_history_->UpdateEntry(download);
-  }
+  // If the download hasn't been inserted into the history system
+  // (which occurs strictly after file name determination, intermediate
+  // file rename, and UI display) then it's not ready for completion.
+  return (download->db_handle() != DownloadHistory::kUninitializedHandle);
+}
 
-  UpdateAppIcon();
+void DownloadManager::MaybeCompleteDownload(DownloadItem* download) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << "()" << " download = "
+           << download->DebugString(false);
 
-  // If this a dangerous download not yet validated by the user, don't do
-  // anything. When the user notifies us, it will trigger a call to
-  // ProceedWithFinishedDangerousDownload.
-  if (download->safety_state() == DownloadItem::DANGEROUS) {
+  if (!IsDownloadReadyForCompletion(download))
     return;
-  }
 
-  if (download->safety_state() == DownloadItem::DANGEROUS_BUT_VALIDATED) {
-    // We first need to rename the downloaded file from its temporary name to
-    // its final name before we can continue.
-    BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(
-            this, &DownloadManager::ProceedWithFinishedDangerousDownload,
-            download->db_handle(),
-            download->full_path(), download->target_name()));
-    return;
+  // TODO(rdsmith): DCHECK that we only pass through this point
+  // once per download.  The natural way to do this is by a state
+  // transition on the DownloadItem.
+
+  // Confirm we're in the proper set of states to be here;
+  // in in_progress_, have all data, have a history handle.
+  DCHECK_EQ(1u, in_progress_.count(download->id()));
+  DCHECK(download->all_data_saved());
+  DCHECK(download->db_handle() != DownloadHistory::kUninitializedHandle);
+  DCHECK_EQ(1u, history_downloads_.count(download->db_handle()));
+
+  VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
+           << download->DebugString(false);
+
+  // Remove the id from in_progress
+  in_progress_.erase(download->id());
+  UpdateAppIcon();  // Reflect removal from in_progress_.
+
+  // Final update of download item and history.
+  download->MarkAsComplete();
+  download_history_->UpdateEntry(download);
+
+  switch (download->safety_state()) {
+    case DownloadItem::DANGEROUS:
+      // If this a dangerous download not yet validated by the user, don't do
+      // anything. When the user notifies us, it will trigger a call to
+      // ProceedWithFinishedDangerousDownload.
+      return;
+    case DownloadItem::DANGEROUS_BUT_VALIDATED:
+      // The dangerous download has been validated by the user.  We first
+      // need to rename the downloaded file from its temporary name to
+      // its final name.  We will continue the download processing in the
+      // callback.
+      BrowserThread::PostTask(
+          BrowserThread::FILE, FROM_HERE,
+          NewRunnableMethod(
+              this, &DownloadManager::ProceedWithFinishedDangerousDownload,
+              download->db_handle(),
+              download->full_path(), download->target_name()));
+      return;
+    case DownloadItem::SAFE:
+      // The download is safe; just finish it.
+      download->OnSafeDownloadFinished(file_manager_);
+      return;
   }
-
-  download->OnSafeDownloadFinished(file_manager_);
 }
 
 void DownloadManager::RemoveFromActiveList(int32 download_id) {
@@ -667,13 +676,13 @@ void DownloadManager::DownloadCancelled(int32 download_id) {
   if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
     in_progress_.erase(it);
     active_downloads_.erase(download_id);
+    UpdateAppIcon();  // Reflect removal from in_progress_.
     download_history_->UpdateEntry(download);
   }
 
   DownloadCancelledInternal(download_id,
                             download->render_process_id(),
                             download->request_id());
-  UpdateAppIcon();
 }
 
 void DownloadManager::DownloadCancelledInternal(int download_id,
@@ -993,23 +1002,21 @@ void DownloadManager::OnCreateDownloadEntryComplete(
   // Inform interested objects about the new download.
   NotifyModelChanged();
 
-  // If this download has been completed before we've received the db handle,
+  // If this download has been cancelled before we've received the DB handle,
   // post one final message to the history service so that it can be properly
   // in sync with the DownloadItem's completion status, and also inform any
   // observers so that they get more than just the start notification.
+  //
+  // Otherwise, try to complete the download
   if (download->state() != DownloadItem::IN_PROGRESS) {
+    DCHECK_EQ(DownloadItem::CANCELLED, download->state());
     in_progress_.erase(it);
-    // TODO(ahendrickson) -- We don't actually know whether or not we can
-    // remove the download item from the |active_downloads_| map, as there
-    // is no state in |DownloadItem::DownloadState| to indicate that the
-    // downloads system is done with an item.  Fix this when we have a
-    // proper final state to check for.
     active_downloads_.erase(info.download_id);
     download_history_->UpdateEntry(download);
     download->UpdateObservers();
+  } else {
+    MaybeCompleteDownload(download);
   }
-
-  UpdateAppIcon();
 }
 
 void DownloadManager::ShowDownloadInBrowser(const DownloadCreateInfo& info,
@@ -1057,9 +1064,9 @@ DownloadItem* DownloadManager::GetDownloadItem(int id) {
 void DownloadManager::AssertContainersConsistent() const {
 #if !defined(NDEBUG)
   // Turn everything into sets.
-  DownloadSet in_progress_set, history_set;
+  DownloadSet active_set, history_set;
   const DownloadMap* input_maps[] = {&active_downloads_, &history_downloads_};
-  DownloadSet* local_sets[] = {&in_progress_set, &history_set};
+  DownloadSet* local_sets[] = {&active_set, &history_set};
   DCHECK_EQ(ARRAYSIZE_UNSAFE(input_maps), ARRAYSIZE_UNSAFE(local_sets));
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(input_maps); i++) {
     for (DownloadMap::const_iterator it = input_maps[i]->begin();
@@ -1069,7 +1076,7 @@ void DownloadManager::AssertContainersConsistent() const {
   }
 
   // Check if each set is fully present in downloads, and create a union.
-  const DownloadSet* all_sets[] = {&in_progress_set, &history_set,
+  const DownloadSet* all_sets[] = {&active_set, &history_set,
                                    &save_page_as_downloads_};
   DownloadSet downloads_union;
   for (int i = 0; i < static_cast<int>(ARRAYSIZE_UNSAFE(all_sets)); i++) {
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 61b17be..c00437a 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -125,6 +125,17 @@ class DownloadManager
   void PauseDownload(int32 download_id, bool pause);
   void RemoveDownload(int64 download_handle);
 
+  // Determine if the download is ready for completion, i.e. has had
+  // all data received, and completed the filename determination and
+  // history insertion.
+  bool IsDownloadReadyForCompletion(DownloadItem* download);
+
+  // If all pre-requisites have been met, complete download processing, i.e.
+  // do internal cleanup, file rename, and potentially auto-open.
+  // (Dangerous downloads still may block on user acceptance after this
+  // point.)
+  void MaybeCompleteDownload(DownloadItem* download);
+
   // Called when the download is renamed to its final name.
   void DownloadRenamedToFinalName(int download_id, const FilePath& full_path);
 
@@ -369,14 +380,6 @@ class DownloadManager
   // user wants us to prompt for a save location for each download.
   FilePath last_download_path_;
 
-  // Keep track of downloads that are completed before the user selects the
-  // destination, so that observers are appropriately notified of completion
-  // after this determination is made.
-  // The map is of download_id->remaining size (bytes), both of which are
-  // required when calling OnAllDataSaved.
-  typedef std::map<int32, int64> PendingFinishedMap;
-  PendingFinishedMap pending_finished_downloads_;
-
   // The "Save As" dialog box used to ask the user where a file should be
   // saved.
   scoped_refptr<SelectFileDialog> select_file_dialog_;
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index 610fb98..4b7dd8a 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -157,28 +157,29 @@ const struct {
   bool will_delete_crdownload;
   int expected_rename_count;
 } kDownloadRenameCases[] = {
-  // Safe download, download finishes BEFORE rename.
-  // Needs to be renamed only once.  Crdownload file needs to be deleted.
+  // Safe download, download finishes BEFORE file name determined.
+  // Renamed twice (linear path through UI).  Crdownload file does not need
+  // to be deleted.
   { FILE_PATH_LITERAL("foo.zip"),
     false,
     true,
-    true,
-    1, },
-  // Dangerous download, download finishes BEFORE rename.
+    false,
+    2, },
+  // Dangerous download, download finishes BEFORE file name determined.
   // Needs to be renamed only once.
   { FILE_PATH_LITERAL("Unconfirmed xxx.crdownload"),
     true,
     true,
     false,
     1, },
-  // Safe download, download finishes AFTER rename.
+  // Safe download, download finishes AFTER file name determined.
   // Needs to be renamed twice.
   { FILE_PATH_LITERAL("foo.zip"),
     false,
     false,
     false,
     2, },
-  // Dangerous download, download finishes AFTER rename.
+  // Dangerous download, download finishes AFTER file name determined.
   // Needs to be renamed only once.
   { FILE_PATH_LITERAL("Unconfirmed xxx.crdownload"),
     true,
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index f7833d5..e2cebcc 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -687,8 +687,9 @@ void SavePackage::Finish() {
                         save_ids));
 
   download_->OnAllDataSaved(all_save_items_count_);
+  download_->MarkAsComplete();
   // Notify download observers that we are complete (the call
-  // to OnAllDataSaved() set the state to complete but did not notify).
+  // to OnReadyToFinish() set the state to complete but did not notify).
   download_->UpdateObservers();
 
   NotificationService::current()->Notify(