[Downloads] Always call DM::StartDownload() for explicit downloads.

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 ebef25349cd4f290f3437aae2158e8daa11d9bd5..285a40e3ab595a8854dc18956b06f1e15aa2039c 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -170,9 +170,9 @@ DownloadItemImpl::DownloadItemImpl(
     const net::BoundNetLog& bound_net_log)
     : is_save_package_download_(false),
       download_id_(download_id),
-      target_disposition_(
-          (info.save_info->prompt_for_save_location) ?
-              TARGET_DISPOSITION_PROMPT : TARGET_DISPOSITION_OVERWRITE),
+      target_disposition_((info.save_info->prompt_for_save_location)
+                              ? TARGET_DISPOSITION_PROMPT
+                              : TARGET_DISPOSITION_OVERWRITE),
       url_chain_(info.url_chain),
       referrer_url_(info.referrer_url),
       suggested_filename_(base::UTF16ToUTF8(info.save_info->suggested_name)),
@@ -201,7 +201,7 @@ DownloadItemImpl::DownloadItemImpl(
       auto_opened_(false),
       is_temporary_(!info.save_info->file_path.empty()),
       all_data_saved_(false),
-      destination_error_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
+      destination_error_(DOWNLOAD_INTERRUPT_REASON_NONE),
       opened_(false),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
@@ -341,8 +341,9 @@ void DownloadItemImpl::StealDangerousDownload(
 void DownloadItemImpl::Pause() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // Ignore irrelevant states.
-  if (state_ != IN_PROGRESS_INTERNAL || is_paused_)
+  // Ignore irrelevant states. Also ignore the call if there's no request
+  // handle.

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

This sounds like it's trying to handle Pause() requests that come in between the
OnDownloadCreated() and the call to Start().  But in the normal case
(non-resume, no problems starting the download) it seems like we really want
this to work for consumers; the only notification they're going to get is
OnDownloadCreated(), because it's created in the IN_PROGRESS state.  What is a
consumer that wants to pause on start supposed to do?

(This is an equivalent comment to my comment in dmi.cc about moving the
OnDownloadCreated() notification.)

[asanka, 2014/03/19 20:37:06]

Hmm. Good point. We could set is_paused_ and respect the flag when we eventually
receive the request_handle_. SG?

[Randy Smith (Not in Mondays), 2014/03/20 15:24:54]

That's effectively accepting that we have a race and handling it.  I'd rather
see if we could keep the original pattern and never allow the race to happen (by
not exposing the DI until it's in a state the consumer actually can play with). 
Let's see how our other discussions about state transitions fall out and then
revisit this issue.

[asanka, 2016/02/11 23:57:24]

But that's not correct. We are no longer in a place where it has one and only
one request session. After we expose the download it's still possible for it to
lose the request (interruption / auto resume / etc.) The state we expose and the
capabilities associated with each state are all racy (i.e. our visible state is
always subject to TOCTOU races).

[Randy Smith (Not in Mondays), 2016/02/12 18:58:10]

Ok.  I'm good with your race handling suggestion earlier in the thread.  Is that
what's now reflected in the code?  

("TOCTOU"?)

[asanka, 2016/02/12 20:52:28]

Time Of Check vs. Time Of Use. :-) Basically, the result of a CanResume() call
isn't a guarantee that the caller can call Resume() and expect something to
happen in an arbitrary point in the future.

I didn't add logic to set is_paused_ here and honor it the next time around.

+  if (state_ != IN_PROGRESS_INTERNAL || is_paused_ || !request_handle_)
     return;
 
   request_handle_->PauseRequest();
@@ -408,7 +409,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
   if (!is_save_package_download_ && download_file_)
     ReleaseDownloadFile(true);
 
-  if (state_ == IN_PROGRESS_INTERNAL) {
+  if (state_ == IN_PROGRESS_INTERNAL && request_handle_) {
     // Cancel the originating URL request unless it's already been cancelled
     // by interrupt.
     request_handle_->CancelRequest();
@@ -926,6 +927,7 @@ void DownloadItemImpl::MergeOriginInfoOnResume(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(RESUMING_INTERNAL, state_);
   DCHECK(!new_create_info.url_chain.empty());
+  DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, new_create_info.interrupt_reason);
 
   // We are going to tack on any new redirects to our list of redirects.
   // When a download is resumed, the URL used for the resumption request is the
@@ -1106,11 +1108,10 @@ void DownloadItemImpl::Init(bool active,
 // We're starting the download.
 void DownloadItemImpl::Start(
     scoped_ptr<DownloadFile> file,
-    scoped_ptr<DownloadRequestHandleInterface> req_handle) {
+    scoped_ptr<DownloadRequestHandleInterface> req_handle,
+    const DownloadCreateInfo& new_create_info) {
   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();
@@ -1123,6 +1124,24 @@ void DownloadItemImpl::Start(
     return;
   }
 
+  DCHECK(state_ == IN_PROGRESS_INTERNAL || state_ == RESUMING_INTERNAL);
+  if (new_create_info.interrupt_reason != DOWNLOAD_INTERRUPT_REASON_NONE) {
+    DCHECK(!download_file_.get());
+    // If the download failed, then skip the download_file_ initialization. It
+    // isn't going to be used anyway. OnDownloadFileInitialized() handles all
+    // interrupts that happen up until after download file initialization. Allow
+    // it to handle the new interrupt as well.
+    OnDownloadFileInitialized(new_create_info.interrupt_reason);

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

nit: Might want to change the name if we're expanding the contexts in which it's
called?

[Randy Smith (Not in Mondays), 2014/03/20 15:24:54]

Just noting you didn't respond to this nit (I don't care a lot, just making sure
it isn't dropped on the floor).

[asanka, 2016/02/11 23:57:24]

Whoops. Missed. I'm doing a pass through the previous patchsets to make sure I
get all the comments. Rietveld UX fail.

However, this comment is no longer applicable since I removed this call.
OnDownloadFileInitialized is now exclusively used as a completion callback for
download file initialization.

+    return;
+  }
+
+  // Successful download start.
+  DCHECK(download_file_.get());
+  DCHECK(request_handle_.get());
+
+  if (state_ == RESUMING_INTERNAL)
+    MergeOriginInfoOnResume(new_create_info);
+
   TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
 
   BrowserThread::PostTask(
@@ -1138,16 +1157,25 @@ void DownloadItemImpl::OnDownloadFileInitialized(
     DownloadInterruptReason result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (result != DOWNLOAD_INTERRUPT_REASON_NONE) {
+    // Either the download was interrupted at Start() or the download file
+    // failed to initialize. Either way, the download will not transition to an
+    // interrupted state.
     Interrupt(result);
-    // TODO(rdsmith/asanka): Arguably we should show this in the UI, but
-    // it's not at all clear what to show--we haven't done filename
-    // determination, so we don't know what name to display.  OTOH,
-    // the failure mode of not showing the DI if the file initialization
-    // fails isn't a good one.  Can we hack up a name based on the
-    // URLRequest?  We'll need to make sure that initialization happens
-    // properly.  Possibly the right thing is to have the UI handle
-    // this case specially.
-    return;
+
+    // The download may transition to RESUMING_INTERNAL as a result of the
+    // Interrupt() call above if it was resumed automatically. If so, then
+    // Start() is expected to be called again and the download progression
+    // cascade will be redone. Therefore we are done for now.
+    //
+    // NOTE: The check below assumes that resumption doesn't call Start()
+    // synchronously. If it did, then state_ may not be RESUMING_INTERNAL when
+    // we get here. This constraint is currently enforced via a DCHECK in
+    // Start() which requires that the download be in RESUMING_INTERNAL or
+    // IN_PROGRESS_INTERNAL state when Start() is called. If Start() was called
+    // synchronously, then the state of the DownloadItem would be
+    // INTERRUPTED_INTERNAL.
+    if (state_ == RESUMING_INTERNAL)
+      return;
   }
 
   delegate_->DetermineDownloadTarget(
@@ -1171,20 +1199,19 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
     return;
   }
 
-  // TODO(rdsmith,asanka): We are ignoring the possibility that the download
-  // has been interrupted at this point until we finish the intermediate
-  // rename and set the full path.  That's dangerous, because we might race
-  // with resumption, either manual (because the interrupt is visible to the
-  // UI) or automatic.  If we keep the "ignore an error on download until file
-  // name determination complete" semantics, we need to make sure that the
-  // error is kept completely invisible until that point.
-
   VLOG(20) << __FUNCTION__ << " " << target_path.value() << " " << disposition
            << " " << danger_type << " " << DebugString(true);
 
   target_path_ = target_path;
   target_disposition_ = disposition;
   SetDangerType(danger_type);
+  if (state_ != IN_PROGRESS_INTERNAL) {
+    // The download could be already interrupted, in which case there's nothing
+    // more we can do. Notify observers and quit the download progression
+    // cascade.
+    UpdateObservers();
+    return;
+  }
 
   // We want the intermediate and target paths to refer to the same directory so
   // that they are both on the same device and subject to same
@@ -1210,7 +1237,6 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
   //               filename. Unnecessary renames may cause bugs like
   //               http://crbug.com/74187.
   DCHECK(!is_save_package_download_);
-  DCHECK(download_file_.get());

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

Ok, I'm confused.  It seems to me that if we're not interrupted at this point,
we've got a download file.  Also, we're about to do a PostTask() to the DF in a
couple of lines.  Why remove the DCHECK?  Just because it's obvious and we'll
trip in an obvious way below if the download_file_ is null?

[asanka, 2014/03/19 20:37:06]

Ah. The DCHECK removal was due to an interim change that got pulled back. But
trying to invoke DownloadFile::RenameAndUniquify() on a NULL DownloadFile would
trip hard. I restored the DCHECK() to avoid distractions.

   DownloadFile::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
@@ -1403,22 +1429,6 @@ void DownloadItemImpl::Completed() {
   }
 }
 
-void DownloadItemImpl::OnResumeRequestStarted(
-    DownloadItem* item,
-    DownloadInterruptReason interrupt_reason) {
-  // If |item| is not NULL, then Start() has been called already, and nothing
-  // more needs to be done here.
-  if (item) {
-    DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
-    DCHECK_EQ(static_cast<DownloadItem*>(this), item);
-    return;
-  }
-  // Otherwise, the request failed without passing through
-  // DownloadResourceHandler::OnResponseStarted.
-  DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
-  Interrupt(interrupt_reason);
-}
-
 // **** End of Download progression cascade
 
 // An error occurred somewhere.
@@ -1443,34 +1453,31 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
 
   ResumeMode resume_mode = GetResumeMode();
 
-  if (state_ == IN_PROGRESS_INTERNAL) {

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

So this change is removing a bit of redundancy from the code, in a way I'm not
completely comfortable with.  I'd rather be able to tell all "important" things
about the DI state from the state_ variable, as opposed to having extra booleans
implied based on null/non-null of various variables.  The only condition I think
of offhand in which the DI is in IN_PROGRESS_INTERNAL and we don't have a
download_file_ or request_handle_ is between creation and start, which was
historically handled by not doing any updates in that (so that no-one could call
back into the DI).  What problems do you see in going back to that?  Is it only
the transition around construction->Start()?

[asanka, 2014/03/19 20:37:06]

The problem was the handling of downloads that were interrupted on start. Since
I'm avoiding creating a DownloadFile for those, the initial Interrupt() call
can't assume the existence of a download_file_.

I suppose the problem is with the (increasingly) overloaded IN_PROGRESS_INTERNAL
state not capturing the finer grained state in the DownloadItem. I see your
suggestion to always start the download in an INTERRUPTED_INTERNAL state. I'm a
bit apprehensive about that because we'd then be overloading the meaning of
INTERRUPTED_INTERNAL which can be confusing. How about another internal state
STARTING_INTERNAL which maps externally to IN_PROGRESS?

Alternatively, if external consumers had a way to distinguish between new
downloads and old ones being restored from history without relying on an initial
IN_PROGRESS state, then we can create the download directly in an
INTERRUPTED_INTERNAL state if the request failed.

[Randy Smith (Not in Mondays), 2014/03/20 15:24:54]

We could do this, but my instinct is that we should avoid creating states unless
we have to; it'll take several iterations to make sure we get the state
definition and transitions correct.  
Yeah, this is the way I'd rather go if we can.  In what situations do external
consumers need to know the difference between downloads that fail as they're
started and downloads that are being restored in an interrupted state?

[asanka, 2016/02/11 23:57:24]

New downloads that start in a failed state need to be displayed to the user,
while restored interrupted downloads don't.

[Randy Smith (Not in Mondays), 2016/02/12 18:58:10]

I feel absolutely no need to resolve this old philosophical issue for this CL,
but I do still hold the generic opinions expressed two years ago :-}.  Let's
chat offline at some point about what set of guidelines we want to have for
managing DII state going forward.  It's far more complicated than I'd ideally
like, and that may or may not be inevitable.

[asanka, 2016/02/12 20:52:28]

Acknowledged.

-    // Cancel (delete file) if:
-    // 1) we're going to restart.
-    // 2) Resumption isn't possible (download was cancelled or blocked due to
-    //    security restrictions).
-    // 3) Resumption isn't enabled.
-    // No point in leaving data around we aren't going to use.
+  // Cancel (delete file) if:
+  // 1) we're going to restart.
+  // 2) Resumption isn't possible (download was cancelled or blocked due to
+  //    security restrictions).
+  // 3) Resumption isn't enabled.
+  // No point in leaving data around we aren't going to use.
+  if (download_file_.get())
     ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
                         resume_mode == RESUME_MODE_USER_RESTART ||
                         resume_mode == RESUME_MODE_INVALID ||
                         !IsDownloadResumptionEnabled());
 
-    // Cancel the originating URL request.
+  // Cancel the originating URL request.
+  if (request_handle_.get())
     request_handle_->CancelRequest();
-  } else {
-    DCHECK(!download_file_.get());
-  }
 
-  // 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.
+  // 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;
 
   TransitionTo(INTERRUPTED_INTERNAL, DONT_UPDATE_OBSERVERS);
@@ -1701,9 +1708,6 @@ 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()));

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

Isn't this assuming that neither of the fall-through cases in which
DownloadResourceHandler is created but not invoked is going to happen?  Is that
safe, even for resumption?

[asanka, 2014/03/19 20:37:06]

I verified that ResourceHandler::OnResponseCompleted() would be invoked if
another handler returned false from an OnWillStart() call. The other case where
the on_started callback would have been necessary is if the
DownloadResourceHandler was created in vain and destroyed. In the case of
resumption, we don't go through the BufferedResourceHandler where this problem
exists.

There's currently no API contract that we can rely on which says that Start()
MUST be called if delegate_->ResumeInterruptedDownload() is called. We could
enforce such a thing if: a) we don't go through the ResourceLoader stack for
resumption (which is one of our long term goals), and/or b) we change the
ResourceLoader stack to enforce that any ResourceHandler isn't created in vain
in which case we can rely on the OnResponseCompleted() callback to schedule a
Start() call.

[Randy Smith (Not in Mondays), 2014/03/20 18:04:46]

What about the places outside DownloadResourceHandler where the started callback
gets called?  (E.g. ResourcDispatcherHostImp::BeginDownload())

[asanka, 2016/02/11 23:57:24]

RDH::BeginDownload doesn't call the started callback. We do have one hole where
the WebContents might go away between DII::ResumeInterruptedDownload and the
response coming back. I'll work on closing that by making the DownloadManager
determination not depend on the RVH.

 
   delegate_->ResumeInterruptedDownload(download_params.Pass(), GetId());
   // Just in case we were interrupted while paused.