Check for cancel in DownloadManager::OnAllDataSaved().

Check for cancel in DownloadManager::OnAllDataSaved().


BUG=70592
TEST=All downloads tests pass

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72395

[Randy Smith (Not in Mondays), 2011-01-24 21:37:36]

Quick review to nail a canary common crasher?  I'll take the first LGTM I get
for speed, but would be interested in follow-on comments.  To answer the obvious
question: I'm not putting in a new test for this case, because that test is
already in progress in a different branch (you folks have all seen it), it is
*not* simple, and I want to get this in quickly.

To turn my embarrassment into a teachable moment: One common pattern in the
download system that I have every intention of keeping as I refactor, is that
the long-lived core classes (ResourceDispatcherHost, DownloadFileManager,
DownloadManager) serve as targets for messages aimed at the per-download objects
on their threads (DownloadResourceHandler, DownloadFile, DownloadItem), and if
those objects have gone away, they are supposed to drop those messages on the
floor.  In my ideal world, that'd be the main thing they did, and the objects
would do most of the real work.  My recent history deflake commit ignored that
invariant, and now I'm paying the price.  I invite folks to keep an eye out for
this failure mode on future commits (as I'll try to).

[ahendrickson, 2011-01-24 21:44:27]

LGTM.

On 2011/01/24 21:37:36, rdsmith wrote:
> Quick review to nail a canary common crasher?  I'll take the first LGTM I get
> for speed, but would be interested in follow-on comments.  To answer the
obvious
> question: I'm not putting in a new test for this case, because that test is
> already in progress in a different branch (you folks have all seen it), it is
> *not* simple, and I want to get this in quickly.
> 
> To turn my embarrassment into a teachable moment: One common pattern in the
> download system that I have every intention of keeping as I refactor, is that
> the long-lived core classes (ResourceDispatcherHost, DownloadFileManager,
> DownloadManager) serve as targets for messages aimed at the per-download
objects
> on their threads (DownloadResourceHandler, DownloadFile, DownloadItem), and if
> those objects have gone away, they are supposed to drop those messages on the
> floor.  In my ideal world, that'd be the main thing they did, and the objects
> would do most of the real work.  My recent history deflake commit ignored that
> invariant, and now I'm paying the price.  I invite folks to keep an eye out
for
> this failure mode on future commits (as I'll try to).

[Paweł Hajdan Jr., 2011-01-25 07:27:45]

OK

http://codereview.chromium.org/6327016/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6327016/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:512: // If it's not in
active_downloads_, that means it was cancelled; just
How about removing the download from active_downloads_ instead?

I can't really tell which one is better, but it seems to me removing the
download would be more elegant.

[Randy Smith (Not in Mondays), 2011-01-25 19:11:42]

On 2011/01/25 07:27:45, Paweł Hajdan Jr. wrote:
> OK
> 
>
http://codereview.chromium.org/6327016/diff/1/chrome/browser/download/downloa...
> File chrome/browser/download/download_manager.cc (right):
> 
>
http://codereview.chromium.org/6327016/diff/1/chrome/browser/download/downloa...
> chrome/browser/download/download_manager.cc:512: // If it's not in
> active_downloads_, that means it was cancelled; just
> How about removing the download from active_downloads_ instead?

I'm confused.  This is adapting to the download having been removed from
active_downloads_ by the cancel.  What modification are you suggesting?

> 
> I can't really tell which one is better, but it seems to me removing the
> download would be more elegant.

[Paweł Hajdan Jr., 2011-01-25 19:21:06]

Oops, sorry, I meant to avoid calling OnAllDataSaved.

On Tue, Jan 25, 2011 at 20:11, <rdsmith@chromium.org> wrote:

> On 2011/01/25 07:27:45, Paweł Hajdan Jr. wrote:
>
>> OK
>>
>
>
>
>
http://codereview.chromium.org/6327016/diff/1/chrome/browser/download/downloa...
>
>> File chrome/browser/download/download_manager.cc (right):
>>
>
>
>
>
http://codereview.chromium.org/6327016/diff/1/chrome/browser/download/downloa...
>
>> chrome/browser/download/download_manager.cc:512: // If it's not in
>> active_downloads_, that means it was cancelled; just
>> How about removing the download from active_downloads_ instead?
>>
>
> I'm confused.  This is adapting to the download having been removed from
> active_downloads_ by the cancel.  What modification are you suggesting?
>
>
>
>  I can't really tell which one is better, but it seems to me removing the
>> download would be more elegant.
>>
>
>
>
> http://codereview.chromium.org/6327016/
>

[Randy Smith (Not in Mondays), 2011-01-25 19:37:11]

On 2011/01/25 19:21:06, Paweł Hajdan Jr. wrote:
> Oops, sorry, I meant to avoid calling OnAllDataSaved.

I'm still not sure I follow you.  OnAllDataSaved is called by posting a task
from the FILE thread; we don't have the option of reaching into
active_downloads_ and figuring out whether or not we should call it before
posting the task.  In terms of avoiding the call by other means, we've got an
unavoidable race here--if the cancel is done from the UI thread, a message to
cancel the DownloadFile is posted to the file thread, but could pass the
OnAllDataSaved post going the other way.

(Just noting that the test I put in is "Is download_id in active_downloads_",
not "does download_id lookup a null pointer".  I don't use ContainsKey because
that's a layer on top of the STL, and I prefer to stick with the STL rather than
extra code when I can do the same test in the same number of lines.)

[Paweł Hajdan Jr., 2011-01-26 20:03:47]

LGTM