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

[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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200332

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

So as you probably expect :-}, my primary concern in this CL is about races,
specifically races happening during the window we're RESUMING_INTERNAL.  There's
some discussion of Cancel/Interrupt below (and I think those behaviors are
basically right), and I also think ignoring double resumes is right.  What are
your thoughts about races with filename determination?   It looks basically
right to me, but that's partially because of the weak pointer revocation in
ReleaseDownloadFile(), which seems a little confusing to rely on for
non-download-file callbacks.  Are you relying on that?  Or is there some other
reason you consider yourself safe for races with the filename determination?

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1061:
request_handle_->CancelRequest();
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.

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1383: if (state_ !=
IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)
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().

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.h:219: // TODO(rdsmith): Put in
state variable for file name determination.
Add a note that RESUMING_INTERNAL can transition into this state?

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.h:233: // Only incoming transition
IN_PROGRESS->
This is wrong; we can cancel interrupted downloads as well.  Willing to fix it?

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.h:237: // Only incoming transitions
IN_PROGRESS-> and RESUMING_INTERNAL.
I don't actually remember how I was thinking of the transition syntax I used
here (which means you're welcome to change it to something else if you think
it's confusing), but this looks inconsistent.  If you keep this syntax, either
add a --> after the RESUMING_INTERNAL, or put the RESUMING_INTERNAL next to the
IN_PROGRESS so that the existing arrow can do double duty?

Alternatively, you could do a transition summary at the beginning of this enum
in either transition table form, or just listing all possible transitions.

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

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1676: }
Thanks for fixing the above lines, please also dedent this line.

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

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1061:
request_handle_->CancelRequest();
On 2013/05/09 01:00:57, rdsmith wrote:
> 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.

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.

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1383: if (state_ !=
IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)
On 2013/05/09 01:00:57, rdsmith wrote:
> 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().

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

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1676: }
On 2013/05/09 14:49:51, benjhayden_chromium wrote:
> Thanks for fixing the above lines, please also dedent this line.

Ah thanks! Missed that one.

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.h:219: // TODO(rdsmith): Put in
state variable for file name determination.
On 2013/05/09 01:00:57, rdsmith wrote:
> Add a note that RESUMING_INTERNAL can transition into this state?

Done.

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.h:233: // Only incoming transition
IN_PROGRESS->
On 2013/05/09 01:00:57, rdsmith wrote:
> This is wrong; we can cancel interrupted downloads as well.  Willing to fix
it?

Done.

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.h:237: // Only incoming transitions
IN_PROGRESS-> and RESUMING_INTERNAL.
On 2013/05/09 01:00:57, rdsmith wrote:
> I don't actually remember how I was thinking of the transition syntax I used
> here (which means you're welcome to change it to something else if you think
> it's confusing), but this looks inconsistent.  If you keep this syntax, either
> add a --> after the RESUMING_INTERNAL, or put the RESUMING_INTERNAL next to
the
> IN_PROGRESS so that the existing arrow can do double duty?
> 
> Alternatively, you could do a transition summary at the beginning of this enum
> in either transition table form, or just listing all possible transitions.

I added notes listing all the transitions. Let me know if it's not clear.

The transitions weren't linear enough for a single comment at the top. :-/

[asanka, 2013-05-09 17:06:11]

On 2013/05/09 01:00:56, rdsmith wrote:
> So as you probably expect :-}, my primary concern in this CL is about races,
> specifically races happening during the window we're RESUMING_INTERNAL. 
There's
> some discussion of Cancel/Interrupt below (and I think those behaviors are
> basically right), and I also think ignoring double resumes is right.  What are
> your thoughts about races with filename determination?   It looks basically
> right to me, but that's partially because of the weak pointer revocation in
> ReleaseDownloadFile(), which seems a little confusing to rely on for
> non-download-file callbacks.  Are you relying on that?  Or is there some other
> reason you consider yourself safe for races with the filename determination?

We specifically avoid transitioning to INTERRUPTED while filename determination
(FND) is in progress. Which implies that INTERRUPTED always happens after FND is
done. Also, FND only starts in IN_PROGRESS.

=> The INTERRUPTED -> RESUMING -> [INTERRUPTED -> RESUMING]* -> IN_PROGRESS
transition happens outside of FND.

I don't rely on the weak pointer revocation in ReleaseDownloadFile() since that
only happens outside  RESUMING -> [INTERRUPTED -> RESUMING]* -> IN_PROGRESS.
Note that Cancel() only calls ReleaseDownloadFile() if download_file_ is
non-empty.

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

Let's talk in person about filename determination races; I want to explore
Cancel/Interrupt/Resume/FND transition.  At first glance I think you're right
that our holding off interrupts during FND means that we're ok, but it feels
messy to me, so I want to take a second glance.

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1061:
request_handle_->CancelRequest();
On 2013/05/09 16:54:33, asanka wrote:
> On 2013/05/09 01:00:57, rdsmith wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/14955002/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1383: if (state_ !=
IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)
On 2013/05/09 16:54:33, asanka wrote:
> On 2013/05/09 01:00:57, rdsmith wrote:
> > 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().
> 
> RESUMING_INTERNAL -> INTERRUPTED_INTERNAL happens if the resumption request
> fails. E.g.: internet disconnected while trying to resume a download.

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.

[Randy Smith (Not in Mondays), 2013-05-10 16:57:49]

As noted offline, LGTM conditional on fixing REMOVED race.

[asanka, 2013-05-10 21:08:15]

Resolved REMOVED race.

The changes are in download_manager_impl*, download_browsertest* and
download_resource_handler.

[benjhayden, 2013-05-11 17:10:09]

lgtm, one question

https://codereview.chromium.org/14955002/diff/14001/content/browser/download/...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/14955002/diff/14001/content/browser/download/...
content/browser/download/download_resource_handler.cc:69: started_cb.Run(item,
item ? net::OK : net::ERR_FAILED);
Could item be NULL for any reason other than the Remove race? Would
net::ERR_ABORTED be more accurate here?

[Randy Smith (Not in Mondays), 2013-05-13 15:59:26]

LGTM.

https://codereview.chromium.org/14955002/diff/14001/content/browser/download/...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/14955002/diff/14001/content/browser/download/...
content/browser/download/download_manager_impl.cc:390: download =
downloads_[id.local()];
I'm tempted to suggest you explicitly look up the iterator, test against end()
to check if the key is there, then dereference to get the download, to avoid the
double lookup.  But I'm good with this code; just wanted to raise the option.

[asanka, 2013-05-13 19:19:01]

Thanks!

https://codereview.chromium.org/14955002/diff/14001/content/browser/download/...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/14955002/diff/14001/content/browser/download/...
content/browser/download/download_manager_impl.cc:390: download =
downloads_[id.local()];
On 2013/05/13 15:59:26, rdsmith wrote:
> I'm tempted to suggest you explicitly look up the iterator, test against end()
> to check if the key is there, then dereference to get the download, to avoid
the
> double lookup.  But I'm good with this code; just wanted to raise the option.

Done.

https://codereview.chromium.org/14955002/diff/14001/content/browser/download/...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/14955002/diff/14001/content/browser/download/...
content/browser/download/download_resource_handler.cc:69: started_cb.Run(item,
item ? net::OK : net::ERR_FAILED);
On 2013/05/11 17:10:09, benjhayden_chromium wrote:
> Could item be NULL for any reason other than the Remove race? Would
> net::ERR_ABORTED be more accurate here?

Done.

[commit-bot: I haz the power, 2013-05-15 14:19:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/14955002/27001

[commit-bot: I haz the power, 2013-05-15 14:23:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/14955002/11004

[commit-bot: I haz the power, 2013-05-15 19:20:03]

Change committed as 200332