1463809 - When downloading a RAR blob, Firefox no longer asks if you want to save the file or open it

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P1)

Description

[Marco Castelluccio [:marco]]

Not sure if this is the right component.

Comment 1

[Marco Castelluccio [:marco]]

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9dd8b6f551abb9d1e3ecb3b68eba4ee683371376&tochange=e933d5d558ec8e57bdb1e6f302a180d68ae850c5

Comment 2

[u580221]

Just as an end user, I suggest that firefox should guess based on file extension for application/octet-stream - or alternatively by downloading parts of the file and peeking at the file header.

Comment 3

[Jan Horak [:jhorak]]

Hm, for me the Firefox 60 has same behaviour as nightly, for both I see:

You have chosen to open:
   ...
   ...
Would you like to save this file?

Comment 4

[Jan Horak [:jhorak]]

Okay, the problem is that content-type has higher priority than file extension [1]. Before the bug 1427700 landed we could return invalid (but some) mimeinfo for the application/octet-stream content type, so Open with dialog showed. After the fix landed we return generic mimeinfo which leads to Save/Cancel dialog.

What I believe we need to do is to use file extension to determine the right mimeinfo in case of application/octet-stream content type (like win does [2]).

[1] https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/uriloader/exthandler/unix/nsOSHelperAppService.cpp#1453
[2] https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/uriloader/exthandler/win/nsOSHelperAppService.cpp#402

Comment 5

[Jan Horak [:jhorak]]

Attachment: Bug 1463809 - Don't lookup mimeinfo of application/octet-stream;

Mimeinfo returned for the generic application/octet-stream content
type is usually irrelevant to the file extension, getting mimeinfo
from extension itself is more reliable.

Review commit: https://reviewboard.mozilla.org/r/246462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246462/

Comment 6

[:Paolo Amadini]

Comment on attachment 8980297 [details]
Bug 1463809 - Don't lookup mimeinfo of application/octet-stream;

https://reviewboard.mozilla.org/r/246462/#review252886

Comment 7

[:Paolo Amadini]

This leads to failures in "browser_unknownContentType_dialog_layout.js". Jan, do you have time to look into these?

Given the merge day is getting closer, and system integration issues generally require more time to get sufficient testing in the field, we should either land this soon or revert bug 1427700 in the meantime.

Comment 8

[Jan Horak [:jhorak]]

I've noticed the test failure but I'm not able to reproduce it in Fedora ATM. I'm curious about the test environment because it looks like the .pif file extension is associated with some mimetype.

I've made the test to trying to download .bin file and that seems to be okay for linux [2], but obviously bad for Windows. Any suggestions how to fix this?

[1] https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/tests/browser/browser_unknownContentType_dialog_layout.js#15

[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=51f36ccc736a9f988e98efd6033364b9fc0e0850&selectedJob=181747574

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression (from bug 1427700), tracking for 62. We could probably still take an uplift during beta.

Comment 10

[:Paolo Amadini]

The test failure may involve an actual regression, or at least a behavior change that may be correct but we don't understand yet. We should thus back out bug 1427700 from Beta. I think we can keep it in Nightly while we fix this bug, and this will give us more testing time for the final file handling situation on Linux.

What happens here is that we get a different dialog type on Linux for the ".pif" extension with the "application/octet-stream" type. On Windows, this is an executable file and should not show an "open with" dialog for this reason alone. On other platforms, I'm not quite sure what the logic intended to be... by looking at the code, the "openWithDefaultOK" in "nsHelperAppDlg.js" checks "hasDefaultHandler" to determine if we can show an application choice at all.

Maybe this is what changed here in the case of the "application/octet-stream" type, and there may be opportunities for simplification. Also note that this code considers "application/x-msdownload" in the same way, so it seems to me the platform code should also behave in the same way for this file type.

Jan, can you take a look at this on Linux? What do you think should be the right logic there?

We may need to add more test cases for the different dialog types, the two we have now don't seem exhaustive.

Comment 11

[:Paolo Amadini]

Filed bug 1472105 to back out bug 1427700.

Comment 12

[Mike Taylor [:miketaylr]]

(In reply to :Paolo Amadini from comment #11)
> Filed bug 1472105 to back out bug 1427700.

With that fixed, this should be Nightly-only now (I believe...).

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 63, Jan did you have time to have a look at the Linux logic for this bug? Thanks

Comment 14

[Jan Horak [:jhorak]]

Sorry for the delay, I've been on vacation. I think the problem lies there [1]. Previously before the fix for [2] (Don't return valid HandlerApp for the application/octet-stream content type) landed we never made it to asking the extras [3] because simply gio returned "unknown" handler for application/octet-stream. 

With the fix for [2] we're trying to get mimeinfo from extras by looking up vague APPLICATION_OCTET_STREAM and we set extensions for the mimeinfo to the exe, com, bin despite we're opening a pif file. The wrong extension is then used in
nsMIMEInfoUnix::GetHasDefaultHandler [4] (thanks for the hint paolo) and leads to returning valid mimeInfo when calling nsGNOMERegistry::GetFromExtension. I think we should keep the pif extension.

I'm not very fluent in the content-types. Is it possible to consider application/octet-stream as generic binary data and should we rely on extension only? Because when I apply the Windows only condition [5] also on Linux, I get the expected behaviour (only to save .pif file, as well as other application/octet-streams which extension is unknown for the system and on the other hand I'm able to open application/octet-streams with known extensions for the system).

[1] https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/uriloader/exthandler/nsExternalHelperAppService.cpp#2608
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1427700
[3] https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/uriloader/exthandler/nsExternalHelperAppService.cpp#2595
[4] https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp#24
[5] https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/uriloader/exthandler/nsExternalHelperAppService.cpp#2606

Comment 15

[:Paolo Amadini]

(In reply to Jan Horak [:jhorak] from comment #14)
> I'm not very fluent in the content-types. Is it possible to consider
> application/octet-stream as generic binary data and should we rely on
> extension only? Because when I apply the Windows only condition [5] also on
> Linux, I get the expected behaviour (only to save .pif file, as well as
> other application/octet-streams which extension is unknown for the system
> and on the other hand I'm able to open application/octet-streams with known
> extensions for the system).

Thanks for debugging this! Yes, I think that's totally how it should have worked all along. By looking at the source code, this is also what Chrome seems to do on all platforms. So, just removing the conditional compilation directive should work.

We can probably also remove the "application/x-msdownload" special case we have in two places, and treat this as an ordinary MIME type like Chrome, but it can be a separate bug.

Comment 16

[Jan Horak [:jhorak]]

Comment on attachment 8980297 [details]
Bug 1463809 - Don't lookup mimeinfo of application/octet-stream;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246462/diff/1-2/

Comment 17

[:Paolo Amadini]

Thank you, r+ from me on the changes and the new comment. Feel free to mark for landing after you've tested the result.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79f1e9fd7cb7
Don't lookup mimeinfo of application/octet-stream; r=Paolo

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/79f1e9fd7cb7