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

Steps to reproduce:

1. Open https://www.erpsever.bg/bg/document/download/144 and select "Open with Nightly"
2. Click on the Link in step 1 (on page 2) that reads "371 Искане за проучване условията за присъединяване на обект на клиент (ФЛ)".

Actual results:

Nightly doesn't show the option to "Open with Nightly", as it did when opening the PDF from https://www.erpsever.bg/bg/document/download/144

Expected results:

Nightly should've shown the option "Open with Nightly".

I will set a component to have a starting point of this. If this is not the right component please feel free to route this ticket to the corresponding team, thanks!

(In reply to kernp25 from comment #2)

Maybe because of https://searchfox.org/mozilla-central/rev/a5d9abfda1e26b1207db9549549ab0bdd73f735d/toolkit/mozapps/downloads/HelperAppDlg.jsm#1316-1318

Yes. This check is here to avoid showing the option for when people click the download / save icon inside pdf.js , as that would obviously be wrong/confusing.

We have access to the URL of the item for which we're showing the prompt in the helperappdlg.jsm code (see https://searchfox.org/mozilla-central/rev/a5d9abfda1e26b1207db9549549ab0bdd73f735d/toolkit/mozapps/downloads/HelperAppDlg.jsm#505-509 ). :bdahl, if we're looking for a more reliable way to do that check, will the "save" or "download" item from PDF.js always load a blob: URI (or maybe the URI of the pdf as it was loaded - so matching the document URI of the browsing context) ? Perhaps we could use that?

Let's mark this as a good first bug.

To fix this, the check at https://searchfox.org/mozilla-central/rev/a5d9abfda1e26b1207db9549549ab0bdd73f735d/toolkit/mozapps/downloads/HelperAppDlg.jsm#1316-1318 should be replaced with a check that this.mLauncher.source is either a blob URL or corresponds to the URL of the browsing context. You can check the former using the url's scheme property, and the latter by using its equalsExceptRef method, comparing with the browsing context top embedder element's currentURI property. The latter will only be possible if the download has an associated browsingContext.

Then there will need to be an addition to the test. There's an existing test at https://searchfox.org/mozilla-central/rev/50215d649d4854812837f1343e8f47bd998dacb5/uriloader/exthandler/tests/mochitest/browser_download_open_with_internal_handler.js#74-80 and we should add some code to that task to load a different PDF served with content-disposition: attachment into the same browsing context, and verify that the option to use pdf.js (ie "Open with Firefox/Nightly") does appear in that case.

Assignee

Comment 6

Hi, I'd like to work on this bug. I'll have some time later this week to work on it.

(In reply to Paul Dill from comment #6)

Hi, I'd like to work on this bug. I'll have some time later this week to work on it.

That would be great! Let me know if there's anything you need help with.

Assignee

Comment 8

Ok, I've been looking through this and I have a couple of questions. First, do I access the url's scheme property using the following:
!browsingContext?.currentWindowGlobal?.documentPrincipal?.URI?.schemeIs( "resource" )
If so, I'm thinking that it will end up looking something like this:

    if (primaryExtension == "pdf") {
      return (
       (url === ( !browsingContext?.currentWindowGlobal?.documentPrincipal?.URI?.schemeIs(
          "resource"
        ) || equalsExceptRef(browsingContext.currentURI) ) ) &&
        !Services.prefs.getBoolPref("pdfjs.disabled", true) &&
        Services.prefs.getBoolPref(
          "browser.helperApps.showOpenOptionForPdfJS",
          false
        )
      );
    }

Does it seem like I'm on the right path here, or am I far from it? Any feedback is much appreciated!

Assignee

Comment 9

(In reply to Paul Dill from comment #8)

Ok, I've been looking through this and I have a question. Do I access the url's scheme property using the following:
!browsingContext?.currentWindowGlobal?.documentPrincipal?.URI?.schemeIs( "resource" )
If so, I'm thinking that it will end up looking something like this:

    if (primaryExtension == "pdf") {
      return (
       (url === ( !browsingContext?.currentWindowGlobal?.documentPrincipal?.URI?.schemeIs(
          "resource"
        ) || equalsExceptRef(browsingContext.currentURI) ) ) &&
        !Services.prefs.getBoolPref("pdfjs.disabled", true) &&
        Services.prefs.getBoolPref(
          "browser.helperApps.showOpenOptionForPdfJS",
          false
        )
      );
    }

Does it seem like I'm on the right path here, or am I far from it? Any feedback is much appreciated!

Hey Paul, thanks for looking at this! Confusingly, there are a few different URIs in play here:

1. the URI for which we're showing the dialog (ie the thing being "downloaded") -- this is the new PDF, and is this.mLauncher.source
2. the document URI of the browsing context (think "tab") into which we would have loaded it, if it had been a webpage instead of a PDF or something else for which we offer download as an option -- this is browsingContext.currentWindowGlobal.documentURI if the current page is a PDF loaded in the PDF preview
3. the principal ("security privileges", also represented as a wrapper around a URI) of that document - in the PDF case, because we load the PDF in pdf.js, this is the URI of PDF.js, which means that browsingContext.currentWindowGlobal.documentPrincipal.URI is going to point to pdf.js .

Right now, we're checking (3). Your example in comment #8/9 appears to still be attempting to do that.

Instead, we should be checking (1) and (2). We should check if those are the same, or if (1) is a blob URI.

All 3 of those JS expressions evaluate to a URI object, and you can get the scheme/protocol of the URI using the scheme property (or compare it with the schemeIs method), the full URI as a string with the spec property, and you can compare with another URI object with the equalsExceptRef method, which takes another URI object as argument and returns a boolean to indicate whether the two URIs are the same.

Does that help?

(Apologies for the slow response; I'm on some time off at the moment and in the middle of moving house, I'll be back full-time on Tuesday next week.)

Assignee

Comment 11

Ok, I'm starting to understand this a little better. So, I want to check if
this.mLauncher.source === (browsingContext.currentWindowGlobal.documentURI.schemeIs("blob") || browsingContext.currentWindowGlobal.documentURI.equalsExceptRef(currentURI) ?

Does that seem right?

(I hope your move went well! I've also been busy with family in town hence my massive delays.)

(In reply to Paul Dill from comment #11)

Ok, I'm starting to understand this a little better. So, I want to check if
this.mLauncher.source === (browsingContext.currentWindowGlobal.documentURI.schemeIs("blob") || browsingContext.currentWindowGlobal.documentURI.equalsExceptRef(currentURI) ?

Does that seem right?

Not quite, I think you want this.mLauncher.source.schemeIs("blob") || this.mLauncher.source.equalsExceptRef(browsingContext.currentWindowGlobal.documentURI)

Does that make sense?

(I hope your move went well! I've also been busy with family in town hence my massive delays.)

No rush, thank you for working on this!

Oh ok! So it will be like this:

    if (primaryExtension == "pdf") {
      return (
        this.mLauncher.source === (this.mLauncher.source.schemeIs("blob") || 
        this.mLauncher.source.equalsExceptRef(browsingContext.currentWindowGlobal.documentURI)) ?
         &&
        !Services.prefs.getBoolPref("pdfjs.disabled", true) &&
        Services.prefs.getBoolPref(
          "browser.helperApps.showOpenOptionForPdfJS",
          false
        )
      );
    }

As far as the testing goes, I'll confess, I have no experience with it, and I'm unsure quite where to start. I notice that there's a couple of file paths defined at the beginning of the section for (let file of [ "file_pdf_application_pdf.pdf", "file_pdf_binary_octet_stream.pdf", ])
Do I need to add anything there, or am I just going to add the the variables that follow? Create something like:
let differentPDF = await extraTab.anotherPDFPathHere
Shot in the dark, i know, but I wanted to have something to offer. Thank you for your guidance!

(In reply to Paul Dill from comment #13)

(Edited for formatting)

Oh ok! So it will be like this:

    if (primaryExtension == "pdf") {
      return (
        this.mLauncher.source === (this.mLauncher.source.schemeIs("blob") || 

Almost - schemeIs returns a boolean, so you don't need to compare it to anything anymore, just:

this.mLauncher.source.schemeIs("blob") ||

instead of that line would work.

As far as the testing goes, I'll confess, I have no experience with it, and I'm unsure quite where to start. I notice that there's a couple of file paths defined at the beginning of the section for (let file of [ "file_pdf_application_pdf.pdf", "file_pdf_binary_octet_stream.pdf", ])
Do I need to add anything there, or am I just going to add the the variables that follow? Create something like:
let differentPDF = await extraTab.anotherPDFPathHere
Shot in the dark, i know, but I wanted to have something to offer. Thank you for your guidance!

I don't think we need a new variable here, but I think when we are inside the loop, we should try loading the other of the two pdf files in the same context, and check that the "open with Firefox" option is available. If you look at this section:

https://searchfox.org/mozilla-central/rev/50215d649d4854812837f1343e8f47bd998dacb5/uriloader/exthandler/tests/mochitest/browser_download_open_with_internal_handler.js#143-165

it clicks the "download" button inside pdfjs and checks that the resulting dialog doesn't have the "open with Firefox" option (because we're downloading the PDF that we already opened in Firefox, so showing that option would make no sense).

We can add a bit of code before this section that does something similar - we can copy let subdialogPromise = BrowserTestUtils.domWindowOpenedAndLoaded();, and instead load the other PDF by using BrowserTestUtils.loadURI(newTab.linkedBrowser, url-to-other-pdf-goes-here), then await the subdialogPromise to get the dialog in the same way that the existing section does. Then we can verify that the "open in Firefox" option is present (the opposite of the check on line 162-165), and then cancel the dialog. Then we leave the existing section in place, and it'll go ahead with the download and check that opening the download works as it does now.

Hopefully that makes sense?

Assignee

Comment 15

Got it, I removed the redundant this.mLauncher.source === check.

I took a shot at creating the test. I did what you suggested, and pulled the pieces of code that seemed relevant from the surrounding tests. How does this look?
let subdialogPromise = BrowserTestUtils.domWindowOpenedAndLoaded(); BrowserTestUtils.loadURI(newTab.linkedBrowser, TEST_PATH); await subdialogPromise; let subDoc = subDialogWindow.document; // Prevent racing with initialization of the dialog and make sure that // the final state of the dialog has the correct visibility of the internal-handler option. await waitForAcceptButtonToGetEnabled(subDoc); let subInternalHandlerRadio = subDoc.querySelector("#handleInternally"); ok( subInternalHandlerRadio.exists, "This option should be shown when the dialog is opened internally" ); // Remove the first file (can't do this sooner or the second load fails): if (extraTab?.target.exists) { try { info("removing " + extraTab.target.path); await IOUtils.remove(extraTab.target.path); } catch (ex) { /* ignore */ } }

Comment hidden (obsolete)

Assignee

Comment 17

/Man facepalming emoji here/

    let subdialogPromise = BrowserTestUtils.domWindowOpenedAndLoaded();
    BrowserTestUtils.loadURI(newTab.linkedBrowser, TEST_PATH);
    await subdialogPromise;
    let subDoc = subDialogWindow.document;
    // Prevent racing with initialization of the dialog and make sure that
    // the final state of the dialog has the correct visibility of the internal-handler option.
    await waitForAcceptButtonToGetEnabled(subDoc);
    let subInternalHandlerRadio = subDoc.querySelector("#handleInternally");
    ok(
      subInternalHandlerRadio.exists,
      "This option should be shown when the dialog is opened internally"
    );
    // Remove the first file (can't do this sooner or the second load fails):
    if (extraTab?.target.exists) {
      try {
        info("removing " + extraTab.target.path);
        await IOUtils.remove(extraTab.target.path);
      } catch (ex) {
        /* ignore */
      }
    }

Please excuse my markdown errors.

(In reply to Paul Dill from comment #17)

    let subdialogPromise = BrowserTestUtils.domWindowOpenedAndLoaded();
    BrowserTestUtils.loadURI(newTab.linkedBrowser, TEST_PATH);

I think you'll want to add the filename of either of the PDFs to TEST_PATH - otherwise it'll just be a folder. The one we've currently loaded will be available as file (see the start of the loop) - we'll want the other one.

    await subdialogPromise;
    let subDoc = subDialogWindow.document;
    // Prevent racing with initialization of the dialog and make sure that
    // the final state of the dialog has the correct visibility of the internal-handler option.
    await waitForAcceptButtonToGetEnabled(subDoc);
    let subInternalHandlerRadio = subDoc.querySelector("#handleInternally");
    ok(
      subInternalHandlerRadio.exists,
      "This option should be shown when the dialog is opened internally"
    );

This should check the item isn't hidden, ie !subInternalHandlerRadio.hidden.

    // Remove the first file (can't do this sooner or the second load fails):
    if (extraTab?.target.exists) {
      try {
        info("removing " + extraTab.target.path);
        await IOUtils.remove(extraTab.target.path);
      } catch (ex) {
        /* ignore */
      }
    }

Hm, not entirely sure what you're trying to do here - it looks like a cool mash-up between removing a file and removing a tab. At this point though, I don't think we need to do either - you can use subDoc.querySelector("#unknownContentType").cancelDialog(); to cancel the dialog, and that should be sufficient.

Keep in mind that because there's some duplication in variable names and you can only declare let variables once, you'll need to remove let in some of the subsequent code in the same block.

If you commit a revision in hg/git, you can upload it here with moz-phab and we can continue the conversation in phabricator? That makes it a bit easier to talk about patches. :-)

Assignee

Comment 19

Sound good! I've gone through the setup process for moz-phab, but I'm having an issue submitting the commit. Is it just moz-phab submit <fileName> ?

(In reply to Paul Dill from comment #19)

Sound good! I've gone through the setup process for moz-phab, but I'm having an issue submitting the commit. Is it just moz-phab submit <fileName> ?

If you've already committed with hg commit (or git commit if you're using git), and you've included the bug number at the start of the commit message, then you can submit with just moz-phab submit without further arguments. You can also look at moz-phab help submit to see a list of options you could use if there are issues.

Assignee

Comment 21

I see. I hadn't committed using hg commit. I'm still getting an error message when I do moz-phab submit though. It's saying that I'm unable to submit the commit because user is disabled. I can't find much info online about this error message.

dill5446@awesom-o:~/firefox-source$ moz-phab submit
Submitting 1 commit for review:
(New) 557946:5ead9a2cc303 Bug 1672384 - Links to PDFs from other PDFs don't show the option 'Open with Firefox' r?Paul Dill
Unable to submit commits:

557946:5ead9a2cc303 Bug 1672384 - Links to PDFs from other PDFs don't show the option 'Open with Firefox' r?Paul Dill
- User paul is disabled

(In reply to Paul Dill from comment #21)

I see. I hadn't committed using hg commit. I'm still getting an error message when I do moz-phab submit though. It's saying that I'm unable to submit the commit because user is disabled. I can't find much info online about this error message.

dill5446@awesom-o:~/firefox-source$ moz-phab submit
Submitting 1 commit for review:
(New) 557946:5ead9a2cc303 Bug 1672384 - Links to PDFs from other PDFs don't show the option 'Open with Firefox' r?Paul Dill
Unable to submit commits:

557946:5ead9a2cc303 Bug 1672384 - Links to PDFs from other PDFs don't show the option 'Open with Firefox' r?Paul Dill
- User paul is disabled

Cool, you're almost there - the annotation after the r? is meant to point to the person who you want to review the patch (me, in this case). So if you use hg amend -e or hg commit --amend and change the commit message to end with r?gijs then it should work.

(Right now, because phab usernames have no spaces, it's looking for the phabricator user "paul", and apparently their account is disabled? I dunno.)

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

This issue is reproducible in Release v85.0, and the fix is verified in Beta v86.0b3 and Nightly v87.0a1 from 2021-01-29. Verified on Windows 10 and Mac OS 11.1.