Closed Bug 1390708 Opened 2 years ago Closed 2 years ago

.hqx files are all corrupted when downloaded with FF unless going through SAVE AS...

Categories

(Core :: Networking, defect, P2)

58 Branch
x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: mozilla, Assigned: schien)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Following a link by left clicking or accessing a direct URL (e.g.: copy/paste an URL in the URL bar) to download a .hqx file when using any version of Firefox always results in a corrupted download.  Right-clicking the same URL in a web page and choosing "Save as..." always results in a non-corrupted download.

TRY IT FOR YOURSELF:

Copy/paste this random HQX file on ARCHIVE.ORG in your URL bar and press return to begin download: https://web.archive.org/web/20051226193648if_/http://semicolon.jp:80/ussy/soundset/archives/Apple_soundset.sit.hqx




Actual results:

This results in a corrupted HQX file when done with Firefox.  Now try and do the exact same thing with any other browser, e.g.: Internet Explorer, Chrome or Safari and compare the HQX file size with the one downloaded with Firefox and you'll see the Firefox one is corrupted.

The result file size with Firefox is: 2 107 586 bytes (corrupted)
The result file size with any other browser is: 2 173 122 bytes (not corrupted)




Expected results:

Unless a .hqx file URL is right-clicked and "Save as..." is chosen, Firefox will not download the file correctly.  This problem has indirectly been reported for probably as far back as Firefox existed, some 15 years ago.  See this 15 years old bug: https://bugzilla.mozilla.org/show_bug.cgi?id=204744 or this 9 years old bug: https://bugzilla.mozilla.org/show_bug.cgi?id=438300

Under Firefox 54 it still does not download .hqx file attachments right under most scenarios as mentioned above.
Forgot to mention if it's not obvious but IN STEPS TO REPRODUCE:

2) Left-clicking the above ARCHIVE.ORG URL with Firefox results in a corrupted file with a size of 2 042 050 bytes.
3) Left-clicking A SECOND time also results in a corrupted file but with a size of 2 173 122 bytes.
4) Right-clicking and choosing "Save as..." results in a correctly downloaded file which size is: 2 074 818 bytes.

Firefox always saving *almost* complete HQX files when left-clicking is scary, but file size of the iterated attempts VARYING is beyond me. What the actual F?... Oh, and 15 years.  Fifteen YEARS.
Typo... 3) and 4) sizes are reversed, should read:
3) size 2 074 818 bytes
4) size 2 173 122 bytes
Component: Untriaged → File Handling
Can't reproduce, it works for me on macOS 10.12 with Nighty 57. Please reopen if you have more specific steps to reproduce with 57.
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
Hi.  I'm under Windows 7 x64 and it's definitely reproduceable.  Try the steps on Windows please.
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Steps to reproduce:

1) Copy/paste this HQX file in your URL bar and press ENTER: https://web.archive.org/web/20051226193648if_/http://semicolon.jp:80/ussy/soundset/archives/Apple_soundset.sit.hqx

It downloads 2.1MB file.

2) Repeat step 1.  It instantly downloads a 1.9MB file.

This results in a corrupted (varying size, incomplete) HQX file when done with Firefox, but is non-corrupted, full size on all other browsers.

The only way to download that file without Firefox stripping a variable number of KB from the file is to right-click and choose "SAVE AS".

I feel like this is an important bug that has to be fixed.  I don't understand why an ingineer would only test on his Mac and assume that this bug doesn't exist.  Do you want me to upload a video?
Version: 54 Branch → 55 Branch
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #3)
> Please reopen if you have more specific steps to reproduce with 57.

So have you tried with 57 and does it happen there as well?
Here's a screenshot of the Nightly Build 56.0b11 still happens and I'd bet my shirt it still happens on 57, but I couldn't find where to download that version.  Firefox website gives me the 56.0b11 version instead of 57 : http://oi68.tinypic.com/2wh3t47.jpg

https://www.mozilla.org/en-US/firefox/channel/desktop/
Nevermind, I found the Nightly Build 57.0a1 (2017-09-13)

aaannnd.... same thing happens.

Screenshot: http://oi63.tinypic.com/k0k5ea.jpg
I went the extra mile testing for you and under my Mac OS X 10.10 virtual machine, I installed Nightly Build 57.0a1 (2017-09-15) and it shows that the issue is only happening on Windows and NOT on Mac.  See the following Mac screenshot showing that downloading the same file 5x writes the same file size: http://oi67.tinypic.com/x3vlmx.jpg

But it still is 100% reproduceable under Windows; Just download the same HQX file twice and you'll see the bug immediately.  See with Nightly Build 57.0a1 (2017-09-15) today, same thing happening as with all versions of Firefox I've seen over the last decade: http://oi65.tinypic.com/wmb8jn.jpg
I reproduced this using the link in comment 0 with Firefox 55 on Windows 7.

I'm not sure why this is happening - it's more likely that the Networking team will be able to figure this out.
Status: UNCONFIRMED → NEW
Component: File Handling → Networking
Ever confirmed: true
Product: Firefox → Core
(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #6)
> (In reply to Panos Astithas [:past] (56 Regression Engineering Owner)
> (please ni?) from comment #3)
> > Please reopen if you have more specific steps to reproduce with 57.
> 
> So have you tried with 57 and does it happen there as well?

Yes.  So do you care to check now? BTW, I'm not the only one reporting this, it has been reported for as far back as 15 years ago.  Also, somebody just reported it as reproduceable, see the previous comment to this one.  Please consider bubbling this upwards until an engineer fixes it!
Still happening in 58.0a1 Nightly, is anybody even considering working on fixing that +DECADE OLD bug?

http://oi63.tinypic.com/3160ope.jpg
Version: 55 Branch → 58 Branch
Try reproduce this today, can only reproduce it on Windows but not OSX.

Capture corresponding MOZ_LOG for left click URL and right click to "Save As".
https://gist.github.com/anonymous/c844b13bc36d22198bdc1ac38348da36
Here is the log with HelperAppService log turned on.
https://gist.github.com/anonymous/a5dbc54de9241652ccbd92170660dbca

This issue is related to our BinHex decoder support. The "corrupted" file is actually the decoded content of the .hqx file.
We cannot reproduce this issue on OSX because BinHex Decoder is only enabled on non-OSX platform.
https://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/netwerk/build/nsNetModule.cpp#46

When left click the URL, Necko detect the MIME type "application/mac-binhex40" and try stream decoding the body of HTTP response. However the decoded content still cannot be handled by Gecko, which triggers file download by nsExternalHelperAppService. Therefore, the decoded "application/x-stuffit" content is saved but the filename is still end with ".hqx".

When using "save as", the body of HTTP response is directly saved to file without going through BinHex decoder.

Three solution in my head:
 1) remove the ".hqx" from the saved filename since we already complete the binhex decoding
 2) save the original binhex content
 3) remove the binhex support

@mcmanus, do you have any suggestion?
Flags: needinfo?(mcmanus)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #14)
> This issue is related to our BinHex decoder support. The "corrupted" file is
> actually the decoded content of the .hqx file.
> We cannot reproduce this issue on OSX because BinHex Decoder is only enabled
> on non-OSX platform.
> 
> When left click the URL, Necko detect the MIME type
> "application/mac-binhex40" and try stream decoding the body of HTTP
> response. However the decoded content still cannot be handled by Gecko,
> which triggers file download by nsExternalHelperAppService. Therefore, the
> decoded "application/x-stuffit" content is saved but the filename is still
> end with ".hqx".
> 
> When using "save as", the body of HTTP response is directly saved to file
> without going through BinHex decoder.
> 
> Three solution in my head:
>  1) remove the ".hqx" from the saved filename since we already complete the
> binhex decoding
>  2) save the original binhex content
>  3) remove the binhex support
> 
> @mcmanus, do you have any suggestion?

Thank you Shih-Chiang Chien !

Now, my question is: Ain't it ridiculous that BinHex decoding support is enabled only on non-Mac platforms when most HQX files are Mac only files that contain a resource fork that instantly gets dropped/blackholed when extracted by a non-Mac platform? What is even the point of an integrated, automated non-Mac BinHex decoder in Firefox anyway?

Please somebody, PLEASE implement solution 3) and let's all turn the page on this utter nonsense.

BinHex files always have been decoded via your favorite decompression utility anyway, such as: The Unarchiver, Stuffit Expander, etc...
it sounds like 3 matches chrome? If so I think that's fine for this ..
Flags: needinfo?(mcmanus)
...or at least implement solution 2) if not 3).  That would work too, as long as all the bytes in the file are saved untouched.
Assignee: nobody → schien
Didn't find BinHex support in Chrome source code, should be safe to remove it.
@mcmanus I'm thinking about sending out an "Intend to Unship" mail on dev-platform, maybe also thunderbird mailing list. Do you think it is worth to do that?
Flags: needinfo?(mcmanus)
sure.. please save us all the confusion and clarify that scope of what's being unshipped (i.e. decoding for non apple platforms).  We could fix this, but mostly this is just exposure to parsing bugs for little value.
Flags: needinfo?(mcmanus)
What "value" is there in a feature that serves absolutely no useful purpose? Decoding classic Mac OS HQX files would destroy them by stripping their resource fork anyways and render them 100% useless, so why would such a strange feature even exist in non-Mac Firefox to begin with?? Just have Firefox not process or decode the HQX files, simply saving them untouched is a PERFECT solution.  Just think of a HQX file like a plain ASCII text file, no processing required.
Priority: -- → P2
Whiteboard: [necko-triaged]
> What "value" is there in a feature that serves absolutely no useful purpose?

There was sort of a purpose originally: If you have HTML inside binhex, we can render the HTML directly, just like we decompress "content-encoding:gzip" automatically and render the HTML contained therein.

The obvious differences are that we're guessing here, not being told by the server, that the decompression is worth doing, _and_ messing up the resulting filenames (which is worth looking into on its own).

That said, you should look at the reason the code is there: bug 81352.  Please ensure that that use case does not get broken.
The other option, of course, is to modify the stream converter setup such that we have converters that are marked as "not visible" for purposes of the uriloader but can still be instantiated by someone who explicitly wants to do so (like the mailnews code does, or did as of right after bug 81352 was fixed).
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #23)
> There was sort of a purpose originally: If you have HTML inside binhex, we
> can render the HTML directly, just like we decompress
> "content-encoding:gzip" automatically and render the HTML contained therein.

So what you're saying is, for instance, years (decades?) ago, some Apache setups were sending BinHex compressed web pages as opposed to nowadays, where probably all Apache setups use GZIP deflating.  Got it.  That's nice, except, it really shouldn't decode BinHex when it's an attachment/download.  I would understand it trying to decode BinHex if it was to be displayed in the browser, but if it's saving a file to the hard disk, it really shouldn't touch it at all and it should save it AS IS.

Bug 81352 was reported some 17 (SEVENTEEN) years ago... wow.
> it really shouldn't decode BinHex when it's an attachment/download.

Sure.  The problem is that we can't decide whether we can render the thing inline until we undo the binhex encoding...

Anyway, I think restricting this behavior to mailnews, assuming mailnews still wants it, is pretty reasonable.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #26)
> The problem is that we can't decide whether we can render the thing
> inline until we undo the binhex encoding...

How come? Can't you take a look at headers sent from Apache? It will say if it's an attachment/download or a web page, no?
I just realized not all PHP scripts correctly set the headers, so obviously, this won't work everywhere.  Got it.
Without looking at it in detail, TB appears to be using the stream decoder's BinHex option:
https://dxr.mozilla.org/comm-central/search?q=APPLICATION_BINHEX
Combined with the feedback on dev-platform mailing list [1], remove the BinHex support from mozilla-central is a reasonable way to continue. Mailnews can decide whether they want to keep this code in comm-central.

@jcranmer, what's the best way to remove BinHex stream converter from mozilla-central while keeping it in comm-central? Will it cause any trouble next time comm-central update from mozilla-central if I remove the nsBinHexDecoder.cpp and .h from mozilla-central?

[1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/F1DUJc754jI
Flags: needinfo?(Pidgeot18)
Attachment #8917242 - Flags: review?(mcmanus)
Attachment #8917243 - Flags: review?(mcmanus)
Comment on attachment 8917243 [details]
Bug 1390708 - remove nsBinHexDecoder

https://reviewboard.mozilla.org/r/188258/#review200904
Attachment #8917243 - Flags: review?(mcmanus) → review+
Comment on attachment 8917242 [details]
Bug 1390708 - remove BinHex support from stream converter

https://reviewboard.mozilla.org/r/188256/#review200906
Attachment #8917242 - Flags: review?(mcmanus) → review+
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eed77d18d006
remove BinHex support from stream converter r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/5638d0a184ff
remove nsBinHexDecoder r=mcmanus
https://hg.mozilla.org/mozilla-central/rev/eed77d18d006
https://hg.mozilla.org/mozilla-central/rev/5638d0a184ff
Status: NEW → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1417187
Flags: needinfo?(Pidgeot18)
You need to log in before you can comment on or make changes to this bug.