[Downloads] Add more browsertests for resumption.

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 1018 matching lines @@
   if (GetState() == CANCELLED) {
     // 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;
-
   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())));
 }
 
 void DownloadItemImpl::OnDownloadFileInitialized(
@@ skipping 301 matching lines @@
   // Whatever happens, the first one to hit the UI thread wins.
   if (state_ != IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)
     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.
-  ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
-                      resume_mode == RESUME_MODE_USER_RESTART ||
-                      !IsDownloadResumptionEnabled());
+  if (download_file_.get()) {

[Randy Smith (Not in Mondays), 2013/06/19 21:46:21]

What are the circumstances that trigger this code (and the parallel
request_handle_ code below)?  This makes me worried that our state machine isn't
well-defined, as I think it's implying we can be interrupted while we're in
RESUMING.

[asanka, 2013/06/20 20:04:01]

That's exactly the transition that's being handled here (RESUMING ->
INTERRUPTED), which happens when the resumption request fails before a response
is received.

[Randy Smith (Not in Mondays), 2013/06/20 20:55:22]

Gotcha.  To the extent possible, I'd like to make state related conditionals
explicitly depend on the internal state, as part of trying to keep the
code-path-important changes in state to the state variable as much as we can. 
Willing to rewrite these to be conditionalized on state_, possibly with some
assertions?

[asanka, 2013/06/20 22:40:13]

Yup. That's a good suggestion.

+    ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
+                        resume_mode == RESUME_MODE_USER_RESTART ||
+                        !IsDownloadResumptionEnabled());
+  }
 
   // Reset all data saved, as even if we did save all the data we're going
   // to go through another round of downloading when we resume.
   // There's a potential problem here in the abstract, as if we did download
   // all the data and then run into a continuable error, on resumption we
   // won't download any more data.  However, a) there are currently no
   // continuable errors that can occur after we download all the data, and
   // b) if there were, that would probably simply result in a null range
   // request, which would generate a DestinationCompleted() notification
   // from the DownloadFile, which would behave properly with setting
   // all_data_saved_ to false here.
   all_data_saved_ = false;
 
   // Cancel the originating URL request.
-  request_handle_->CancelRequest();
+  if (request_handle_.get())
+    request_handle_->CancelRequest();
 
   TransitionTo(INTERRUPTED_INTERNAL);
   RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
 
   AutoResumeIfValid();
+
+  // Since the external state could be INTERRUPTED before and after the
+  // TransitionTo() calls, the observers may not be notified. Manually trigger
+  // an update since the value of CanResume() may have changed.
+  UpdateObservers();

[Randy Smith (Not in Mondays), 2013/06/19 21:46:21]

Making a note to myself to come back to this after I understand the state
diagram implications better.

 }
 
 void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (destroy_file) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         // Will be deleted at end of task execution.
         base::Bind(&DownloadFileCancel, base::Passed(&download_file_)));
@@ skipping 283 matching lines @@
     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