User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

1. In about:preferences#general -> Applications, make sure the Action for PDFs is "Preview in Firefox
2. click the Download button on e.g. https://github.com/trailofbits/publications/blob/master/reviews/hegic-summary.pdf

Actual results:

The download is immediately started, without a confirmation popup. Given that this is unexpected, the file downloaded animation is easy to miss.

Expected results:

Either:

• the pdf should have been previewed with pdf.js
  Or:
• the download confirmation popup should have been displayed

Note: I've found a similar Bug, but I belive this to be a different issue:

• Bug 1231127 - "Open with" option missing for files sent as Content-Type: application/octet-stream

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

How is your downloads option set in about:preferences, is it set to Ask Everytime or Download to a common folder?

I can reproduce what you report if I set the option to always download to the same folder, and that's the expected behavior.

Not downloading the file (and previewing it) in this case sounds wrong, because the website i explicitely saying "download" and setting headers to make the browser manage it as a download.

yes, download location is set to ~/Downloads, but that selects where to download to, not that I want to download every time. I expect either the pdf to still be displayed, or at least the "open with/save" dialog to pop up, like it does with other file types. see for example here: trying to download this tar.gz file (which is served with the same headers as the pdf) does the expected thing (that is, what is specified in prefs->applications (e.g. "ask every time")): https://github.com/fork-graveyard/test-binaries/blob/master/archive.tar.gz, but compare this to https://github.com/fork-graveyard/test-binaries/blob/master/doc.pdf, which does not do what i asked it in prefs->applications, but silently downloads it.

even with the setting you have described set, other file types aren't silently downloaded--just pdfs.

Thank you for the counter example, that will be useful to debug this. I noticed Random Bytes example works like the tar.gz example, so apparently something special happens for pdf

Setting PDF to Always Ask opens the dialog, so this is related to the Preview in Firefox setting.

Another observation: with the same settings, PDFs served with a Content-Disposition header are given the "What should Firefox do with this file?" dialog. I guess that that's acceptable behaviour, even though I personally would still rather see the "Preview in Firefox" preference respected (it still doesn't do what I asked it to, but at least it's not silent). example file

relevant headers as reported by curl:
< Content-Disposition: attachment; filename="gastlizenz_amateur.pdf"
< Content-Type: application/pdf;charset=UTF-8

The priority flag is not set for this bug.
:Gijs, could you have a look please?

For more information, please visit auto_nag documentation.

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is -- (non,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

I just spent some quality time with a debugger, and this is a fun one.

We hit the external helper app service to try to handle the channel with a helper app. In ::CreateListener, we go ask the external helper app service how to deal with this mimetype+extension combination.

We go ask the OS for info on the mimetype at https://searchfox.org/mozilla-central/rev/0688ffdef223dac527c2fcdb25560118c4e4df51/uriloader/exthandler/nsExternalHelperAppService.cpp#2455 .

On Windows, when we get a bogus mimetype (application/octet-stream) from github, and no content-disposition header (so we don't force asking the user what to do, fixing which would be bug 453455) we realize the mimetype is bogus, and use the file extension (.pdf) instead. This is probably a good thing. We do a lookup based on the pdf extension, which gets us a mimeinfo object.

Then we proceed to add info to this object based on the info stored in the handler service (ie your user preferences). There's nothing there for the mimetype, so we fall back to checking with the extension, and so we find out that the default is "preview with Firefox" ("open with Firefox or "open with Nightly" on 77 and later, after some changes to clarify language in the dropdown, in bug 1581383).

The problem is.. the external helper app code isn't what deals with this. For previewing the pdf, we should have hit the streamconverter for pdfjs earlier in this process. But we still honour the fact that the mime info object says "don't ask" (after all, the user preference isn't set up to "always ask"), and because neither use of the OS default helper or another external helper app is specified, we download the file. Without asking. Oops.

A conservative change we can make immediately (and probably should, irrespective of what we do for the next para) is to make the external helper app code check whether we're set to handle internally, in which case, we should force alwaysAsk to true, because we've already failed to handle things internally -- things are only gonna go downhill from there if we stick our fingers in our ears... Then we'll ask what to do, instead. Asking will be less terrible after bug 773942 lands, as there'll be an option to open in pdfjs (which we'll select by default).

A less conservative change would be to allow the stream converter (ie pdfjs) to convert from application/octet-stream if the channel is for a .pdf file (and if there's no content-disposition: attachment header, viz. bug 453455 as mentioned earlier). This feels a bit counterintuitive here because the github button is labeled, uh, "Download" - but tbf, they haven't specified the content-disposition header to enforce that.

Matt, and Jared, thoughts on whether we should pursue the less conservative change here?

Thanks for the deep dive and write-up Gijs. Are we able to pop-up the unknown content type dialog if PDF.js fails to parse the stream? It would be great if we could, though this shouldn't be so necessary since there are inevitably corrupt pdf files with a correct mime/type that we fail to parse today and we don't need to expand this to make that situation better (if needed).

I'm definitely okay with the conservative change, and I'm just curious about the repercussions of the less-conservative change with corrupt files. Otherwise I think the UX makes sense with both changes.

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

Are we able to pop-up the unknown content type dialog if PDF.js fails to parse the stream?

I'm not sure - hopefully Matt would know?

(In reply to :Gijs (he/him) from comment #11)

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

Are we able to pop-up the unknown content type dialog if PDF.js fails to parse the stream?

I'm not sure - hopefully Matt would know?

I don't think we can easily unfortunately.

We make the decision about whether to pass the stream onto the external helper app in the parent process, before passing it to the content process. PDF.js runs in the content process, so we wouldn't know about the failure until we're in the middle of sending everything to the content process.

(In reply to :Gijs (he/him) from comment #9)

A less conservative change would be to allow the stream converter (ie pdfjs) to convert from application/octet-stream if the channel is for a .pdf file (and if there's no content-disposition: attachment header, viz. bug 453455 as mentioned earlier). This feels a bit counterintuitive here because the github button is labeled, uh, "Download" - but tbf, they haven't specified the content-disposition header to enforce that.

Matt, and Jared, thoughts on whether we should pursue the less conservative change here?

I think we should give this a go. We could put the new behaviour behind a pref to give us the option of keeping it on Nightly for a while if needed.

The content-disposition header is there for sites to less us know what to do, we can't expect to know that they labelled their button Download and what that means.

Depends on D75581

Backed out for mochitest failure on test_allowContentRetargeting.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/7a039ce4992268e293abc61a441a4aa06aaeda3c
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302585860&repo=autoland&lineNumber=110381

https://hg.mozilla.org/mozilla-central/rev/13d87924353a
https://hg.mozilla.org/mozilla-central/rev/ba9302c4f9b4

Verified fixed with Fx 78.0a1 (2020-05-21) across platforms - Windows 10 64bit, macOS 10.15 and Ubuntu 18.04.