This is basically bug 453455, but behind the pref, so I don't want us to mark that bug as fixed when in practice, users will still be seeing it until the improvements_to_download_panel pref change rides to release.

We should be able to change the code that does this at https://searchfox.org/mozilla-central/rev/eecd2dd4ba630bea4ce103623a4bfb398065b64b/uriloader/exthandler/nsExternalHelperAppService.cpp#1867-1877, if the pref is set. I think there's already an attempt there to do this, but I don't think it's 100% effective at the moment.

This is going to require some testing with different files (pdf, word doc, images), with/without the Content-Disposition: attachment header, and various configurations for those file types to either save to disk or open with an external helper app, or internally, and verifying that we honour those prefs irrespective of the header.

Added unit tests to verify that pdf, word doc, and image files, with/without the Content-Disposition: attachment header, and various configurations for those file types to either save to disk or open with an external helper app, or internally are honored irrespective of the header.

Still incomplete:

• verify opening with helper app or saving to disk based on user preferences and not a simulated click
• bug in cleanup when running multiple tests

Please take a look at my WIP patch. My tests are throwing errors on cleanup but run fine if I only do one instead of looping through all cases. I'm not sure how to fix that.

It would have been helpful to include the error messages that are confusing you; right now I don't know if I'm seeing the same thing.

The error message on my machine is:

 0:42.06 FAIL Cleanup function threw an exception - [Exception... "Unexpected error"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://testing-common/httpd.js :: stop :: line 631"  data: no]

The line number and filename point to this:

https://searchfox.org/mozilla-central/rev/2c4b830b924f42283632b70f39a60fd36433dd4d/netwerk/test/httpserver/httpd.js#631

So something is calling stop(), but the socket is already gone.

Then you look for what might be doing that, and having looked at your patch, I noticed this:

  return Downloads.createDownload({
-     source: httpUrl("interruptible.txt"),
+    source: httpUrl(file),

You're calling this helper that seems to have to do with the http server. So then I go check what that helper is: https://searchfox.org/mozilla-central/source/browser/components/downloads/test/browser/head.js#335

That's using a gHttpServer, and it turns out all your testcases call startServer - which overwrites that variable, and then adds a cleanup function. So then all the N cleanup functions for N tests call stop on the same server, and things break.

startServer isn't meant to be called more than once, because it's using that one global. Just once per test is sufficient. I'm not sure why you decided to call it in every testcase, or what test you copied that is using the server. I don't think you need that server for your testcase - I don't think you need your http server to be interruptible at all, and the mochitest framework already serves all your support-files on http://example.com/, .org, the https equivalent, and a pile of other hosts. Can you clarify why you think you need the interruptibility?

I would also have thought that you could start the download by clicking a link in content, and then waiting for the download object to be created, rather than creating download objects manually - the closer the test is to what happens to users, the better. Once you've got a download, you can check that we're set up to do the right thing with it and cancel it immediately. Does that sound like it'll work?

I've applied these changes. The only thing left is checking if the downloaded file is opened in Firefox, opened in an app, saved on disk, handled according to system defaults, or if the user was prompted to choose. Unfortunately, it doesn't appear that information is available from the JS environment.

(In reply to Bernard Igiri from comment #4)

I've applied these changes. The only thing left is checking if the downloaded file is opened in Firefox, opened in an app, saved on disk, handled according to system defaults, or if the user was prompted to choose. Unfortunately, it doesn't appear that information is available from the JS environment.

I think you can get this information off the mimeInfo object, which can be retrieved from the nsIMIMEService. In particular, you can call getFromTypeAndExtension. See https://searchfox.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js#148-153 for an example on this.

@Micah, (In reply to Micah [:mtigley] (she/her) from comment #5)

I'm using that to set the preferredAction so I was hoping to find an independent way to verify that it's doing what that setting is asking for.

    let mimeType = gHandlerService.getTypeFromExtension(fileExtension);
    let mimeSettings = gMIMEService.getFromTypeAndExtension(
      mimeType,
      fileExtension
    );
    mimeSettings.preferredAction = preferredAction;

You should be able to determine whether the download manager will open the file using the launchWhenSucceeded property on the download object, similar to this test: https://searchfox.org/mozilla-central/rev/d37daf2f82ed22b6a2a5cbbb975423825dfd69fa/browser/components/downloads/test/browser/browser_download_opens_on_click.js#74 .

You could also check that something calls launchDownload, like the code at https://searchfox.org/mozilla-central/rev/d37daf2f82ed22b6a2a5cbbb975423825dfd69fa/toolkit/components/downloads/test/unit/common_test_Download.js#2366-2376 and elsewhere in that test.

For internally handled files, you should be able to check that a tab opens with the right file, and for saved-to-disk files, I'd expect verifying that the file gets saved and that launchWhenSucceeded is false is sufficient (hard to otherwise verify that something doesn't happen, of course).

Apparently the Content-Disposition: attachment header is necessary. It turns out that my testing method was only triggering the JavaScript and the native code that actually implemented the preferredAction logic was not running. I have since changed my test modeling uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js to trigger the download by opening a new tab. Using this method I discovered that the header is necessary.

I also had trouble getting uriloader/exthandler/tests/mochitest/file_xml_attachment_binary_octet_stream.xml to download using the alwaysAsk preferred action. Although may be a fluke in my testing as alwaysAsk had to be run separately from the other test conditions to get it to work. I suspect that something isn't being cleaned up properly. Regardless, the tests in the patch all complete successfully.

Should a new bug be filed to address the header issue?

(In reply to Bernard Igiri from comment #8)

Apparently the Content-Disposition: attachment header is necessary. It turns out that my testing method was only triggering the JavaScript and the native code that actually implemented the preferredAction logic was not running. I have since changed my test modeling uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js to trigger the download by opening a new tab. Using this method I discovered that the header is necessary.

I don't really understand this comment - the header is necessary... for what? What happens without the header?

I also had trouble getting uriloader/exthandler/tests/mochitest/file_xml_attachment_binary_octet_stream.xml to download using the alwaysAsk preferred action. Although may be a fluke in my testing as alwaysAsk had to be run separately from the other test conditions to get it to work. I suspect that something isn't being cleaned up properly. Regardless, the tests in the patch all complete successfully.

Again, I feel like there are details missing here - what exactly were you trying to do, what did you expect, and what happened instead? You've not described the failure case (you're implying the file didn't download - what happened instead? The patch says there were "errors" - what kind of errors?)

Should a new bug be filed to address the header issue?

I don't know what "the header issue" is. The point of this bug is primarily to ensure that, if a user has configured a filetype to "always open with [name of external application]" (either the system default or a separately configured external app), that that configuration is honoured for that filetype, both when it is served with a Content-Disposition: attachment header, and when it isn't. It seemed to make sense to me in comment #0 to do the same testing for the other possible user configurations (always ask, save to disk, open internally). If the header is still influencing behaviour when the filetype is set to "open with [external app]", then the point of this bug is to address that. If you're running into issues with one of the other configurations (which one?) then that could probably be a separate bug.

(In reply to :Gijs (he/him) from comment #9)
File downloads have failed to complete under the following conditions:

• Attempting to download a png or webm file with Content-Disposition: inline
• Attempting to download a text file without the Content-Disposition header at all

The test in the patch for this ticket makes it easy to add those kinds of test cases and more. That is why I want to separate this ticket from the work to resolve the behavior when the Content-Disposition header is missing or set to a value other than attachment.

(In reply to Bernard Igiri from comment #10)

(In reply to :Gijs (he/him) from comment #9)
File downloads have failed to complete under the following conditions:

• Attempting to download a png or webm file with Content-Disposition: inline
• Attempting to download a text file without the Content-Disposition header at all

Can you explain what is meant by "failed to complete"? Does the download start and stay in the downloads panel permanently?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)

(In reply to Bernard Igiri from comment #10)

(In reply to :Gijs (he/him) from comment #9)
File downloads have failed to complete under the following conditions:

• Attempting to download a png or webm file with Content-Disposition: inline
• Attempting to download a text file without the Content-Disposition header at all

Can you explain what is meant by "failed to complete"? Does the download start and stay in the downloads panel permanently?

Yes.

(In reply to Bernard Igiri from comment #12)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)

(In reply to Bernard Igiri from comment #10)

(In reply to :Gijs (he/him) from comment #9)
File downloads have failed to complete under the following conditions:

• Attempting to download a png or webm file with Content-Disposition: inline
• Attempting to download a text file without the Content-Disposition header at all

Can you explain what is meant by "failed to complete"? Does the download start and stay in the downloads panel permanently?

Yes.

Can you describe more? Show a screenshot? Is there any message to the user? What is the state of the download object?

So it turns out that when I open a file that Firefox can display in a new tab with a Content-Disposition other than attached then Firefox will just open that file in the browser which is correct behavior. However as Gijs just pointed out to me, mime types that are not supported by Firefox should still be downloaded even without the Content-Disposition header. So I added test cases with mime types application/ms-word and application/x-7z-compressed without the Content-Disposition header and with Content-Disposition: inline respectively, and the tests all pass. I even used a node server to simulate a host serving these files with those headers, and those also downloaded correctly.

I believe that this sufficiently shows that the browser is handling these mime types correctly.

(In reply to Bernard Igiri from comment #14)

Great, thanks!

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit auto_nag documentation.

Attempting to use waitForNewTab with https://phabricator.services.mozilla.com/D127759 patch.

https://hg.mozilla.org/mozilla-central/rev/7c69c84030a5