Fix crash bug when attempting to download a url with unsupported scheme, e.g....

Fix crash bug when attempting to download a url with unsupported scheme, e.g. 'data:', by 'Alt + Click'.

Note: This change only fixes crash. Supporting a download for URL with 'data:' scheme is not implemented in the change.

BUG=24846
TEST=See the first entry of bug 24846 and confirm that the browser won't crash.

[darin (slow to review), 2009-10-16 15:50:44]

data: URLs don't get downloaded because we eliminated the URLRequestDataJob.  I
think we probably need to bring that back.  WebURLLoaderImpl currently handles
data: URLs in the renderer, but downloading requires the browser to be able to
do all of the resource loading w/o involving the renderer.

http://codereview.chromium.org/287002/diff/2001/3001
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/287002/diff/2001/3001#newcode695
Line 695: if (!URLRequest::IsHandledURL(url)) {
I think this needs to be after LoadChromePlugins, which is what loads Gears. 
Gears can intercept some URL loads, and in particular it can inject support for
new URL schemes.

[hayato-do-not-use-this-account, 2009-10-19 04:29:00]

Thank you for the review and useful info!

http://codereview.chromium.org/287002/diff/2001/3001
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/287002/diff/2001/3001#newcode695
Line 695: if (!URLRequest::IsHandledURL(url)) {
On 2009/10/16 15:50:44, darin wrote:
> I think this needs to be after LoadChromePlugins, which is what loads Gears. 
> Gears can intercept some URL loads, and in particular it can inject support
for
> new URL schemes.

Done. Thank you.

[tony, 2009-10-19 17:49:35]

On 2009/10/16 15:50:44, darin wrote:
> data: URLs don't get downloaded because we eliminated the URLRequestDataJob. 
I
> think we probably need to bring that back.  WebURLLoaderImpl currently handles
> data: URLs in the renderer, but downloading requires the browser to be able to
> do all of the resource loading w/o involving the renderer.

Hmm, I see.  It would be nice if we could avoid it because for long URLs, it
causes an extra copy in the browser process.  I wonder if we also store data:
urls in history?  It might cause slowness like in http://crbug.com/24947 .

I'm able to drag an image that is a data URL to the desktop.  I wonder if we can
do something similar for the alt+click case (oddly enough, I can't seem to save
the page either when it's a data url).

[hayato-do-not-use-this-account, 2009-10-20 08:40:05]

FYI. 
Roland is working on handling data URL in another change.
Please see: http://codereview.chromium.org/297003


darin, tomy,
If this change is okay, could you commit it on behave of me?
I don't have a commit access.

[tony, 2009-10-20 17:17:28]

Landed in r29524.

[darin (slow to review), 2009-10-20 17:44:42]

On Mon, Oct 19, 2009 at 10:49 AM, <tony@chromium.org> wrote:

> On 2009/10/16 15:50:44, darin wrote:
>
>> data: URLs don't get downloaded because we eliminated the
>> URLRequestDataJob.
>>
> I
>
>> think we probably need to bring that back.  WebURLLoaderImpl currently
>> handles
>> data: URLs in the renderer, but downloading requires the browser to be
>> able to
>> do all of the resource loading w/o involving the renderer.
>>
>
> Hmm, I see.  It would be nice if we could avoid it because for long URLs,
> it
> causes an extra copy in the browser process.  I wonder if we also store
> data:
> urls in history?  It might cause slowness like in http://crbug.com/24947 .
>
> I'm able to drag an image that is a data URL to the desktop.  I wonder if
> we can
> do something similar for the alt+click case (oddly enough, I can't seem to
> save
> the page either when it's a data url).
>
>
Roland's patch just gives the browser the ability to load data: URLs.  It
does not
take away WebURLLoaderImpl's shortcut for loading data: URLs in-process.

I think that's probably OK given how simple the proposed URLRequestDataJob
is.

-Darin


>
>
> http://codereview.chromium.org/287002
>

[hayato-do-not-use-this-account, 2009-10-21 03:43:38]

Thank you for committing the change.

On 2009/10/20 17:17:28, tony wrote:
> Landed in r29524.