Support for adding save page download items into downloads history.

content/browser/download/save_package.cc

 // Copyright (c) 2011 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 "content/browser/download/save_package.h"
 
 #include <algorithm>
 
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/i18n/file_util_icu.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "base/string_piece.h"
 #include "base/string_split.h"
 #include "base/sys_string_conversions.h"
 #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"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/prefs/pref_member.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/content_browser_client.h"
@@ skipping 13 matching lines @@
 #include "net/base/mime_util.h"
 #include "net/base/net_util.h"
 #include "net/url_request/url_request_context.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebPageSerializerClient.h"
 
 using base::Time;
 using WebKit::WebPageSerializerClient;
 
 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;
 
 // Default name which will be used when we can not get proper name from
 // resource URL.
 const char kDefaultSaveName[] = "saved_resource";
 
 // Maximum number of file ordinal number. I think it's big enough for resolving
 // name-conflict files which has same base file name.
 const int32 kMaxFileOrdinalNumber = 9999;
@@ skipping 49 matching lines @@
 #else
     FILE_PATH_LITERAL("html");
 #endif
 
 SavePackage::SavePackage(TabContents* tab_contents,
                          SavePackageType save_type,
                          const FilePath& file_full_path,
                          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),
       saved_main_directory_path_(directory_full_path),
       title_(tab_contents->GetTitle()),
       finished_(false),
       user_canceled_(false),
       disk_error_occurred_(false),
       save_type_(save_type),
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(tab_contents->GetRenderProcessHost()->id()),
       unique_id_(g_save_package_id++),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(page_url_.is_valid());
   DCHECK(save_type_ == SAVE_AS_ONLY_HTML ||
          save_type_ == SAVE_AS_COMPLETE_HTML);
   DCHECK(!saved_main_file_path_.empty() &&
          saved_main_file_path_.value().length() <= kMaxFilePathLength);
   DCHECK(!saved_main_directory_path_.empty() &&
          saved_main_directory_path_.value().length() < kMaxFilePathLength);
   InternalInit();
 }
 
 SavePackage::SavePackage(TabContents* tab_contents)
     : TabContentsObserver(tab_contents),
       file_manager_(NULL),
+      download_manager_(NULL),
       download_(NULL),
       page_url_(GetUrlToBeSaved()),
       title_(tab_contents->GetTitle()),
       finished_(false),
       user_canceled_(false),
       disk_error_occurred_(false),
       save_type_(SAVE_TYPE_UNKNOWN),
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(tab_contents->GetRenderProcessHost()->id()),
       unique_id_(g_save_package_id++),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(page_url_.is_valid());
   InternalInit();
 }
 
 // This is for testing use. Set |finished_| as true because we don't want
 // method Cancel to be be called in destructor in test mode.
 // We also don't call InternalInit().
 SavePackage::SavePackage(TabContents* tab_contents,
                          const FilePath& file_full_path,
                          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),
       finished_(true),
       user_canceled_(false),
       disk_error_occurred_(false),
       save_type_(SAVE_TYPE_UNKNOWN),
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(0),
       unique_id_(g_save_package_id++),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
 }
 
 SavePackage::~SavePackage() {
   // Stop receiving saving job's updates
   if (!finished_ && !canceled()) {
     // Unexpected quit.
     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()));
   // Free all SaveItems.
   while (!waiting_item_queue_.empty()) {
     // We still have some items which are waiting for start to save.
     SaveItem* save_item = waiting_item_queue_.front();
     waiting_item_queue_.pop();
     delete save_item;
   }
 
   STLDeleteValues(&saved_success_items_);
   STLDeleteValues(&in_progress_items_);
   STLDeleteValues(&saved_failed_items_);
 
-  // The DownloadItem is owned by DownloadManager.
-  download_ = NULL;
-
   file_manager_ = NULL;
 }
 
 GURL SavePackage::GetUrlToBeSaved() {
   // Instead of using tab_contents_.GetURL here, we use url()
   // (which is the "real" url of the page)
   // from the NavigationEntry because it reflects its' origin
   // rather than the displayed one (returned by GetURL) which may be
   // different (like having "view-source:" on the front).
   NavigationEntry* active_entry =
@@ skipping 35 matching lines @@
 
   wait_state_ = START_PROCESS;
 
   // Initialize the request context and resource dispatcher.
   Profile* profile = tab_contents()->profile();
   if (!profile) {
     NOTREACHED();
     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_);
-
-  tab_contents()->OnStartDownload(download_);
+  // Transfer ownership to the download manager.
+  download_manager_->SavePageDownloadStarted(download_);
 
   // Check save type and process the save page job.
   if (save_type_ == SAVE_AS_COMPLETE_HTML) {
     // Get directory
     DCHECK(!saved_main_directory_path_.empty());
     GetAllSavableResourceLinksForCurrentPage();
   } else {
     wait_state_ = NET_FILES;
     SaveFileCreateInfo::SaveFileSource save_source = page_url_.SchemeIsFile() ?
         SaveFileCreateInfo::SAVE_FILE_FROM_FILE :
@@ skipping 341 matching lines @@
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(file_manager_,
                         &SaveFileManager::RemoveSavedFileFromFileMap,
                         save_ids));
 
   finished_ = true;
   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() {
   if (in_process_count() || finished_)
     return;
 
   FilePath dir = (save_type_ == SAVE_AS_COMPLETE_HTML &&
                   saved_success_items_.size() > 1) ?
                   saved_main_directory_path_ : FilePath();
 
@@ skipping 32 matching lines @@
   for (SaveUrlItemMap::iterator it = saved_failed_items_.begin();
        it != saved_failed_items_.end(); ++it)
     save_ids.push_back(it->second->save_id());
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(file_manager_,
                         &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,
       Source<SavePackage>(this),
       Details<GURL>(&page_url_));
 }
 
 // Called for updating end state.
 void SavePackage::SaveFinished(int32 save_id, int64 size, bool is_success) {
   // Because we might have canceled this saving job before,
   // so we might not find corresponding SaveItem. Just ignore it.
   SaveItem* save_item = LookupItemInProcessBySaveId(save_id);
   if (!save_item)
     return;
 
   // Let SaveItem set end state.
   save_item->Finish(size, is_success);
   // Remove the associated save id and SavePackage.
   file_manager_->RemoveSaveFile(save_id, save_item->url(), this);
 
   PutInProgressItemToSavedMap(save_item);
 
   // 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()) {
     // If size of main HTML page is 0, treat it as disk error.
     Cancel(false);
     return;
   }
 
   if (canceled()) {
     DCHECK(finished_);
@@ skipping 20 matching lines @@
     return;
   }
   SaveItem* save_item = it->second;
 
   save_item->Finish(0, false);
 
   PutInProgressItemToSavedMap(save_item);
 
   // 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) {
     // We got error when saving page. Treat it as disk error.
     Cancel(true);
   }
 
   if (canceled()) {
     DCHECK(finished_);
     return;
@@ skipping 243 matching lines @@
     const std::vector<GURL>& referrers_list,
     const std::vector<GURL>& frames_list) {
   if (wait_state_ != RESOURCES_LIST)
     return;
 
   DCHECK(resources_list.size() == referrers_list.size());
   all_save_items_count_ = static_cast<int>(resources_list.size()) +
                            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.
     for (int i = 0; i < static_cast<int>(resources_list.size()); ++i) {
       const GURL& u = resources_list[i];
       DCHECK(u.is_valid());
       SaveFileCreateInfo::SaveFileSource save_source = u.SchemeIsFile() ?
           SaveFileCreateInfo::SAVE_FILE_FROM_FILE :
           SaveFileCreateInfo::SAVE_FILE_FROM_NET;
       SaveItem* save_item = new SaveItem(u, referrers_list[i],
@@ skipping 267 matching lines @@
 bool SavePackage::IsSavableContents(const std::string& contents_mime_type) {
   // WebKit creates Document object when MIME type is application/xhtml+xml,
   // so we also support this MIME type.
   return contents_mime_type == "text/html" ||
          contents_mime_type == "text/xml" ||
          contents_mime_type == "application/xhtml+xml" ||
          contents_mime_type == "text/plain" ||
          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();
+}