Support for adding save page download items into downloads history.

content/browser/download/save_package.cc

Index: content/browser/download/save_package.cc
===================================================================
--- content/browser/download/save_package.cc	(revision 94123)
+++ content/browser/download/save_package.cc	(working copy)
@@ -18,6 +18,7 @@
 #include "base/task.h"
 #include "base/threading/thread.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_item.h"
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_manager.h"
@@ -51,6 +52,46 @@
 
 namespace {
 
+/*
+Race analysis:

[Paweł Hajdan Jr., 2011/07/27 16:53:51]

I'm not sure about adding this as a comment in the code, it is likely to get out
of sync with reality.

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

Yeah.  Another take on this is that if we need this kind of analysis in order to
reassure yourself that there aren't races in our code, we're writing fragile
code.  I'm just not seeing a better way to handle this issue short of
integrating with the download system :-{.

Achuith: I think you can summarize my analysis below in substantially fewer
words.  Something like:
* The history callback will call OnSavePageDownloadEntryAdded and either
completion or canceling of the save page will call
SavePageDownloadFinished()--those two functions are written to correctly manage
the download item whichever order they occur in.
* If something removes the download item from the download manager (Remove,
Shutdown) the result will be that the SavePage system will not be able to
properly update the download item (which no longer exists) or the download
history, but the action will complete properly anyway.  This may lead to the
history entry being wrong on a reload of chrome (specifically in the case of
Initiation -> History Callback -> Removal -> Completion), but there's no way to
solve that without canceling on Remove (which would then update the DB).
------------

I'm also realizing looking at this writeup, that it's really out of context
here.  It's really about the events sent to the DownloadManager by the
SavePackage system and the history callback.  As such, the comment should be
near those functions in DownloadManager.  Could you also move it there?

Pawel, what do you think of that set of modifications?

[achuithb, 2011/07/28 00:45:58]

I've moved the comments to the end of download_manager.cc where the SavePackage
methods lie, and abbreviated them. PTAL. I agree that these comments about the
races are necessary since these are treacherous waters and the code looks
deceptively simple.

++ The actions that can race against each other are:
+  * History callback
+  * Completion (save page as finishes)
+  * Cancel (SavePackage::Stop())
+  * Shutdown
+  * Remove
++ Cancel and completion can be considered equivalent for race
+  processing, as they have the same effect on the download manager, and
+  SavePackage won't produce any more events after either of them.  I'll
+  refer to the OR of these two events as "Finishing".
++ Anything that occurs after both History callback and Finishing is
+  uninteresting, as there won't be anything in the queues anymore.
++ Only the History Callback or Remove can occur after Shutdown (the
+  SavePackage system won't produce any more events after these two).
++ Remove does not cancel (probably not a mistake you'd make, but it
+  does on downloads, so I'm noting it for myself).
++ Shutdown can only occur after Finishing(Cancel) (destroying a
+  profile happens strictly after destroying all TabContents associated
+  with the profile).
++ Remove can only occur after the History Callback.
+
+This leads to the following possible orders of events:
+-- Finishing, History Callback
+   Variations on the basic one.  Ok.
+-- Finishing(Cancel), Shutdown, History Callback
+   The cancel leaves the DI in the queue, shutdown removes and deletes
+   it, and the history callback doesn't find it, leaving it in the DB
+   as IN_PROGRESS, which will be turned into Cancelled on next load.
+   Ok.
+-- History Callback, Finishing
+   The basic order.  Ok.
+-- History Callback, Remove, Finishing
+   This results in a lack of update of the database to indicate
+   that the download has completed (if the finishing action is
+   completed), but that's not solvable until we cancel on Remove
+   (which would update the DB).
+*/
+
 // A counter for uniquely identifying each save package.
 int g_save_package_id = 0;
 
@@ -120,6 +161,7 @@
                          const FilePath& directory_full_path)
     : TabContentsObserver(tab_contents),
       file_manager_(NULL),
+      download_manager_(NULL),
       download_(NULL),
       page_url_(GetUrlToBeSaved()),
       saved_main_file_path_(file_full_path),
@@ -147,6 +189,7 @@
 SavePackage::SavePackage(TabContents* tab_contents)
     : TabContentsObserver(tab_contents),
       file_manager_(NULL),
+      download_manager_(NULL),
       download_(NULL),
       page_url_(GetUrlToBeSaved()),
       title_(tab_contents->GetTitle()),
@@ -171,6 +214,7 @@
                          const FilePath& directory_full_path)
     : TabContentsObserver(tab_contents),
       file_manager_(NULL),
+      download_manager_(NULL),
       download_(NULL),
       saved_main_file_path_(file_full_path),
       saved_main_directory_path_(directory_full_path),
@@ -192,6 +236,10 @@
     Cancel(true);
   }
 
+  // We should no longer be observing the DownloadManager at this point.
+  CHECK(!download_manager_);
+  CHECK(!download_);
+
   DCHECK(all_save_items_count_ == (waiting_item_queue_.size() +
                                    completed_count() +
                                    in_process_count()));
@@ -207,9 +255,6 @@
   STLDeleteValues(&in_progress_items_);
   STLDeleteValues(&saved_failed_items_);
 
-  // The DownloadItem is owned by DownloadManager.
-  download_ = NULL;
-
   file_manager_ = NULL;
 }
 
@@ -265,20 +310,23 @@
     return false;
   }
 
-  // Create the fake DownloadItem and display the view.
-  DownloadManager* download_manager =
-      tab_contents()->profile()->GetDownloadManager();
-  download_ = new DownloadItem(download_manager,
+  // Get the download manager and add ourselves as an observer.
+  download_manager_ = tab_contents()->profile()->GetDownloadManager();
+  if (!download_manager_) {
+    NOTREACHED();
+    return false;
+  }
+
+  // Create the download item.
+  download_ = new DownloadItem(download_manager_,
                                saved_main_file_path_,
                                page_url_,
                                profile->IsOffTheRecord());
+  download_->AddObserver(this);
 
-  // Transfer the ownership to the download manager. We need the DownloadItem
-  // to be alive as long as the Profile is alive.
-  download_manager->SavePageAsDownloadStarted(download_);
+  // Transfer ownership to the download manager.
+  download_manager_->SavePageDownloadStarted(download_);
 
-  tab_contents()->OnStartDownload(download_);
-
   // Check save type and process the save page job.
   if (save_type_ == SAVE_AS_COMPLETE_HTML) {
     // Get directory
@@ -639,7 +687,10 @@
   wait_state_ = FAILED;
 
   // Inform the DownloadItem we have canceled whole save page job.
-  download_->Cancel(false);
+  if (download_) {
+    download_->Cancel(false);
+    FinalizeDownloadEntry();
+  }
 }
 
 void SavePackage::CheckFinish() {
@@ -692,8 +743,11 @@
                         &SaveFileManager::RemoveSavedFileFromFileMap,
                         save_ids));
 
-  download_->OnAllDataSaved(all_save_items_count_);
-  download_->MarkAsComplete();
+  if (download_) {
+    download_->OnAllDataSaved(all_save_items_count_);
+    download_->MarkAsComplete();
+    FinalizeDownloadEntry();
+  }
 
   NotificationService::current()->Notify(
       content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
@@ -718,7 +772,8 @@
 
   // Inform the DownloadItem to update UI.
   // We use the received bytes as number of saved files.
-  download_->Update(completed_count());
+  if (download_)
+    download_->Update(completed_count());
 
   if (save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM &&
       save_item->url() == page_url_ && !save_item->received_bytes()) {
@@ -759,7 +814,8 @@
 
   // Inform the DownloadItem to update UI.
   // We use the received bytes as number of saved files.
-  download_->Update(completed_count());
+  if (download_)
+    download_->Update(completed_count());
 
   if (save_type_ == SAVE_AS_ONLY_HTML ||
       save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM) {
@@ -1023,7 +1079,8 @@
                            static_cast<int>(frames_list.size());
 
   // We use total bytes as the total number of files we want to save.
-  download_->set_total_bytes(all_save_items_count_);
+  if (download_)
+    download_->set_total_bytes(all_save_items_count_);
 
   if (all_save_items_count_) {
     // Put all sub-resources to wait list.
@@ -1311,3 +1368,30 @@
          contents_mime_type == "text/css" ||
          net::IsSupportedJavascriptMimeType(contents_mime_type.c_str());
 }
+
+void SavePackage::StopObservation() {
+  DCHECK(download_);
+  DCHECK(download_manager_);
+
+  download_->RemoveObserver(this);
+  download_ = NULL;
+  download_manager_ = NULL;
+}
+
+void SavePackage::OnDownloadUpdated(DownloadItem* download) {
+  DCHECK(download_);
+  DCHECK(download_ == download);
+  DCHECK(download_manager_);
+
+  // Check for removal.
+  if (download->state() == DownloadItem::REMOVING)
+    StopObservation();
+}
+
+void SavePackage::FinalizeDownloadEntry() {
+  DCHECK(download_);
+  DCHECK(download_manager_);
+
+  download_manager_->SavePageDownloadFinished(download_);
+  StopObservation();
+}