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

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!

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.

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 on attachment 780871 [details] [diff] [review] Bug-852868.patch Looks good to me. Please add a test for your patch!

(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

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

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.

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.

(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.

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.

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.