[Resumption 6/11] Add a RESUMING_INTERNAL state to DownloadItem.

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.
 
 // File method ordering: Methods in this file are in the same order as
 // in download_item_impl.h, with the following exception: The public
 // interface Start is placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state
 // transitions as the download progresses.  See "Download progression
 // cascade" later in this file.
@@ skipping 291 matching lines @@
   if (state_ != IN_PROGRESS_INTERNAL || is_paused_)
     return;
 
   request_handle_->PauseRequest();
   is_paused_ = true;
   UpdateObservers();
 }
 
 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;
+    case INTERRUPTED_INTERNAL:
+      auto_resume_count_ = 0;  // User input resets the counter.
+      ResumeInterruptedDownload();
+      return;
+
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      NOTREACHED();
   }
-  DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
-
-  request_handle_->ResumeRequest();
-  is_paused_ = false;
-  UpdateObservers();
 }
 
 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;
   }
 
   last_reason_ = user_cancel ?
       DOWNLOAD_INTERRUPT_REASON_USER_CANCELED :
       DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN;
 
   RecordDownloadCount(CANCELLED_COUNT);
 
   // TODO(rdsmith/benjhayden): Remove condition as part of
   // |SavePackage| integration.
   // |download_file_| can be NULL if Interrupt() is called after the
   // download file has been released.
   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();
   }
 
   TransitionTo(CANCELLED_INTERNAL);
 }
 
 void DownloadItemImpl::Delete(DeleteReason reason) {
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
@@ skipping 675 matching lines @@
     scoped_ptr<DownloadFile> file,
     scoped_ptr<DownloadRequestHandleInterface> req_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!download_file_.get());
   DCHECK(file.get());
   DCHECK(req_handle.get());
 
   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();

[Randy Smith (Not in Mondays), 2013/05/09 01:00:57]

Is there a reason to do this here, rather than above in Cancel()? I think
there's a virtue in having cancel imply that there isn't a download file; if
not, there's effectively one more emergent state to track.

I *think* you could make it fire-and-forget by revoking the weak pointers at
that point as well.

[asanka, 2013/05/09 16:54:33]

I may be misreading this comment.

The code is trying to handle a race between a URLRequest dispatched for a
resumption and a possibly user initiated Cancel. For example, if the following
sequence of events happen:

1. ::Interrupt().
2. ::ResumeInterruptedDownload(): Fires off a request.
3. ::Cancel().
4. Response received for request sent in #2.
5. ::Start() called in response to #4.

The request_handle and download_file being discarded are those that were created
in response to #4.

[Randy Smith (Not in Mondays), 2013/05/10 15:00:30]

Whoops; the key thing I was spacing on was the last point.  We've started off
the request, but we don't have a handle for it yet.  Ok, I'm good.

+    return;
+  }
+
   TransitionTo(IN_PROGRESS_INTERNAL);
 
   last_reason_ = DOWNLOAD_INTERRUPT_REASON_NONE;
 
   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,
@@ skipping 263 matching lines @@
     // we still need to set it auto-opened so that it can be removed from the
     // download shelf.
     if (!IsTemporary())
       OpenDownload();
 
     auto_opened_ = true;
     UpdateObservers();
   }
 }
 
+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.
 void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // 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 (state_ != IN_PROGRESS_INTERNAL)
+  if (state_ != IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)

[Randy Smith (Not in Mondays), 2013/05/09 01:00:57]

Why should an interrupt fail in RESUMING_INTERNAL mode?  I can't offhand think
why one would happen, but that would mean a DCHECK(), not an ignore.  Cancels
work; if we're going to implement a particular behavior for this case, I would
think it would be parallel to Cancel().

[asanka, 2013/05/09 16:54:33]

RESUMING_INTERNAL -> INTERRUPTED_INTERNAL happens if the resumption request
fails. E.g.: internet disconnected while trying to resume a download.

[Randy Smith (Not in Mondays), 2013/05/10 15:00:30]

Got it.  It was interrupts happening before we got a response to our request
dispatch that I was thinking of, but of course the request dispatch itself can
fail.

     return;
 
   last_reason_ = reason;
 
   ResumeMode resume_mode = GetResumeMode();
   // Cancel (delete file) if we're going to restart; no point in leaving
   // data around we aren't going to use.  Also cancel if resumption isn't
   // enabled for the same reason.
   bool resumption_enabled = CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableDownloadResumption);
@@ skipping 192 matching lines @@
 void DownloadItemImpl::ResumeInterruptedDownload() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // If the flag for downloads resumption isn't enabled, ignore
   // this request.
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
   if (!command_line.HasSwitch(switches::kEnableDownloadResumption))
     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.
   // TODO(rdsmith): Find some alternative web contents to use--this
   // means we can't restart a download if it's a download imported
   // from the history.
   if (!GetWebContents())
     return;
 
   // Reset the appropriate state if restarting.
   ResumeMode mode = GetResumeMode();
   if (mode == RESUME_MODE_IMMEDIATE_RESTART ||
       mode == RESUME_MODE_USER_RESTART) {
     received_bytes_ = 0;
     hash_state_ = "";
     last_modified_time_ = "";
     etag_ = "";
   }
 
   scoped_ptr<DownloadUrlParameters> download_params(
       DownloadUrlParameters::FromWebContents(GetWebContents(),
                                              GetOriginalUrl()));
 
   download_params->set_file_path(GetFullPath());
   download_params->set_offset(GetReceivedBytes());
   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
 DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
     DownloadInternalState internal_state) {
   switch (internal_state) {
     case IN_PROGRESS_INTERNAL:
       return IN_PROGRESS;
     case COMPLETING_INTERNAL:
       return IN_PROGRESS;
     case COMPLETE_INTERNAL:
       return COMPLETE;
     case CANCELLED_INTERNAL:
       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;
  }

[benjhayden, 2013/05/09 14:49:51]

Thanks for fixing the above lines, please also dedent this line.

[asanka, 2013/05/09 16:54:33]

Ah thanks! Missed that one.

 
 const char* DownloadItemImpl::DebugDownloadStateString(
     DownloadInternalState state) {
   switch (state) {
     case IN_PROGRESS_INTERNAL:
       return "IN_PROGRESS";
     case COMPLETING_INTERNAL:
       return "COMPLETING";
     case COMPLETE_INTERNAL:
       return "COMPLETE";
     case CANCELLED_INTERNAL:
       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) {
   switch (mode) {
     case RESUME_MODE_INVALID:
       return "INVALID";
     case RESUME_MODE_IMMEDIATE_CONTINUE:
       return "IMMEDIATE_CONTINUE";
     case RESUME_MODE_IMMEDIATE_RESTART:
       return "IMMEDIATE_RESTART";
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content