[Downloads] Add a RESUMING_INTERNAL state to DownloadItem.

When resuming an interrupted download, the download switches to this
state after the resumption request has been dispatched. This prevents a
second resumption request from being sent during the window between
dispatching the first request and receiving a response.

BUG=7648

Review URL: https://chromiumcodereview.appspot.com/14955002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@200332 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 203 insertions, 70 deletions

diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index 9d1a3c6..f535d18 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -1378,4 +1378,42 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithFileFinalRenameError) {
   EXPECT_EQ(download->GetState(), DownloadItem::COMPLETE);
 }
 
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, RemoveResumingDownload) {
+  SetupEnsureNoPendingDownloads();
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+  ASSERT_TRUE(test_server()->Start());
+
+  GURL url = test_server()->GetURL(
+      base::StringPrintf("rangereset?size=%d&rst_boundary=%d",
+                   GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+
+  MockDownloadManagerObserver dm_observer(DownloadManagerForShell(shell()));
+  EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1);
+
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  WaitForData(download, GetSafeBufferChunk());
+  ::testing::Mock::VerifyAndClearExpectations(&dm_observer);
+
+  // Tell the server to send the RST and confirm the interrupt happens.
+  ReleaseRSTAndConfirmInterruptForResume(download);
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+
+  // Resume and remove download. We expect only a single OnDownloadCreated()
+  // call, and that's for the second download created below.
+  EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1);
+  download->Resume();
+  download->Remove();
+
+  // Start the second download and wait until it's done.
+  base::FilePath file(FILE_PATH_LITERAL("download-test.lib"));
+  GURL url2(URLRequestMockHTTPJob::GetMockUrl(file));
+  // Download the file and wait.
+  DownloadAndWait(shell(), url2, DownloadItem::COMPLETE);
+
+  EXPECT_TRUE(EnsureNoPendingDownloads());
+}
+
 }  // namespace content
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index f1e45f2..b5230a3 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -309,33 +309,39 @@ void DownloadItemImpl::Pause() {
 
 void DownloadItemImpl::Resume() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  switch (state_) {
+    case IN_PROGRESS_INTERNAL:
+      if (!is_paused_)
+        return;
+      request_handle_->ResumeRequest();
+      is_paused_ = false;
+      UpdateObservers();
+      return;
 
-  // Ignore irrelevant states.
-  if (state_ == COMPLETE_INTERNAL ||
-      state_ == COMPLETING_INTERNAL ||
-      state_ == CANCELLED_INTERNAL ||
-      (state_ == IN_PROGRESS_INTERNAL && !is_paused_))
-    return;
+    case COMPLETING_INTERNAL:
+    case COMPLETE_INTERNAL:
+    case CANCELLED_INTERNAL:
+    case RESUMING_INTERNAL:
+      return;
 
-  if (state_ == INTERRUPTED_INTERNAL) {
-    auto_resume_count_ = 0;  // User input resets the counter.
-    ResumeInterruptedDownload();
-    return;
-  }
-  DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
+    case INTERRUPTED_INTERNAL:
+      auto_resume_count_ = 0;  // User input resets the counter.
+      ResumeInterruptedDownload();
+      return;
 
-  request_handle_->ResumeRequest();
-  is_paused_ = false;
-  UpdateObservers();
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      NOTREACHED();
+  }
 }
 
 void DownloadItemImpl::Cancel(bool user_cancel) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
-  if (state_ != IN_PROGRESS_INTERNAL && state_ != INTERRUPTED_INTERNAL) {
-    // Small downloads might be complete before this method has
-    // a chance to run.
+  if (state_ != IN_PROGRESS_INTERNAL &&
+      state_ != INTERRUPTED_INTERNAL &&
+      state_ != RESUMING_INTERNAL) {
+    // Small downloads might be complete before this method has a chance to run.
     return;
   }
 
@@ -352,7 +358,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
   if (!is_save_package_download_ && download_file_)
     ReleaseDownloadFile(true);
 
-  if (state_ != INTERRUPTED_INTERNAL) {
+  if (state_ == IN_PROGRESS_INTERNAL) {
     // Cancel the originating URL request unless it's already been cancelled
     // by interrupt.
     request_handle_->CancelRequest();
@@ -1009,6 +1015,14 @@ void DownloadItemImpl::Start(
   download_file_ = file.Pass();
   request_handle_ = req_handle.Pass();
 
+  if (IsCancelled()) {
+    // The download was in the process of resuming when it was cancelled. Don't
+    // proceed.
+    ReleaseDownloadFile(true);
+    request_handle_->CancelRequest();
+    return;
+  }
+
   TransitionTo(IN_PROGRESS_INTERNAL);
 
   last_reason_ = DOWNLOAD_INTERRUPT_REASON_NONE;
@@ -1287,6 +1301,25 @@ void DownloadItemImpl::Completed() {
   }
 }
 
+void DownloadItemImpl::OnResumeRequestStarted(DownloadItem* item,
+                                              net::Error error) {
+  // If |item| is not NULL, then Start() has been called already, and nothing
+  // more needs to be done here.
+  if (item) {
+    DCHECK_EQ(net::OK, error);
+    DCHECK_EQ(static_cast<DownloadItem*>(this), item);
+    return;
+  }
+  // Otherwise, the request failed without passing through
+  // DownloadResourceHandler::OnResponseStarted.
+  if (error == net::OK)
+    error = net::ERR_FAILED;
+  DownloadInterruptReason reason =
+      ConvertNetErrorToInterruptReason(error, DOWNLOAD_INTERRUPT_FROM_NETWORK);
+  DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason);
+  Interrupt(reason);
+}
+
 // **** End of Download progression cascade
 
 // An error occurred somewhere.
@@ -1303,7 +1336,7 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   // interrupts to race with cancels.
 
   // Whatever happens, the first one to hit the UI thread wins.
-  if (state_ != IN_PROGRESS_INTERNAL)
+  if (state_ != IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)
     return;
 
   last_reason_ = reason;
@@ -1516,7 +1549,7 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
     return;
 
   // If we're not interrupted, ignore the request; our caller is drunk.
-  if (!IsInterrupted())
+  if (state_ != INTERRUPTED_INTERNAL)
     return;
 
   // If we can't get a web contents, we can't resume the download.
@@ -1545,11 +1578,15 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
   download_params->set_hash_state(GetHashState());
   download_params->set_last_modified(GetLastModifiedTime());
   download_params->set_etag(GetETag());
+  download_params->set_callback(
+      base::Bind(&DownloadItemImpl::OnResumeRequestStarted,
+                 weak_ptr_factory_.GetWeakPtr()));
 
   delegate_->ResumeInterruptedDownload(download_params.Pass(), GetGlobalId());
-
   // Just in case we were interrupted while paused.
   is_paused_ = false;
+
+  TransitionTo(RESUMING_INTERNAL);
 }
 
 // static
@@ -1566,30 +1603,33 @@ DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
       return CANCELLED;
     case INTERRUPTED_INTERNAL:
       return INTERRUPTED;
-    default:
-      NOTREACHED();
+    case RESUMING_INTERNAL:
+      return INTERRUPTED;
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      break;
   }
+  NOTREACHED();
   return MAX_DOWNLOAD_STATE;
 }
 
 // static
 DownloadItemImpl::DownloadInternalState
 DownloadItemImpl::ExternalToInternalState(
-     DownloadState external_state) {
-   switch (external_state) {
-     case IN_PROGRESS:
-       return IN_PROGRESS_INTERNAL;
-     case COMPLETE:
-       return COMPLETE_INTERNAL;
-     case CANCELLED:
-       return CANCELLED_INTERNAL;
-     case INTERRUPTED:
-       return INTERRUPTED_INTERNAL;
-     default:
-       NOTREACHED();
-   }
-   return MAX_DOWNLOAD_INTERNAL_STATE;
- }
+    DownloadState external_state) {
+  switch (external_state) {
+    case IN_PROGRESS:
+      return IN_PROGRESS_INTERNAL;
+    case COMPLETE:
+      return COMPLETE_INTERNAL;
+    case CANCELLED:
+      return CANCELLED_INTERNAL;
+    case INTERRUPTED:
+      return INTERRUPTED_INTERNAL;
+    default:
+      NOTREACHED();
+  }
+  return MAX_DOWNLOAD_INTERNAL_STATE;
+}
 
 const char* DownloadItemImpl::DebugDownloadStateString(
     DownloadInternalState state) {
@@ -1604,10 +1644,13 @@ const char* DownloadItemImpl::DebugDownloadStateString(
       return "CANCELLED";
     case INTERRUPTED_INTERNAL:
       return "INTERRUPTED";
-    default:
-      NOTREACHED() << "Unknown download state " << state;
-      return "unknown";
+    case RESUMING_INTERNAL:
+      return "RESUMING";
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      break;
   };
+  NOTREACHED() << "Unknown download state " << state;
+  return "unknown";
 }
 
 const char* DownloadItemImpl::DebugResumeModeString(ResumeMode mode) {
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index 4487423..4223335 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -210,33 +210,72 @@ class CONTENT_EXPORT DownloadItemImpl
   virtual void DestinationCompleted(const std::string& final_hash) OVERRIDE;
 
  private:
-  // Fine grained states of a download.
-  enum DownloadInternalState {
-    // Unless otherwise specified, state transitions are linear forward
-    // in this list.
+  // Fine grained states of a download. Note that active downloads are created
+  // in IN_PROGRESS_INTERNAL state. However, downloads creates via history can
+  // be created in COMPLETE_INTERNAL, CANCELLED_INTERNAL and
+  // INTERRUPTED_INTERNAL.
 
-    // Includes both before and after file name determination.
+  enum DownloadInternalState {
+    // Includes both before and after file name determination, and paused
+    // downloads.
     // TODO(rdsmith): Put in state variable for file name determination.
+    // Transitions from:
+    //   <Initial creation>    Active downloads are created in this state.
+    //   RESUMING_INTERNAL
+    // Transitions to:
+    //   COMPLETING_INTERNAL   On final rename completion.
+    //   CANCELLED_INTERNAL    On cancel.
+    //   INTERRUPTED_INTERNAL  On interrupt.
+    //   COMPLETE_INTERNAL     On SavePackage download completion.
     IN_PROGRESS_INTERNAL,
 
     // Between commit point (dispatch of download file release) and completed.
-    // Embedder may be opening the file in this state.  Note that the
-    // DownloadItem may be deleted (by shutdown) or interrupted (e.g. due to a
-    // failure during AnnotateWithSourceInformation()) in this state.
+    // Embedder may be opening the file in this state.
+    // Transitions from:
+    //   IN_PROGRESS_INTERNAL
+    // Transitions to:
+    //   COMPLETE_INTERNAL     On successful completion.
     COMPLETING_INTERNAL,
 
     // After embedder has had a chance to auto-open.  User may now open
     // or auto-open based on extension.
+    // Transitions from:
+    //   COMPLETING_INTERNAL
+    //   IN_PROGRESS_INTERNAL  SavePackage only.
+    //   <Initial creation>    Completed persisted downloads.
+    // Transitions to:
+    //   <none>                Terminal state.
     COMPLETE_INTERNAL,
 
     // User has cancelled the download.
-    // Only incoming transition IN_PROGRESS->
+    // Transitions from:
+    //   IN_PROGRESS_INTERNAL
+    //   INTERRUPTED_INTERNAL
+    //   RESUMING_INTERNAL
+    //   <Initial creation>    Canceleld persisted downloads.
+    // Transitions to:
+    //   <none>                Terminal state.
     CANCELLED_INTERNAL,
 
     // An error has interrupted the download.
-    // Only incoming transition IN_PROGRESS->
+    // Transitions from:
+    //   IN_PROGRESS_INTERNAL
+    //   RESUMING_INTERNAL
+    //   <Initial creation>    Interrupted persisted downloads.
+    // Transitions to:
+    //   RESUMING_INTERNAL     On resumption.
     INTERRUPTED_INTERNAL,
 
+    // A request to resume this interrupted download is in progress.
+    // Transitions from:
+    //   INTERRUPTED_INTERNAL
+    // Transitions to:
+    //   IN_PROGRESS_INTERNAL  Once a server response is received from a
+    //                         resumption.
+    //   INTERRUPTED_INTERNAL  If the resumption request fails.
+    //   CANCELLED_INTERNAL    On cancel.
+    RESUMING_INTERNAL,
+
     MAX_DOWNLOAD_INTERNAL_STATE,
   };
 
@@ -290,6 +329,9 @@ class CONTENT_EXPORT DownloadItemImpl
   // is completed.
   void Completed();
 
+  // Callback invoked when the URLRequest for a download resumption has started.
+  void OnResumeRequestStarted(DownloadItem* item, net::Error error);
+
   // Helper routines -----------------------------------------------------------
 
   // Indicate that an error has occurred on the download.
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 3fa9043..5dc5aa1 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -247,12 +247,14 @@ DownloadManagerImpl::~DownloadManagerImpl() {
   DCHECK(!shutdown_needed_);
 }
 
-void DownloadManagerImpl::CreateActiveItem(
+DownloadItemImpl* DownloadManagerImpl::CreateActiveItem(
     DownloadId id, const DownloadCreateInfo& info) {
   net::BoundNetLog bound_net_log =
       net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-  downloads_[id.local()] =
+  DownloadItemImpl* download =
       item_factory_->CreateActiveItem(this, id, info, bound_net_log);
+  downloads_[id.local()] = download;
+  return download;
 }
 
 DownloadId DownloadManagerImpl::GetNextId() {
@@ -377,6 +379,26 @@ DownloadItem* DownloadManagerImpl::StartDownload(
     scoped_ptr<ByteStreamReader> stream) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  DownloadId id(info->download_id);
+  const bool new_download = !id.IsValid();
+  DownloadItemImpl* download = NULL;
+
+  if (new_download) {
+    id = GetNextId();
+    download = CreateActiveItem(id, *info);
+  } else {
+    DownloadMap::iterator item_iterator = downloads_.find(id.local());
+    // Trying to resume an interrupted download.
+    if (item_iterator == downloads_.end()) {
+      // If the download is no longer known to the DownloadManager, then it was
+      // removed after it was resumed. Ignore.
+      info->request_handle.CancelRequest();
+      return NULL;
+    }
+    download = item_iterator->second;
+    DCHECK(download->IsInterrupted());
+  }
+
   base::FilePath default_download_directory;
   if (delegate_) {
     base::FilePath website_save_directory;  // Unused
@@ -385,20 +407,6 @@ DownloadItem* DownloadManagerImpl::StartDownload(
                           &default_download_directory, &skip_dir_check);
   }
 
-  // If we don't have a valid id, that's a signal to generate one.
-  DownloadId id(info->download_id);
-  if (!id.IsValid())
-    id = GetNextId();
-
-  // Create a new download item if this isn't a resumption.
-  bool new_download(!ContainsKey(downloads_, id.local()));
-  if (new_download)
-    CreateActiveItem(id, *info);
-
-  DownloadItemImpl* download(downloads_[id.local()]);
-  DCHECK(download);
-  DCHECK(new_download || download->IsInterrupted());
-
   // Create the download file and start the download.
   scoped_ptr<DownloadFile> download_file(
       file_factory_->CreateFile(
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index 6885068..bbdd085 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -108,7 +108,8 @@ class CONTENT_EXPORT DownloadManagerImpl : public DownloadManager,
 
   // Create a new active item based on the info.  Separate from
   // StartDownload() for testing.
-  void CreateActiveItem(DownloadId id, const DownloadCreateInfo& info);
+  DownloadItemImpl* CreateActiveItem(DownloadId id,
+                                     const DownloadCreateInfo& info);
 
   // Get next download id.
   DownloadId GetNextId();
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index 388bcd1..35c6b24 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -65,8 +65,9 @@ static void StartOnUIThread(
   DownloadItem* item = download_manager->StartDownload(
       info.Pass(), stream.Pass());
 
+  // |item| can be NULL if the download has been removed.
   if (!started_cb.is_null())
-    started_cb.Run(item, net::OK);
+    started_cb.Run(item, item ? net::OK : net::ERR_ABORTED);
 }
 
 }  // namespace