1751061 - PDFjs download button has weird edge case where we still show 'what do you want to do with this file' dialog that is tested in browser_download_open_with_internal_handler.js

Status: RESOLVED DUPLICATE of bug 1757771

(Firefox :: Downloads Panel, defect, P3)

Description

[:Gijs (he/him)]

browser_download_open_with_internal_handler.js, among many many other things (maybe we should split that test...) opens a dummy PDF file from http, with PDF set to "always ask". Then it picks "open with Firefox". So the test downloads to the download folder (a subfolder of tmp, in the test) and displays it.

Then (after another bit of testing with a separate http load that's not related to this bug) we click the download button inside PDF.js. See this link for code at time of writing, though I'm tweaking it in bug 1750042.

Now, after bug 1740135 I'd have expected that to open a "save as" dialog. But it doesn't, it opens another "what do you want to do with this file" dialog.

The reason seems to be that somehow we throw an exception of sorts (my JS debugger on nightly trunk appears semi-broken so I'm not sure what exactly) in this code and then end up hitting the downloadUrl catch call. That ends us up with this code:

  downloadUrl(url, filename) {
    FirefoxCom.request("download", {
      originalUrl: url,
      filename
    });
  }

This ends up talking to the stream converter code which has this check:

    // If the download was triggered from the ctrl/cmd+s or "Save Page As"
    // or the download button, launch the "Save As" dialog.
    const saveOnDownload = getBoolPref(
      "browser.download.improvements_to_download_panel",
      false
    );

    if (
      data.sourceEventType == "save" ||
      (saveOnDownload && data.sourceEventType == "download")
    ) {
      let actor = getActor(this.domWindow);
      actor.sendAsyncMessage("PDFJS:Parent:saveURL", {
        blobUrl,
        filename,
      });
      return;
    }

in other words, for saved PDFs (e.g. with form fields filled in) or downloaded PDFs if the download improvements pref is on, show a "save file" dialog. Otherwise, we fall through to the code below it, which shows the "what do you want to do with this file" dialog (unknownContentType.xhtml).

Now... the callsite above doesn't pass a sourceEventType at all. So neither of these conditions pass, and we end up falling through to the code below it anyway.

I think this isn't what we want? But I haven't thought about it too much. How user-relevant this is probably depends on what the condition is under which the viewer.js code throws an exception and falls back to downloadUrl - a quick test with a PDF I happened to have in my download folder locally doesn't show this behaviour, it shows the "save file as.." dialog as expected. If I had to guess, there's something wrong with the dummy PDF with which we're testing and it throws off the code here.

Sam, thoughts?

Comment 1

[Sam Foster [:sfoster] (he/him)]

Now... the callsite above doesn't pass a sourceEventType at all. So neither of these conditions pass, and we end up falling through to the code below it anyway.

I think this isn't what we want? But I haven't thought about it too much. How user-relevant this is probably depends on what the condition is under which the viewer.js code throws an exception and falls back to downloadUrl - a quick test with a PDF I happened to have in my download folder locally doesn't show this behaviour, it shows the "save file as.." dialog as expected. If I had to guess, there's something wrong with the dummy PDF with which we're testing and it throws off the code here.

The short answer is I think we want "save" to be the fallback behavior here, so maybe that code should check:

if (
      (!data.sourceEvent || data.sourceEventType == "save") ||
      (saveOnDownload && data.sourceEventType == "download")
    ) {
...
}

It looks like the PDF files served up for that test are empty? E.g. file_pdf_application_pdf.pdf which unless I'm misunderstanding how this all works will indeed produce an exception. Elsewhere, we use a minimal-valid PDF file for testing.
Regardless - or in fact especially in this case - we should be saving rather than attempting to pass exception-causing, potentially-bad files over to an external application. So perhaps when we refactor this test we can add explicit coverage for this failure case too.

Comment 2

[Neil Deakin]

I looked at this and it is the same issue as 1757771.