Closed Bug 852868 Opened 7 years ago Closed 6 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 --- affected
firefox26 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 4 obsolete files)

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
Blocks: 881062
No longer blocks: jsdownloads
Assignee: nobody → marcos
Marcos, have you made any progress here?
Hi Gavin,

Yes. I'll upload a patch today or early tomorrow, so Paolo can check it out and provide some feedback.
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
(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!
Attached patch Bug852868.patch (obsolete) — Splinter Review
Attachment #780042 - Flags: feedback?(raymond)
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.
Attachment #780042 - Flags: feedback?(raymond) → feedback+
Attached patch Bug-852868.patch (obsolete) — Splinter Review
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.
Attachment #780042 - Attachment is obsolete: true
Attachment #780871 - Flags: review?(raymond)
Comment on attachment 780871 [details] [diff] [review]
Bug-852868.patch

Looks good to me.  Please add a test for your patch!
Attachment #780871 - Flags: review?(raymond) → review+
Attachment #780871 - Flags: review+ → feedback+
(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
Attached patch Bug-852868.patch (obsolete) — Splinter Review
Added test and rebased with the latest code changes from the server, as it was not working anymore.
Attachment #780871 - Attachment is obsolete: true
Attachment #783467 - Flags: review?(paolo.mozmail)
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.
Attachment #783467 - Flags: review?(paolo.mozmail)
Attached patch Bug-852868.patch (obsolete) — Splinter Review
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.
Attachment #783467 - Attachment is obsolete: true
Attachment #790537 - Flags: review?(paolo.mozmail)
(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);
    }
  }
}
This also fixes a problem by limiting test output to printable characters.
Assignee: marcos → paolo.mozmail
Attachment #790537 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #790537 - Flags: review?(paolo.mozmail)
Attachment #791437 - Flags: review?(enndeakin)
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.
Attachment #791437 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/3bcc3e774eee
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Since this apparently fixed bug 865364, can we please request approval to uplift this to Aurora for Fx25?
(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.
You need to log in before you can comment on or make changes to this bug.