Closed Bug 1463809 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: File Handling, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 - unaffected
firefox63 + fixed

People

(Reporter: marco, Assigned: jhorak)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

Not sure if this is the right component.
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.
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?
Flags: needinfo?(mcastelluccio)
Assignee: nobody → 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
Flags: needinfo?(mcastelluccio)
Comment on attachment 8980297 [details]
Bug 1463809 - Don't lookup mimeinfo of application/octet-stream;

https://reviewboard.mozilla.org/r/246462/#review252886
Attachment #8980297 - Flags: review?(paolo.mozmail) → review+
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.
Flags: needinfo?(jhorak)
Priority: -- → P1
Status: NEW → ASSIGNED
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
Flags: needinfo?(jhorak) → needinfo?(paolo.mozmail)
Recent regression (from bug 1427700), tracking for 62. We could probably still take an uplift during beta.
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.
Flags: needinfo?(paolo.mozmail) → needinfo?(jhorak)
Depends on: 1472105
Filed bug 1472105 to back out bug 1427700.
(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...).
Tracking for 63, Jan did you have time to have a look at the Linux logic for this bug? Thanks
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
Flags: needinfo?(jhorak) → needinfo?(paolo.mozmail)
(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.
Flags: needinfo?(paolo.mozmail)
Thank you, r+ from me on the changes and the new comment. Feel free to mark for landing after you've tested the result.
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79f1e9fd7cb7
Don't lookup mimeinfo of application/octet-stream; r=Paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/79f1e9fd7cb7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.