Replace the DownloadFileManager with direct ownership

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 81 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";
   }
 };
 
+// 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 {
 
 // 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 159 matching lines @@
       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());
+
   TransitionTo(REMOVING);
   STLDeleteContainerPairSecondPointers(
       external_data_map_.begin(), external_data_map_.end());
   delegate_->AssertStateConsistent(this);
   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));
 
   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 40 matching lines @@
     return;
   }
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
   TransitionTo(CANCELLED);
   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.

[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
   // 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.
@@ skipping 16 matching lines @@
   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(!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 168 matching lines @@
 
   DCHECK(IsInProgress());
   if (is_paused_)
     request_handle_->ResumeRequest();
   else
     request_handle_->PauseRequest();
   is_paused_ = !is_paused_;
   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()
            << " " << 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.
+  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();
   else
     delegate_delayed_complete_ = true;
 }
 
 void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     content::DownloadInterruptReason reason,
     const FilePath& full_path) {
@@ skipping 110 matching lines @@
 
 void DownloadItemImpl::OnContentCheckCompleted(
     content::DownloadDangerType danger_type) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(AllDataSaved());
   SetDangerType(danger_type);
 }
 
 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 {
   return current_path_;
 }
 
 FilePath DownloadItemImpl::GetFileNameToReportUser() const {
   if (!display_name_.empty())
     return display_name_;
   return target_path_.BaseName();
 }
 
 void DownloadItemImpl::SetDisplayName(const FilePath& name) {
   display_name_ = name;
 }
 
 FilePath DownloadItemImpl::GetUserVerifiedFilePath() const {
   return (safety_state_ == DownloadItem::SAFE) ?
       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,
                             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 90 matching lines @@
         " total = %" PRId64
         " received = %" PRId64
         " reason = %s"
         " paused = %c"
         " otr = %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',
         IsOtr() ? '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 118 matching lines @@
   std::map<const void*, ExternalData*>::iterator it =
       external_data_map_.find(key);
 
   if (it == external_data_map_.end()) {
     external_data_map_[key] = data;
   } else if (it->second != data) {
     delete it->second;
     it->second = data;
   }
 }