852868 - Disable channel decoding if the "Content-Encoding" header matches the file extension

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

When downloading a file from a channel whose "Content-Encoding" header matches
the compression type indicated by the file extension (for example, the ".gz"
extension and the "gzip" encoding), the file should be saved to disk with the
specified extension and without decoding it.

This corresponds to the nsIExternalHelperAppService::applyDecodingForExtension
function, the nsIWebBrowserPersist::PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION
flag, and this list of translated encoding names and extensions:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#498

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Marcos, have you made any progress here?

Comment 2

[Marcos Aruj]

Hi Gavin,

Yes. I'll upload a patch today or early tomorrow, so Paolo can check it out and provide some feedback.

Comment 3

[Marcos Aruj]

Hi Paolo,

I have a couple of questions. I'm adding code to DownloadCore.jsm for this bug. In method execute : DCS_execute , I'm checking if the channel instanceof nsIEncodedChannel. If it is, then I can check for content-enconding header matching any of the file types to ignore decoding and set channel.applyConversion = false. 

1) Where do you recommend adding the file extension types for which encoding should be disabled.

2) How can I access the request header for Content-Enconding. I see nsIChannel has some helpers to access other header values, but I'm not sure how to get the Enconding header, or if there's another way I'm missing.

Thanks for your help!

Marcos

Comment 4

[:Paolo Amadini]

(In reply to Marcos Aruj from comment #3)
> 1) Where do you recommend adding the file extension types for which encoding
> should be disabled.

I think that checking nsIExternalHelperAppService::applyDecodingForExtension and
seeing if it returns false should provide the information we need. In the future
we might move all the logic to JavaScript, including the list, but we probably
don't need that for now.

> 2) How can I access the request header for Content-Enconding. I see
> nsIChannel has some helpers to access other header values, but I'm not sure
> how to get the Enconding header, or if there's another way I'm missing.

nsIEncodedChannel has a "contentEncodings" helper that can enumerate all the
encodings listed in the "Content-Encoding" header. For the purpose of this bug,
it seems the first one in the list is the one we should check.

One of the places that execute this check is here:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1458

It seems it checks the extension on the source URI. This is what we should do
for now. In the future we may consider changing that logic to check the
"suggested file name" when we collect that information.

Thanks!

Comment 5

[Marcos Aruj]

Attachment: Bug852868.patch

Comment 6

[Raymond Lee [:raymondlee]]

Comment on attachment 780042 [details] [diff] [review]
Bug852868.patch

Review of attachment 780042 [details] [diff] [review]:
-----------------------------------------------------------------

Please make sure your next patch work with hg because I can't import this one using hg import.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +718,5 @@
> +            // Check if data from source uri should be decoded, depending on the extension and the Content-Enconding
> +            // header before saving or passing off to helper apps.
> +            if (channel instanceof Ci.nsIEncodedChannel) {
> +              let extension = download.source.uri.path.lastIndexOf('.');
> +              extension = download.source.uri.path.substring(extension+1);

You might use nsIURL to get the fileExtension. nsIURL is a subclass of nsIURI
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIURL

@@ +721,5 @@
> +              let extension = download.source.uri.path.lastIndexOf('.');
> +              extension = download.source.uri.path.substring(extension+1);
> +              if (channel.contentEncodings != null) {
> +                let encoding = channel.contentEncodings.getNext();
> +                channel.applyConversion = extHelperAppSvc.applyDecodingForExtension(extension, extension);

Please check whether encoding is not empty before passing into .applyDecodingForExtension

This should be channel.applyConversion = extHelperAppSvc.applyDecodingForExtension(extension, encoding);

Furthermore, I think this should be done onStartRequest but write a test to confirm that.

Comment 7

[Marcos Aruj]

Attachment: Bug-852868.patch

Fixed the issues you mentioned. Moved the encoding detection code to onStartRequest. According to the docs, contentEcondings are available during or after OnStartRequest.

This patch can be imported through HG.

Do we need to add a "gz" file and a new test for this part of the code? I've created one just in case. Let me know.

Comment 8

[Raymond Lee [:raymondlee]]

Comment on attachment 780871 [details] [diff] [review]
Bug-852868.patch

Looks good to me.  Please add a test for your patch!

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to Marcos Aruj from comment #7)
> Created attachment 780871 [details] [diff] [review]
> Bug-852868.patch
> 
> Fixed the issues you mentioned. Moved the encoding detection code to
> onStartRequest. According to the docs, contentEcondings are available during
> or after OnStartRequest.
> 
> This patch can be imported through HG.
> 
> Do we need to add a "gz" file and a new test for this part of the code? I've
> created one just in case. Let me know.

I don't think you need to add a "gz" file because there is a HttpServer in the test_common_initialize() that you can use
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/head.js#435

Comment 10

[Marcos Aruj]

Attachment: Bug-852868.patch

Added test and rebased with the latest code changes from the server, as it was not working anymore.

Comment 11

[:Paolo Amadini]

Comment on attachment 783467 [details] [diff] [review]
Bug-852868.patch

Review of attachment 783467 [details] [diff] [review]:
-----------------------------------------------------------------

The structure looks good, I've commented for some implementation details.

Note that this patch should be rebased on top of bug 852482 when it lands.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +814,5 @@
>      let download = this.download;
>  
> +    let extHelperAppSvc =
> +      Cc["@mozilla.org/uriloader/external-helper-app-service;1"].
> +        getService(Ci.nsIExternalHelperAppService);

Please use defineLazyServiceGetter at the top of the file, with the name "gExternalHelperAppService".

@@ +877,5 @@
>  
> +          // Check if data from source uri should be decoded, depending on the extension and the Content-Enconding
> +          // header before saving or passing off to helper apps.
> +          if (channel instanceof Ci.nsIEncodedChannel) {
> +            let uri = nsIIOService.newURI(download.source.url, null, null);

You may just use channel.URI here.

@@ +878,5 @@
> +          // Check if data from source uri should be decoded, depending on the extension and the Content-Enconding
> +          // header before saving or passing off to helper apps.
> +          if (channel instanceof Ci.nsIEncodedChannel) {
> +            let uri = nsIIOService.newURI(download.source.url, null, null);
> +            let extension = uri.QueryInterface(Ci.nsIURL).fileExtension;

You should check "instanceof Ci.nsIURL", because it may return false, and the function shouldn't fail.

@@ +879,5 @@
> +          // header before saving or passing off to helper apps.
> +          if (channel instanceof Ci.nsIEncodedChannel) {
> +            let uri = nsIIOService.newURI(download.source.url, null, null);
> +            let extension = uri.QueryInterface(Ci.nsIURL).fileExtension;
> +            if (channel.contentEncodings != null) {

To check for null, just check the truth value:

if (channel.contentEncodings)

The same below.

@@ +882,5 @@
> +            let extension = uri.QueryInterface(Ci.nsIURL).fileExtension;
> +            if (channel.contentEncodings != null) {
> +              let encoding = channel.contentEncodings.getNext();
> +              if (encoding != null)
> +                channel.applyConversion = extHelperAppSvc.applyDecodingForExtension(extension, encoding);

nit: braces around single-line blocks

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +1053,5 @@
> +  let download = yield promiseStartDownload(sourceUrl);
> +  yield promiseDownloadStopped(download);
> +
> +  do_check_eq(download.progress, 100);
> +  do_check_eq(download.totalBytes, TEST_DATA_SHORT_GZIP_ENCODED.length);

Add a promiseVerifyContents at the end.

Comment 12

[Marcos Aruj]

Attachment: Bug-852868.patch

New patch with corrections and rebased to the latest trunk.

@Paolo, The promiseVerifyContents call is returning an error, as it compares the decoded content of TEST_DATA_SHORT with TEST_DATA_SHORT_GZIP_ENCODED

I understand the contents should not be decoded right? That's why I am sending TEST_DATA_SHORT_GZIP_ENCODED as the expected content.

Let me know if I'm doing something wrong. 

Thank you.

Comment 13

[:Paolo Amadini]

(In reply to Marcos Aruj from comment #12)
> I understand the contents should not be decoded right? That's why I am
> sending TEST_DATA_SHORT_GZIP_ENCODED as the expected content.

Yes, I think the test fails because there is an issue with the instanceof check
that is done on the channel. Please try something like this (some checks are
reordered just to avoid nesting the statements too much):

if (channel instanceof Ci.nsIEncodedChannel &&
    channel.contentEncodings) {
  let uri = channel.URI;
  if (uri instanceof Ci.nsIURL && uri.fileExtension) {
    let encoding = channel.contentEncodings.getNext();
    if (encoding) {
      channel.applyConversion =
        gExternalHelperAppService.applyDecodingForExtension(
                                  uri.fileExtension, encoding);
    }
  }
}

Comment 14

[:Paolo Amadini]

Attachment: Updated to fix content

This also fixes a problem by limiting test output to printable characters.

Comment 15

[Neil Deakin]

Comment on attachment 791437 [details] [diff] [review]
Updated to fix content

>+  do_check_eq(download.progress, 100);
>+  do_check_eq(download.totalBytes, TEST_DATA_SHORT_GZIP_ENCODED.length);
>+
>+  // Ensure the content matches the encoded test data.  We convert the data to
>+  // string before executing the content check.

We convert the data to *a* string before...


>-      if (contents.length <= TEST_DATA_SHORT.length * 2) {
>+      if (contents.length <= TEST_DATA_SHORT.length * 2 &&
>+          !/[^\x20-\x7E]/.test(contents)) {
>+        // Print the string if it is short and made of printable characters.

The double negative is the expression makes this a bit confusing. Maybe reverse the blocks.

Comment 16

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/3bcc3e774eee

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bcc3e774eee

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Since this apparently fixed bug 865364, can we please request approval to uplift this to Aurora for Fx25?

Comment 19

[:Paolo Amadini]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> Since this apparently fixed bug 865364, can we please request approval to
> uplift this to Aurora for Fx25?

We don't know exactly why this fixed the intermittent failure in Nightly, and we're not sure it will do the same in Aurora (assuming the patch applies cleanly).

It's probably less effort to just disable the tests in Aurora, as the feature is disabled by default and we have extended coverage in Nightly.