Closed Bug 1633790 Opened 2 months ago Closed 2 months ago

Firefox opens infinite tabs when set as the OS default file handler for PDFs

Categories

(Firefox :: File Handling, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 78
Tracking Status
firefox78 --- verified

People

(Reporter: jaws, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

STR:
Set Firefox as the default application handler for PDF
Save a PDF to your desktop
Open the PDF

ER:
One tab is opened and pdf.js is used to display the tab

AR:
Firefox will create infinite tabs as each tab prompts a new tab to open

So this is hairier than I was hoping. Some notes about what's hairy:

  • we support pdfs loaded in <embed> and <object> tags. We used to support either pdfjs or plugins, now we support just pdfjs or nothing. If pdfjs doesn't render these, we show the object/embed fallback content.
  • we determine whether to use pdfjs based on the Firefox preferences (so if you change your prefs to use an external handler, we won't use pdfjs) - but if we don't use pdfjs, you get no download or other UI - you just get the fallback content specified in the <embed> or <object> tag (probably telling you to download adobe reader or whatnot).
  • the prefs are channeled through the cpmm's sharedData, which is updated to the child from the parent process. It tracks one bool which determines whether or not pdfjs is available, based on both the state of the global pdfjs.disabled pref which is also used by enterprise policy, and the state of the handler service.
  • the bool informs the stream converter factory code as to whether it makes the stream converter available
  • whether the stream converter is available determines how the object code reacts, see https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/dom/base/nsContentUtils.cpp#6398 and its callsites.
  • in the more "normal" iframe/toplevel document case, the documentchannel code at https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/netwerk/ipc/DocumentLoadListener.cpp#120 will determine whether we think we can convert the stream while still in the parent process

So my plan right now is:

  1. add js-accessible API support to check if the user has either selected us manually as the default helper app for a given mime info object, or has selected the OS default and we're that default.
  2. in the parent process, add code to the pdfjs implementation of getConvertedType to check with the handler service if the user wants to use pdfjs. If the user doesn't, throw an error, which will cause the documentchannel to not use the stream converter and thus pdfjs in the content process -- with one exception, namely if the APIs from (1) indicate that the alternative selection from the user still ends up pointing to Firefox to handle pdfs (as an external / the OS default). In that case, change the mimeinfo to default to pdfjs and store the updated version, then use pdfjs

And also (not dependent on 1/2):

  • change the pdfjs content process data to only indicate whether pdfjs.disabled is set, so it's still possible to completely turn off pdfjs with enterprise policy or about:config . This will make us use pdfjs for the embed tag variants even when the user has configured a different default. That's strictly an improvement on the status quo, and there's clear UI in pdfjs to download those pdfs or open them with other viewers, so this seems uncontroversial. The parent process code would still prevent pdfjs use (and ask / download / open in a helper instead) if a different viewer is configured as the default;
  • add the channel to the arguments of getConvertedType, and change pdfjs's parent process handling in that method to allow pdfjs use for file: channels loaded with system principal (ie user-opened pdfs that are already on disk), irrespective of user preferences for pdfjs use. That way, when users manually try to open pdfs on their computer with Firefox, we always open them internally rather than with another app, which can't be what users expect in that case.

The APIs from (1) could be used to decide for things other than PDFs to either show error messages or otherwise abort the infinite loops in bug 167320 / bug 215554.

Jared/Matt, does the above seem reasonable to you?

Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jaws)

Yeah that sounds great. Thanks for doing the deep digging here, I know it took a lot of work and I really appreciate it!

Flags: needinfo?(jaws)

Yeah that does indeed sound great! Thanks for working on this :)

Flags: needinfo?(matt.woodrow)

This is related but a bit of a driveby - if the user has selected the running
application as the handler for a file, we currently lie and show the OS
default instead. This is not helpful.

Depends on D73508

Prior to this patch, PDF.js tracks both its own 'disabled' pref (which is used
by enterprise policy) and whether it is the default handler per the handler
service - but it tracks both in one bool, which determines whether its
streamconverter registers.

Really, what we want is to never use PDF.js if it's preffed off.

However, if there is some other default, it should be acceptable to use PDF.js
in some circumstances, like for <embed> or <object>s where otherwise we
would show no content at all.

Even for toplevel PDFs, if the user has configured Firefox to open PDFs in
an external helper app which is Firefox (which is currently an easy mistake
to make in the unknownContentType dialog), or has it set to the OS default,
but has changed their OS default to Firefox, we really still want to open
those PDFs with PDF.js.

This patch fixes all of this by splitting out the pref tracking from the
handler state tracking. Only the pref will completely disable PDF.js.

Then, in the streamconverter code, we check whether PDF.js should be used for
PDFs, and if there's a misconfiguration that we can correct. This code is
invoked from the parent process when we load PDFs in frames or toplevel
documents, and will prevent us from invoking PDF.js in the child if the user
would prefer that not to happen.

As a driveby, this cleans up how we track the pref inside PDF.js, and how we
get notified of changes to the handler - we were missing changes made in the
unknown content type dialog, so it seemed worth making it generic.

Depends on D73509

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f2ff79643918
remove unused method arguments in the external helper app service, r=jaws
https://hg.mozilla.org/integration/autoland/rev/418a69055fc5
add an API on nsIMIMEInfo that exposes whether we're the OS default handler, r=jaws
https://hg.mozilla.org/integration/autoland/rev/979a77b1cbf3
allow PDF.js use when we've misled the user into misconfiguring PDF handlers, r=jaws,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/ae1a72a9dce6
pass channels to stream conversion getContentType and always allow PDF.js use for user-opened local PDF files, r=mattwoodrow,jaws
Blocks: 1631105
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1ae16df985dc
Port bug 1633790 - Add aChannel argument to nsStreamConverter.getConvertedType. rs=bustage-fix
Blocks: 1555637

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

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.