Fix NULL pointer usage, it is P1 crash bug.

Fix NULL pointer usage, it is P1 crash bug.

Just after DownloadControllerAndroidImpl::CreateGETDownload() being called and just before
DownloadControllerAndroidImpl::PrepareDownloadInfo() being run, the Loader object that holds the request pointer could have been removed from the pending_loaders_ map (in resource_dispatcher_host_impl.cc) because of a request cancel.
If that happens, the request pointer will be correctly NULL, but we don't check that.

This scenario is not a race condition since all the three methods are called in the IO thread, but the order of the calling each of the 3 methods can lead to this problem.

BUG=165784
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173504

[Philippe, 2012-12-17 15:20:05]

lgtm (although I'm not a OWNER).

https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (right):

https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:109:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
It's good that we have this DCHECK here. I would add it to the 2 other functions
below.

https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:113: DCHECK(request)
<< "Request to download not found.";
Nit: I would remove these DCHECKs now that we are covered by the if statement
below. As we saw this case looks sane although it's rare.

[felipeg, 2012-12-17 15:21:33]

https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (right):

https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:109:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2012/12/17 15:20:06, Philippe wrote:
> It's good that we have this DCHECK here. I would add it to the 2 other
functions
> below.

Done.

https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:113: DCHECK(request)
<< "Request to download not found.";
On 2012/12/17 15:20:06, Philippe wrote:
> Nit: I would remove these DCHECKs now that we are covered by the if statement
> below. As we saw this case looks sane although it's rare.

Done.

[joth, 2012-12-17 16:15:35]

lgtm (also not an owner)



On 17 December 2012 07:21, <felipeg@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/11592012/diff/1/**
>
content/browser/android/**download_controller_android_**impl.cc<https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/android/download_controller_android_impl.cc>
> File content/browser/android/**download_controller_android_**impl.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/11592012/diff/1/**
>
content/browser/android/**download_controller_android_**impl.cc#newcode109<https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/android/download_controller_android_impl.cc#newcode109>
> content/browser/android/**download_controller_android_**impl.cc:109:
> DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::IO)**);
> On 2012/12/17 15:20:06, Philippe wrote:
>
>> It's good that we have this DCHECK here. I would add it to the 2 other
>>
> functions
>
>> below.
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/11592012/diff/1/**
>
content/browser/android/**download_controller_android_**impl.cc#newcode113<https://chromiumcodereview.appspot.com/11592012/diff/1/content/browser/android/download_controller_android_impl.cc#newcode113>
> content/browser/android/**download_controller_android_**impl.cc:113:
> DCHECK(request) << "Request to download not found.";
> On 2012/12/17 15:20:06, Philippe wrote:
>
>> Nit: I would remove these DCHECKs now that we are covered by the if
>>
> statement
>
>> below. As we saw this case looks sane although it's rare.
>>
>
> Done.
>
>
thanks! I was about to ask the same.


>
https://chromiumcodereview.**appspot.com/11592012/<https://chromiumcodereview...
>

[Jay Civelli, 2012-12-17 16:24:01]

lgtm

[commit-bot: I haz the power, 2012-12-17 16:38:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felipeg@chromium.org/11592012/1002

[commit-bot: I haz the power, 2012-12-17 19:11:37]

Change committed as 173504