1815739 - Support using Firefox as default PDF reader on Android

Status: RESOLVED FIXED

(Firefox for Android :: General, enhancement, P1)

Description

[Marco Castelluccio [:marco]]

Once we have enabled the reader by default on Android and if we are satisfied with the performance, we should evaluate setting ourselves as default reader on Android.

Comment 1

[Marco Castelluccio [:marco]]

The first step should be to at least allow users to do it manually.

Comment 2

[Calixte Denizet (:calixte)]

I tested locally with a modified:
https://github.com/mozilla-mobile/firefox-android/blob/main/fenix/app/src/main/AndroidManifest.xml
to handle pdfs.

Fenix is correctly proposed to open a pdf but then urls like content://media/external_primary/downloads... or content://com.android.providers.downloads.documents/document/msf%3A1000000018 are passed to Fenix which doesn't seem to be able to read them.
So I suppose that a first good step would be to make Fenix supports such a scheme.

Comment 3

[Calixte Denizet (:calixte)]

:csadilek, would it be possible to only allow content://... for pdf files ?

Comment 4

[Christian Sadilek [:csadilek]]

Unfortunately, I don't think so. We need to address bug 1684947 first. Otherwise, we'd open ourselves up for data leaks, see also https://bugzilla.mozilla.org/show_bug.cgi?id=1684761#c13 for more details e.g., a file with a pdf extension could still link to an internal file.

Currently content, resources, and file are blocked for directs loads in Fenix.

Comment 5

[Marco Castelluccio [:marco]]

Do we have internal files that are PDFs?

Comment 6

[Christian Sadilek [:csadilek]]

Hi :marco,

My concern is more: How would we make sure it is really a PDF we're loading and bypassing the rule for? And it's not e.g., a symbolic link to an internal file the user was tricked into creating via a different application?

I think we cannot simply make the decision based on the file extension. So this would need more investigation, ideally addressing bug 1684947 first.

Comment 7

[Calixte Denizet (:calixte)]

I'm not an Android stuff expert, but as far as I understand, nowadays shared uris should use a content scheme rather than a file one.
So if the ContentProvider is trusted, I suppose we can consider those uris as safe.
I think we shouldn't handle file uris.
Considering https://developer.android.com/topic/security/risks/content-resolver, the minimal mitigation we could do is to have a whitelist with the authorities we want to allow to provide some data to Fenix (for example com.android.providers.downloads for the app managing the downloads).
Anyway, I've a patch for handling uris with a content scheme, I'll submit it and we could then iterate to see how make it safe enough.

Comment 8

[Calixte Denizet (:calixte)]

Attachment: Bug 1815739 - Add a protocol handler for content uris in GV r=#geckoview-reviewers

On Android, contents are shared in using uris with a content scheme, hence this patch
adds a new protocol handler for 'content:\', a new type of input stream in order to read
them and a new type of channel.

Comment 9

[zesanup]

whitelist with the authorities we want to allow

If file manager apps are to be included, maybe the whitelist should include com.android.documentsui?

Comment 10

[zesanup]

Can the PDF viewer be completely separated from the main app? As in, opening a PDF link in Fenix would start a separate 'PDF Viewer' activity instead of simply opening a new tab, and you could switch between the two via the Recent Apps switcher.

What I'm envisioning is that opening a local PDF in Fenix would open the PDF Viewer activity only, without starting the main browser. I've seen this behaviour in the Google Drive app's PDF viewer, where any PDF would open near instantly in a separate activity (com.google.android.apps.viewer.PdfViewerActivity), while the main Drive app itself (com.google.android.apps.docs) was quite slow to load.

Instead of waiting for the whole browser (and extensions) to fire up, just launching pdf.js as a separate activity would be faster, and eat much less RAM, too.

Comment 11

[zesanup]

Bug 1813251 is a duplicate of this bug.

Comment 13

[Marco Castelluccio [:marco]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Users will be able to use Firefox as default PDF reader.
[Affects Firefox for Android]: Yes, exclusively Firefox for Android.
[Suggested wording]: Firefox can now be used as default PDF reader.
[Links (documentation, blog post, etc)]: N/A

Comment 14

[Sylvestre Ledru [:Sylvestre]]

maybe "Firefox can now be set as a primary PDF viewer." for the release notes?

Comment 15

[David A. Madore]

(In reply to Marco Castelluccio [:marco] from comment #13)

Release Note Request (optional, but appreciated)
[Why is this notable]: Users will be able to use Firefox as default PDF reader.
[Affects Firefox for Android]: Yes, exclusively Firefox for Android.
[Suggested wording]: Firefox can now be used as default PDF reader.
[Links (documentation, blog post, etc)]: N/A

I'm confused about this comment. Has this bug actually been fixed (I've seen no meaningful activity over the last few months)? Or should I hold my joy because you're just preparing what should be written in the release notes when (and if… 😢) this bug is eventually fixed?

Comment 16

[Marco Castelluccio [:marco]]

We are preparing for when the bug is eventually fixed, but we are planning to fix this very soon (you can see activity in the Phabricator revision that is attached to this bug).

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22970015ff0c
Add a protocol handler for content uris in GV r=geckoview-reviewers,owlish,tthibaud,necko-reviewers,kershaw

Comment 18

[Serban Stanca [:SerbanS]]

Backed out for causing saveAContentPdfDocument related junit failures.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.PdfCreationTest#saveAContentPdfDocument | org.mozilla.geckoview.test.rule.GeckoSessionTestRule$ChildCrashedException: Child process crashed

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61e622def2f6
Add a protocol handler for content uris in GV r=geckoview-reviewers,owlish,tthibaud,necko-reviewers,kershaw

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/61e622def2f6

Comment 21

[Ryan VanderMeulen (PTO, back 6-April)]

Added to the Fx121 relnotes.

Comment 22

[Ryan VanderMeulen (PTO, back 6-April)]

Looks like bug 1864956 is going to be shipping in 122 instead.