Replace the DownloadFileManager with direct ownership of DownloadFile.

content/browser/download/download_item_impl.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 "content/browser/download/download_item_impl.h"
 
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/i18n/case_conversion.h"
 #include "base/i18n/string_search.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file.h"
-#include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_item_impl_delegate.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_persistent_store_info.h"
 #include "net/base/net_util.h"
 
@@ skipping 79 matching lines @@
     return NULL;
   }
   virtual void PauseRequest() const OVERRIDE {}
   virtual void ResumeRequest() const OVERRIDE {}
   virtual void CancelRequest() const OVERRIDE {}
   virtual std::string DebugString() const OVERRIDE {
     return "Null DownloadRequestHandle";
   }
 };
 
+// Wrapper around DownloadFile::Detach and DownloadFile::Cancel that
+// takes ownership of the DownloadFile and hence implicitly destroys it
+// at the end of the function.

[benjhayden, 2012/08/16 20:35:20]

Please also warn against copying callbacks containing base::Passed.

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

I apologize, but I think I'm going to take your out about ignoring the comment. 
The problem is that a warning isn't appropriate here (while these functions are
used in constructing callbacks, there's no hint of callback magic here) and
isn't really appropriate at point of use (in that the callbacks there are being
passed directly to PostTask and hence shouldn't need the warning).  I sorta want
a warning in base/callback.h, but the author didn't choose to put one in, and
there's already one in Will's writeup on constructs in chrome.  So I'm not sure
there's a good place to add it.

+static void DownloadFileDetach(scoped_ptr<DownloadFile> download_file) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  download_file->Detach();
+}
+
+static void DownloadFileCancel(scoped_ptr<DownloadFile> download_file) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  download_file->Cancel();
+}
+
 }  // namespace
 
 namespace content {
 
 // Our download table ID starts at 1, so we use 0 to represent a download that
 // has started, but has not yet had its data persisted in the table. We use fake
 // database handles in incognito mode starting at -1 and progressively getting
 // more negative.
 // static
 const int DownloadItem::kUninitializedHandle = 0;
@@ skipping 153 matching lines @@
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   delegate_->Attach();
   Init(true /* actively downloading */,
        download_net_logs::SRC_SAVE_PAGE_AS);
 }
 
 DownloadItemImpl::~DownloadItemImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // Should always have been nuked before now, at worst in
+  // DownloadManager shutdown.
+  DCHECK(!download_file_.get());
+
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
   delegate_->AssertStateConsistent(this);
   delegate_->Detach();
 }
 
+base::WeakPtr<content::DownloadDestinationObserver>
+DownloadItemImpl::DestinationObserverAsWeakPtr() {
+  // Return does private downcast.
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 void DownloadItemImpl::AddObserver(Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   observers_.AddObserver(observer);
 }
 
 void DownloadItemImpl::RemoveObserver(Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   observers_.RemoveObserver(observer);
@@ skipping 68 matching lines @@
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_ITEM_SAFETY_STATE_UPDATED,
       base::Bind(&download_net_logs::ItemCheckedCallback,
                  GetDangerType(), GetSafetyState()));
 
   UpdateObservers();
 
   delegate_->MaybeCompleteDownload(this);
 }
 
-void DownloadItemImpl::ProgressComplete(int64 bytes_so_far,
-                                        const std::string& final_hash) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  hash_ = final_hash;
-  hash_state_ = "";
-
-  received_bytes_ = bytes_so_far;
-
-  // If we've received more data than we were expecting (bad server info?),
-  // revert to 'unknown size mode'.
-  if (received_bytes_ > total_bytes_)
-    total_bytes_ = 0;
-}
-
 // Updates from the download thread may have been posted while this download
 // was being cancelled in the UI thread, so we'll accept them unless we're
 // complete.
 void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
                                       int64 bytes_per_sec,
                                       const std::string& hash_state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!IsInProgress()) {
     // Ignore if we're no longer in-progress.  This can happen if we race a
@@ skipping 36 matching lines @@
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   if (!IsPartialDownload()) {
     // Small downloads might be complete before this method has
     // a chance to run.
     return;
   }
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
   TransitionTo(CANCELLED);
+
+  // Cancel and remove the download file.
+  DCHECK(download_file_.get());
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      // Will be deleted at end of task execution.
+      base::Bind(&DownloadFileCancel, base::Passed(download_file_.Pass())));
+
+  // Cancel the originating URL request.
+  request_handle_->CancelRequest();
+
   if (user_cancel)
     delegate_->DownloadStopped(this);
 }
 
+// We're starting the download.
+void DownloadItemImpl::Start(scoped_ptr<content::DownloadFile> download_file) {
+  DCHECK(!download_file_.get());
+  download_file_ = download_file.Pass();
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFile::Initialize,
+                 // Safe because we control download file lifetime.
+                 base::Unretained(download_file_.get()),
+                 base::Bind(&DownloadItemImpl::OnDownloadFileInitialized,
+                            weak_ptr_factory_.GetWeakPtr())));
+}
+
 // An error occurred somewhere.
 void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
   // Somewhat counter-intuitively, it is possible for us to receive an
   // interrupt after we've already been interrupted.  The generation of
   // interrupts from the file thread Renames and the generation of
   // interrupts from disk writes go through two different mechanisms (driven
   // by rename requests from UI thread and by write requests from IO thread,
   // respectively), and since we choose not to keep state on the File thread,
   // this is the place where the races collide.  It's also possible for
   // interrupts to race with cancels.
 
   // Whatever happens, the first one to hit the UI thread wins.
   if (!IsInProgress())
     return;
 
   last_reason_ = reason;
   TransitionTo(INTERRUPTED);
+
+  // Cancel and remove the download file.
+  DCHECK(download_file_.get());
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      // Will be deleted at end of task execution.
+      base::Bind(&DownloadFileCancel, base::Passed(download_file_.Pass())));
+
+  // Cancel the originating URL request.
+  request_handle_->CancelRequest();
+
   download_stats::RecordDownloadInterrupted(
       reason, received_bytes_, total_bytes_);
   delegate_->DownloadStopped(this);
 }
 
 void DownloadItemImpl::MarkAsComplete() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
   TransitionTo(COMPLETE);
 }
 
 void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) {
   auto_opened_ = auto_opened;
   Completed();
 }
 
 void DownloadItemImpl::OnAllDataSaved(
-    int64 size, const std::string& final_hash) {
+    const std::string& final_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  DCHECK_EQ(IN_PROGRESS, state_);
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
-  ProgressComplete(size, final_hash);
+
+  // Store final hash and null out intermediate serialized hash state.
+  hash_ = final_hash;
+  hash_state_ = "";
+
   UpdateObservers();
 }
 
 void DownloadItemImpl::OnDownloadedFileRemoved() {
   file_externally_removed_ = true;
   UpdateObservers();
 }
 
 void DownloadItemImpl::MaybeCompleteDownload() {
   // TODO(rdsmith): Move logic for this function here.
@@ skipping 174 matching lines @@
     request_handle_->ResumeRequest();
   else
     request_handle_->PauseRequest();
   is_paused_ = !is_paused_;
   UpdateObservers();
 }
 
 void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (!IsInProgress())
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
   DCHECK(!GetTargetName().empty());
   DCHECK_NE(DANGEROUS, GetSafetyState());
 
+  DCHECK(download_file_.get());
   if (NeedsRename()) {
-    DownloadFileManager::RenameCompletionCallback callback =
+    content::DownloadFile::RenameCompletionCallback callback =
         base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
                    weak_ptr_factory_.GetWeakPtr());
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileManager::RenameDownloadFile,
-                   delegate_->GetDownloadFileManager(), GetGlobalId(),
+        base::Bind(&DownloadFile::Rename,
+                   base::Unretained(download_file_.get()),
                    GetTargetFilePath(), true, callback));
   } else {
     // Complete the download and release the DownloadFile.
-    BrowserThread::PostTask(
+    BrowserThread::PostTaskAndReply(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileManager::CompleteDownload,
-                   delegate_->GetDownloadFileManager(), GetGlobalId(),
-                   base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
-                              weak_ptr_factory_.GetWeakPtr())));
+        base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass())),
+        base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(
     content::DownloadInterruptReason reason,
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (!IsInProgress())
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
            << " needed rename = " << NeedsRename()
            << " " << DebugString(false);
   DCHECK(NeedsRename());
 
   if (content::DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
     return;
   }
 
   // full_path is now the current and target file path.
   DCHECK(!full_path.empty());
   target_path_ = full_path;
   SetFullPath(full_path);
   delegate_->DownloadRenamedToFinalName(this);
 
   // Complete the download and release the DownloadFile.
-  BrowserThread::PostTask(
+  // TODO(rdsmith): Unify this path with the !NeedsRename() path in
+  // OnDownloadCompleting above.  This can happen easily after history
+  // is made into an observer and the path accessors are cleaned up;
+  // that should allow OnDownloadCompleting to simply call
+  // OnDownloadRenamedToFinalName directly.
+  DCHECK(download_file_.get());
+  BrowserThread::PostTaskAndReply(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CompleteDownload,
-                 delegate_->GetDownloadFileManager(), GetGlobalId(),
-                 base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
-                            weak_ptr_factory_.GetWeakPtr())));
+      base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass())),
+      base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DownloadItemImpl::OnDownloadFileInitialized(
+    content::DownloadInterruptReason result) {
+  if (result != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
+    Interrupt(result);
+    // TODO(rdsmith): It makes no sense to continue along the
+    // regular download path after we've gotten an error.  But it's
+    // the way the code has historically worked, and this allows us
+    // to get the download persisted and observers of the download manager
+    // notified, so tests work.  When we execute all side effects of cancel
+    // (including queue removal) immedately rather than waiting for
+    // persistence we should replace this comment with a "return;".
+  }
+
+  delegate_->DelegateStart(this);
 }
 
 void DownloadItemImpl::OnDownloadFileReleased() {
   if (delegate_->ShouldOpenDownload(this))
     Completed();
   else
     delegate_delayed_complete_ = true;
 }
 
 void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
@@ skipping 110 matching lines @@
   target_path_ = target_path;
   target_disposition_ = disposition;
   SetDangerType(danger_type);
   // TODO(asanka): SetDangerType() doesn't need to send a notification here.
 
   // We want the intermediate and target paths to refer to the same directory so
   // that they are both on the same device and subject to same
   // space/permission/availability constraints.
   DCHECK(intermediate_path.DirName() == target_path.DirName());
 
+  if (!IsInProgress()) {
+    // If we've been cancelled or interrupted while the target was being
+    // determined, continue the cascade with a null name.
+    // The error doesn't matter as the cause of download stoppaged
+    // will already have been recorded.
+    OnDownloadRenamedToIntermediateName(
+        content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED, FilePath());
+    return;
+  }
+
   // Rename to intermediate name.
   // TODO(asanka): Skip this rename if AllDataSaved() is true. This avoids a
   //               spurious rename when we can just rename to the final
   //               filename. Unnecessary renames may cause bugs like
   //               http://crbug.com/74187.
-  DownloadFileManager::RenameCompletionCallback callback =
+  DCHECK(download_file_.get());
+  DownloadFile::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::RenameDownloadFile,
-                 delegate_->GetDownloadFileManager(), GetGlobalId(),
+      base::Bind(&DownloadFile::Rename,
+                 // Safe because we control download file lifetime.
+                 base::Unretained(download_file_.get()),
                  intermediate_path, false, callback));
 }
 
 void DownloadItemImpl::OnContentCheckCompleted(
     content::DownloadDangerType danger_type) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(AllDataSaved());
   SetDangerType(danger_type);
   UpdateObservers();
 }
@@ skipping 10 matching lines @@
 
 void DownloadItemImpl::SetDisplayName(const FilePath& name) {
   display_name_ = name;
 }
 
 FilePath DownloadItemImpl::GetUserVerifiedFilePath() const {
   return (safety_state_ == DownloadItem::SAFE) ?
       GetTargetFilePath() : GetFullPath();
 }
 
-void DownloadItemImpl::OffThreadCancel() {
+void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
+                                         int64 bytes_per_sec,
+                                         const std::string& hash_state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  request_handle_->CancelRequest();
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CancelDownload,
-                 delegate_->GetDownloadFileManager(), download_id_));
+  if (!IsInProgress()) {
+    // Ignore if we're no longer in-progress.  This can happen if we race a
+    // Cancel on the UI thread with an update on the FILE thread.
+    //
+    // TODO(rdsmith): Arguably we should let this go through, as this means
+    // the download really did get further than we know before it was
+    // cancelled.  But the gain isn't very large, and the code is more
+    // fragile if it has to support in progress updates in a non-in-progress
+    // state.  This issue should be readdressed when we revamp performance
+    // reporting.
+    return;
+  }
+  bytes_per_sec_ = bytes_per_sec;
+  hash_state_ = hash_state;
+  received_bytes_ = bytes_so_far;
+
+  // If we've received more data than we were expecting (bad server info?),
+  // revert to 'unknown size mode'.
+  if (received_bytes_ > total_bytes_)
+    total_bytes_ = 0;
+
+  if (bound_net_log_.IsLoggingAllEvents()) {
+    bound_net_log_.AddEvent(
+        net::NetLog::TYPE_DOWNLOAD_ITEM_UPDATED,
+        net::NetLog::Int64Callback("bytes_so_far", received_bytes_));
+  }
+
+  UpdateObservers();
+}
+
+void DownloadItemImpl::DestinationError(
+    content::DownloadInterruptReason reason) {
+  // The DestinationError and Interrupt routines are being kept separate
+  // to allow for a future merging of the Cancel and Interrupt routines..
+  Interrupt(reason);
+}
+
+void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
+  if (!IsInProgress())
+    return;
+  OnAllDataSaved(final_hash);
+  delegate_->MaybeCompleteDownload(this);
 }
 
 void DownloadItemImpl::Init(bool active,
                             download_net_logs::DownloadType download_type) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (active)
     download_stats::RecordDownloadCount(download_stats::START_COUNT);
 
   if (target_path_.empty())
@@ skipping 89 matching lines @@
         " db_handle = %" PRId64
         " total = %" PRId64
         " received = %" PRId64
         " reason = %s"
         " paused = %c"
         " safety = %s"
         " last_modified = '%s'"
         " etag = '%s'"
         " url_chain = \n\t\"%s\"\n\t"
         " full_path = \"%" PRFilePath "\""
-        " target_path = \"%" PRFilePath "\"",
+        " target_path = \"%" PRFilePath "\""
+        " has download file = %s",
         GetDbHandle(),
         GetTotalBytes(),
         GetReceivedBytes(),
         InterruptReasonDebugString(last_reason_).c_str(),
         IsPaused() ? 'T' : 'F',
         DebugSafetyStateString(GetSafetyState()),
         GetLastModifiedTime().c_str(),
         GetETag().c_str(),
         url_list.c_str(),
         GetFullPath().value().c_str(),
-        GetTargetFilePath().value().c_str());
+        GetTargetFilePath().value().c_str(),
+        download_file_.get() ? "true" : "false");
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }
 
   description += " }";
 
   return description;
 }
 
 bool DownloadItemImpl::AllDataSaved() const { return all_data_saved_; }
@@ skipping 89 matching lines @@
 void DownloadItemImpl::SetOpened(bool opened) { opened_ = opened; }
 bool DownloadItemImpl::GetOpened() const { return opened_; }
 const std::string& DownloadItemImpl::GetLastModifiedTime() const {
   return last_modified_time_;
 }
 const std::string& DownloadItemImpl::GetETag() const { return etag_; }
 content::DownloadInterruptReason DownloadItemImpl::GetLastReason() const {
   return last_reason_;
 }
 void DownloadItemImpl::MockDownloadOpenForTesting() { open_enabled_ = false; }