Fix content-encoding handling with buggy servers.

Fix content-encoding handling with buggy servers.

Some servers will serve compressed filetypes with "content-encoding: gzip"
because the file's contents were gzipped, even though the server is not
re-encoding the contents at all. The code in Filter::FixupEncodingTypes()
attempts to work around this by inferring whether the file is a gzip file with
bogus encoding based on the file extension in the URL, but this doesn't work
when the file being downloaded comes from a page whose extension doesn't reveal
the downloaded file type.

This change adds code to FixupEncodingTypes to also consider the download
filename, supplied by the "content-disposition" header, if any. This means that
we now behave correctly in the case where:

-> GET /foo.php?download
<- HTTP/1.1 200 OK
<- ...
<- Content-Disposition: attachment;filename="foo.tar.gz"
<- ...
<- Content-Encoding: gzip

BUG=83292
TEST=unit,trybot

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

[Elly Fong-Jones, 2014-03-20 20:10:40]

PTAL? :)

[asanka, 2014-03-20 23:07:16]

LGTM modulo character encoding fun.

https://codereview.chromium.org/206503006/diff/1/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/206503006/diff/1/net/filter/filter.cc#newcode178
net/filter/filter.cc:178: filename =
base::FilePath().AppendASCII(server_filename);
Nit: server_filename is UTF-8, not necessarily ASCII, if it's obtained via
HttpContentDisposition. The encoding of url may not be ASCII either. 

Passing a non-ASCII string to AppendASCII isn't terrible since we are only
interested in extracting the extension and matching it against ASCII strings.
However, it'll trip a DCHECK on a DCHECK enabled build and relies on
implementation quirks to work.

[Elly Fong-Jones, 2014-03-21 14:53:56]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2014-03-21 14:54:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/206503006/20001

[commit-bot: I haz the power, 2014-03-21 15:17:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 15:17:18]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[asanka, 2014-03-21 15:57:07]

Hold!

It occurred to me that the correct way to do this would be to use the URL and
the Content-Disposition header and call GenerateFilename() in net_util.h. That
would take care of most of the ugly name generation logic.

[Elly Fong-Jones, 2014-03-21 17:41:55]

On 2014/03/21 15:57:07, asanka wrote:
> Hold!
> 
> It occurred to me that the correct way to do this would be to use the URL and
> the Content-Disposition header and call GenerateFilename() in net_util.h. That
> would take care of most of the ugly name generation logic.

Hrm. We have the URL, the content-disposition, the mime type, and we can guess
at the referrer charset and leave default_name and suggested_name blank. I like
it.

[asanka, 2014-03-21 17:57:26]

On 2014/03/21 17:41:55, Elly Jones wrote:
> On 2014/03/21 15:57:07, asanka wrote:
> > Hold!
> > 
> > It occurred to me that the correct way to do this would be to use the URL
and
> > the Content-Disposition header and call GenerateFilename() in net_util.h.
That
> > would take care of most of the ugly name generation logic.
> 
> Hrm. We have the URL, the content-disposition, the mime type, and we can guess
> at the referrer charset and leave default_name and suggested_name blank. I
like
> it.

Except leave the mime_type out. The MIME type lookup requires IO and can't be
done on the IO thread. Also we already looked at the MIME type before we check
the filename. At this point we are only interested in knowing whether the URL or
Content-Disposition indicates that the resource should be treated as a gzip
stream rather than a gzip encoded stream.

[Elly Fong-Jones, 2014-03-21 18:08:26]

On 2014/03/21 17:57:26, asanka wrote:
> On 2014/03/21 17:41:55, Elly Jones wrote:
> > On 2014/03/21 15:57:07, asanka wrote:
> > > Hold!
> > > 
> > > It occurred to me that the correct way to do this would be to use the URL
> and
> > > the Content-Disposition header and call GenerateFilename() in net_util.h.
> That
> > > would take care of most of the ugly name generation logic.
> > 
> > Hrm. We have the URL, the content-disposition, the mime type, and we can
guess
> > at the referrer charset and leave default_name and suggested_name blank. I
> like
> > it.
> 
> Except leave the mime_type out. The MIME type lookup requires IO and can't be
> done on the IO thread. Also we already looked at the MIME type before we check
> the filename. At this point we are only interested in knowing whether the URL
or
> Content-Disposition indicates that the resource should be treated as a gzip
> stream rather than a gzip encoded stream.

Alright, how about now? :)

[asanka, 2014-03-21 18:23:14]

https://codereview.chromium.org/206503006/diff/40001/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/206503006/diff/40001/net/filter/filter.cc#new...
net/filter/filter.cc:180: net::GetSuggestedFilename(url, disposition, "UTF-8",
"", "", "");
Nit: already in net namespace.

Also use GenerateFilename() instead. It will return a FilePath directly and
handle the quirks of converting from string16 to FilePath::StringType.

[Elly Fong-Jones, 2014-03-21 19:02:33]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2014-03-21 19:03:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/206503006/60001

[commit-bot: I haz the power, 2014-03-21 19:06:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 19:06:57]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[Elly Fong-Jones, 2014-03-21 19:13:21]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2014-03-21 19:14:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/206503006/60001

[commit-bot: I haz the power, 2014-03-22 00:47:03]

Change committed as 258729