Firefox opens infinite tabs when set as the OS default file handler for PDFs
Categories
(Firefox :: File Handling, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox78 | --- | verified |
People
(Reporter: jaws, Assigned: Gijs)
References
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
Assignee | ||
Comment 3•5 years ago
|
||
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
'ssharedData
, 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 globalpdfjs.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:
- 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.
- 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?
Reporter | ||
Comment 4•5 years ago
|
||
Yeah that sounds great. Thanks for doing the deep digging here, I know it took a lot of work and I really appreciate it!
Comment 5•5 years ago
|
||
Yeah that does indeed sound great! Thanks for working on this :)
Assignee | ||
Comment 6•5 years ago
|
||
Assignee | ||
Comment 7•5 years ago
|
||
Depends on D73507
Assignee | ||
Comment 8•5 years ago
|
||
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
Assignee | ||
Comment 9•5 years ago
|
||
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
Assignee | ||
Comment 10•5 years ago
|
||
Depends on D73510
Assignee | ||
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
Comment 13•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2ff79643918
https://hg.mozilla.org/mozilla-central/rev/418a69055fc5
https://hg.mozilla.org/mozilla-central/rev/979a77b1cbf3
https://hg.mozilla.org/mozilla-central/rev/ae1a72a9dce6
Comment 14•5 years ago
|
||
Comment 15•4 years ago
|
||
Verified fixed with Fx 78.0a1 (2020-05-24) across platforms - Windows 10 64bit, macOS 10.15 and Ubuntu 18.04.
Description
•