Create a Null DownloadRequestHandle for use by SavePackage DownloadItems.

Create a Null DownloadRequestHandle for use by SavePackage DownloadItems.

This will mean that when things like cancel are done on downloads resulting
from a Save Page As ..., chrome will not crash.  They also won't do anything,
but that's the previous (broken :-{) behavior.

BUG=103590
R=cbentzel@chromium.org


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

[Randy Smith (Not in Mondays), 2011-11-10 16:32:18]

Chris, PTAL.  A couple of notes about design choices:

* I believe that the root cause for 103590 is a user choosing "Cancel" on a
download associated with a save page while it's in process.  I spent some time
trying to reproduce this behavior yesterday, but failed (couldn't find a server
where the save page download, happening right after the visiting of the page and
hence presumably mostly from cache, would be slow enough).  However, I'm certain
that choosing cancel on a save page download would cause this behavior, and I
eventually decided that that was enough to move forward with this fix.

* I also spent some time trying to create a test for this case yesterday, and
concluded that it would require either moderately deep modifications to
URLRequestMockHttpJob or adding a bag on the side of URLRequestSlowDownloadJob. 
Those classes are getting close to the point where they could use a
restructuring (Eric's been agitating for that for a while) so I decided to back
off and put it on the long-term list.  

* I originally was going to include an asserting version of
DownloadRequestHandle for history derived DownloadItems, but that breaks the fix
I already put in to check for a null DownloadRequestHandle pointer.  The right
way to fix this is to get rid of the dependency in MatchesQuery on
GetTabContents(), and use an asserting version of the class, and that's in
progress, but it's running into problems changing the signature of
GetAcceptLangs().

* Canceling a DownloadItem associated with a save page hasn't ever done
anything, and as of this CL, still won't.  This is clearly broken, but even a
hacky fix would be fairly painful, and a clean fix is NP complete ^W^W^H
equivalent to unifying SavePackage and the download system.

[Randy Smith (Not in Mondays), 2011-11-11 16:13:36]

Ben, could you take a look?  Chris' and my schedules over the past two days
didn't jibe up.  Make sure to read the introductory comment--it's got a lot of
background thoughts on a simple change.

Chris: I'd still like your feedback when you get a chance, but I won't hold
commit on your review.

[benjhayden, 2011-11-11 16:30:59]

http://codereview.chromium.org/8519008/diff/1/content/browser/download/downlo...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8519008/diff/1/content/browser/download/downlo...
content/browser/download/download_item.cc:736: request_handle_->CancelRequest();
Why not just if(request_handle_) here?

[Randy Smith (Not in Mondays), 2011-11-11 16:35:16]

On 2011/11/11 16:30:59, b s h wrote:
>
http://codereview.chromium.org/8519008/diff/1/content/browser/download/downlo...
> File content/browser/download/download_item.cc (right):
> 
>
http://codereview.chromium.org/8519008/diff/1/content/browser/download/downlo...
> content/browser/download/download_item.cc:736:
request_handle_->CancelRequest();
> Why not just if(request_handle_) here?

You could be asking one of two questions: Why don't I do more to avoid tripping
over a null pointer, and why didn't I just do that rather than put in a complex
nulling class.  I put in the class to try and catch any possible future gotcha
in this area that I haven't thought of--after having two high priority bugs land
in my lap from moving to fast on a refactor CL, I want some belt and suspenders
:-{.  I would have liked to put in a class that DCHECK'd out in the history case
(and started by doing so) but the history case (currently)  needs access to
GetTabContents() (though I've got a CL in flight to let me fix that :-}) so I
can't use the large, economy sledgehammer I originally wanted to.

Does that answer your question or am I confused?

[benjhayden, 2011-11-11 16:39:15]

LGTM
I was wondering how many other places there might be that just needed an
if(request_handle_), but this is safer than hoping we can find all of them.

[cbentzel, 2011-11-15 11:47:56]

LGTM

http://codereview.chromium.org/8519008/diff/7001/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8519008/diff/7001/content/browser/download/dow...
content/browser/download/download_item.cc:713: if (request_handle_.get())
Can you remove the NULL check here?

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

http://codereview.chromium.org/8519008/diff/7001/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8519008/diff/7001/content/browser/download/dow...
content/browser/download/download_item.cc:713: if (request_handle_.get())
On 2011/11/15 11:47:56, cbentzel wrote:
> Can you remove the NULL check here?

No, I can't, though I'd like to and I have another CL working in that direction.
 The problem is that this CL only puts in a null request handle for Save Page
download items, not History download items (see first comment for context).  And
GetTabContents() is called when a search through DownloadItems is done.  So
removing the null check would result in a repeat of the original release blocker
crasher.

[cbentzel, 2011-11-15 19:53:23]

http://codereview.chromium.org/8519008/diff/7001/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8519008/diff/7001/content/browser/download/dow...
content/browser/download/download_item.cc:713: if (request_handle_.get())
On 2011/11/15 19:25:39, rdsmith wrote:
> On 2011/11/15 11:47:56, cbentzel wrote:
> > Can you remove the NULL check here?
> 
> No, I can't, though I'd like to and I have another CL working in that
direction.
>  The problem is that this CL only puts in a null request handle for Save Page
> download items, not History download items (see first comment for context). 
And
> GetTabContents() is called when a search through DownloadItems is done.  So
> removing the null check would result in a repeat of the original release
blocker
> crasher.  

Thanks. It seems very confusing that it's checked for in some cases and not
others, just due to the knowledge that certain portions won't be called based on
the source of the downloaditem. At least put a comment in the code for why the
NULL check is needed here (or why it isn't needed in the other cases).

[Randy Smith (Not in Mondays), 2011-11-15 21:49:49]

http://codereview.chromium.org/8519008/diff/7001/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8519008/diff/7001/content/browser/download/dow...
content/browser/download/download_item.cc:713: if (request_handle_.get())
On 2011/11/15 19:53:23, cbentzel wrote:
> On 2011/11/15 19:25:39, rdsmith wrote:
> > On 2011/11/15 11:47:56, cbentzel wrote:
> > > Can you remove the NULL check here?
> > 
> > No, I can't, though I'd like to and I have another CL working in that
> direction.
> >  The problem is that this CL only puts in a null request handle for Save
Page
> > download items, not History download items (see first comment for context). 
> And
> > GetTabContents() is called when a search through DownloadItems is done.  So
> > removing the null check would result in a repeat of the original release
> blocker
> > crasher.  
> 
> Thanks. It seems very confusing that it's checked for in some cases and not
> others, just due to the knowledge that certain portions won't be called based
on
> the source of the downloaditem. At least put a comment in the code for why the
> NULL check is needed here (or why it isn't needed in the other cases). 

Done.

[commit-bot: I haz the power, 2011-11-15 22:20:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/8519008/10002

[commit-bot: I haz the power, 2011-11-16 00:00:47]

Change committed as 110216