Replace the DownloadFileManager with direct ownership

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index a3e72ffa469e50f0efd18f92c63a50c2a99e0e16..c8a189baf76fc8a34518dc68d2d092098c9e8b47 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -19,7 +19,6 @@
 #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"
@@ -121,6 +120,17 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
   }
 };
 
+// Detach the specified download file, then invoke the callback on the
+// UI thread.  Note that this will also delete the DownloadFile object,
+// as the function accepts ownership and does not transfer it on.
+static void ReleaseDownloadFile(scoped_ptr<DownloadFile> download_file,
+                                const base::Closure& ui_callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  download_file->Detach();
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, ui_callback);
+}
+
 }  // namespace
 
 namespace content {
@@ -300,6 +310,10 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
 DownloadItemImpl::~DownloadItemImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // Should always have been nuked before now, at worst in
+  // DownloadManager shutdown.
+  DCHECK(!download_file_.get());
+
   TransitionTo(REMOVING);
   STLDeleteContainerPairSecondPointers(
       external_data_map_.begin(), external_data_map_.end());
@@ -307,6 +321,12 @@ DownloadItemImpl::~DownloadItemImpl() {
   delegate_->Detach();
 }
 
+base::WeakPtr<content::DownloadDestinationController>
+DownloadItemImpl::DestinationControllerAsWeakPtr() {
+  // Return does private downcast.
+  return weak_ptr_factory_.GetWeakPtr();

[benjhayden, 2012/07/25 15:19:16]

Why don't you want to use SupportsWeakPtr::AsWeakPtr()?

[Randy Smith (Not in Mondays), 2012/07/30 01:07:23]

A couple of reasons.  My usual reason (I don't want to export it as part of the
public interface) doesn't apply since this method is part of the public
interface.  But there was a recent thread on chromium-dev about the relationship
between weak pointer lifetimes and the lifetimes of the objects they point at,
and using a factory allows you to guarantee that the weak pointer is invalidated
before the member variable that you can get at through it are destructed.  Also,
to use SupportsWeakPtr I'd have to make the base class inherit (which is even
worse for the lifetime issue) or make the derived class inherit and downcast the
weak pointer (which I think I could do, but if I do it externally I'd have to do
the friend declaration for whatever method I actually did the downcasting in).

The above is a bit of a handwave, in that I'm not sure that I couldn't use
SupportsWeakPtr cleanly without lifetime concerns.  But I am (pretty :-}) sure I
can use the factory that way, so I figured I'd go with it.

+}
+
 void DownloadItemImpl::AddObserver(Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -395,21 +415,6 @@ void DownloadItemImpl::DangerousDownloadValidated() {
   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.
@@ -470,6 +475,20 @@ void DownloadItemImpl::Cancel(bool 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.

[benjhayden, 2012/07/25 15:19:16]

Couldn't you use weak pointers to bounce a callback off DII?
Everytime I see base::Unretained, I have a heart attack. Reassuring comments are
like flattery from a stranger who wants your money. Maybe they're true, maybe
they used to be true, either way I have a hard time believing them.

[Randy Smith (Not in Mondays), 2012/07/30 01:07:23]

I assume you mean off of the DownloadFile; I am using weak pointers for the
DownloadItemImpl.
So trust but verify :-}.  I think it's a pretty important part of the design of
this CL that DownloadItemImpl owns the DownloadFile and controls its lifetime. 
For reassurance around this PostTask, you simply need to believe that nothing
besides the DII can destroy the DF, and that the DII doesn't hold on to a
pointer to the DF once it's destroyed it.  I think those two assumptions are
core enough to this CL that they can be relied on :-}.

I don't want to use a WeakPtr because then I need to expose the DownloadFile
supporting weak pointers as part of the DownloadFile interface, which
complicates the interface.  

+                 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
@@ -506,12 +525,16 @@ void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) {
 }
 
 void DownloadItemImpl::OnAllDataSaved(
-    int64 size, const std::string& final_hash) {
+    const std::string& final_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   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();
 }
 
@@ -700,43 +723,47 @@ void DownloadItemImpl::TogglePause() {
   UpdateObservers();
 }
 
-void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) {
+void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // Lost Cancel race.
+  if (!IsInProgress())
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
   DCHECK(!GetTargetName().empty());
   DCHECK_NE(DANGEROUS, GetSafetyState());
-  DCHECK(file_manager);
 
   if (NeedsRename()) {
-    DownloadFileManager::RenameCompletionCallback callback =
+    content::DownloadFile::RenameCompletionCallback callback =
         base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   base::Unretained(file_manager));
+                   weak_ptr_factory_.GetWeakPtr());
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileManager::RenameDownloadFile,
-                   file_manager, GetGlobalId(), GetTargetFilePath(),
-                   true, callback));
+        base::Bind(&DownloadFile::Rename,
+                   base::Unretained(download_file_.get()),
+                   GetTargetFilePath(), true, callback));
   } else {
     // Complete the download and release the DownloadFile.
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileManager::CompleteDownload,
-                   file_manager, GetGlobalId(),
+        base::Bind(&ReleaseDownloadFile, base::Passed(download_file_.Pass()),
                    base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                               weak_ptr_factory_.GetWeakPtr())));
   }
 }
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(
-    DownloadFileManager* file_manager,
     content::DownloadInterruptReason reason,
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // Lost Cancel race.
+  if (!IsInProgress())
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
            << " needed rename = " << NeedsRename()
@@ -755,14 +782,30 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
   delegate_->DownloadRenamedToFinalName(this);
 
   // Complete the download and release the DownloadFile.
+  DCHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CompleteDownload,
-                 file_manager, GetGlobalId(),
+      base::Bind(&ReleaseDownloadFile, 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();
@@ -893,16 +936,31 @@ void DownloadItemImpl::OnContentCheckCompleted(
 }
 
 void DownloadItemImpl::OnIntermediatePathDetermined(
-    DownloadFileManager* file_manager,
     const FilePath& intermediate_path) {
-  DownloadFileManager::RenameCompletionCallback callback =
+  if (!IsInProgress()) {
+    // Lost interrupt/cancel race.  We need to continue the cascade
+    // anyway, so that we get this entry persisted and made visible
+    // to observers.  Actual error code doesn't matter as we've
+    // already stored the original reason for failure.
+    //
+    // TODO(rdsmith): Remove this code when we make downloads visible to
+    // observers earlier in the cascade so they can see immediate transitions
+    // to INTERRUPTED.
+    OnDownloadRenamedToIntermediateName(
+        content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED, FilePath());
+    return;
+  }
+
+  content::DownloadFile::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
+
+  DCHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::RenameDownloadFile,
-                 file_manager, GetGlobalId(), intermediate_path,
-                 false, callback));
+      base::Bind(&DownloadFile::Rename,
+                 base::Unretained(download_file_.get()),
+                 intermediate_path, false, callback));
 }
 
 const FilePath& DownloadItemImpl::GetFullPath() const {
@@ -924,14 +982,62 @@ FilePath DownloadItemImpl::GetUserVerifiedFilePath() const {
       GetTargetFilePath() : GetFullPath();
 }
 
-void DownloadItemImpl::OffThreadCancel(DownloadFileManager* file_manager) {
+void DownloadItemImpl::OffThreadCancel() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   request_handle_->CancelRequest();
 
+  DCHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CancelDownload,
-                 file_manager, download_id_));
+      // Will be deleted at end of task execution.
+      base::Bind(&DownloadFile::Cancel, base::Owned(download_file_.release())));
+}
+
+void DownloadItemImpl::DestinationUpdate(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
+    // 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) {
+  Interrupt(reason);
+}
+
+void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
+  if (!IsInProgress())
+    return;
+  OnAllDataSaved(final_hash);
+  delegate_->MaybeCompleteDownload(this);
 }
 
 void DownloadItemImpl::Init(bool active,
@@ -1042,7 +1148,8 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
         " 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(),
@@ -1054,7 +1161,8 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
         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());
   }