1406903 - cannot view files using content:// scheme

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[richardp345]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170926190823

Steps to reproduce:

Open an .html or .txt file with app EDS Lite 2.x


Actual results:

Filefox not listed as an available app



Expected results:

Firefox should be listed

Issue is that Firefox doesn't have the 'content' scheme as an intent-filter in its manifest.

From the Firefox manifest:

                <data android:scheme="file"/>
                <data android:scheme="http"/>
                <data android:scheme="https"/>

From another browser's manifest:

                <data android:scheme="http"/>
                <data android:scheme="https"/>
                <data android:scheme="about"/>
                <data android:scheme="content"/>
                <data android:scheme="javascript"/>

Comment 1

[Jan Henning [:JanH]]

Also affect Android's in-built file explorer that is available on Marshmallow and up. Sadly it's not as simple adding the missing intent filter, as Firefox then doesn't know how to handle the content:// "protocol".

Comment 2

[Jan Henning [:JanH]]

It is sort of possible to convert content:// URIs into file:// URIs to make use of our existing file handling support, but that is rather hacky and not guaranteed to work all the time and across all possible OS versions and content providers, so we should probably immediately forget about that idea again unless we have absolutely no choice.

So the proper way is to use https://developer.android.com/reference/android/content/ContentResolver.html#openInputStream(android.net.Uri), which means a little more work in subsequently getting this into Gecko, though.
In principle we could access the data and store it in a temporary file that could then be accessed via a normal file:// URI, but my preliminary understanding is that if we wanted to preserve the content:// URI on the outside (so history entries/the session store/bookmarks/etc. work correctly and don't refer directly to the temporary file), we'd still have to mess around with the protocol handling somewhat, so we probably might as well go for the full solution, that is implementing a "content" protocol handler that calls out into Java-land to run ContentResolver.openInputStream on the given URI and if successful, then somehow marshals that data back across the JNI border into Gecko to stuff it into an nsIInputStream for further handling by Necko and the rest of Gecko.

Comment 4

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #2)
> (so history entries/the session store/bookmarks/etc. work correctly
> and don't refer directly to the temporary file)

Although for that to work we probably have to call https://developer.android.com/reference/android/content/ContentResolver.html#takePersistableUriPermission(android.net.Uri,%20int) and that only works if the app that gave us the content:// URI made it persistable in the first place.

Comment 5

[Nevin Chen(Not active on Bugzilla)]

NI Joe and Wesly to help prioritize this

Comment 7

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #4)
> (In reply to Jan Henning [:JanH] from comment #2)
> > (so history entries/the session store/bookmarks/etc. work correctly
> > and don't refer directly to the temporary file)
> 
> Although for that to work we probably have to call
> https://developer.android.com/reference/android/content/ContentResolver.
> html#takePersistableUriPermission(android.net.Uri,%20int) and that only
> works if the app that gave us the content:// URI made it persistable in the
> first place.

Chrome's behaviour is clearly suboptimal - if I open a local file through the inbuilt file manager in Chrome and then kill and restart Chrome, I get an ERR_FILE_NOT_FOUND error when Chrome tries to restore that tab with the content URI.
Unfortunately on the other hand there are some signs that there might be an upper limit on the number of persistable URI permissions an app can take [1], which means that we'd have to attempt releasing unused permissions again. The problem is of course that tracking when a permissions becomes unused, as that URI can be referenced from a load of different places:
- the active URL of a tab
- somewhere in the back/forward history of a tab
- a recently closed tab
- a tab from the previous session
- a bookmark
- browsing history
- ???
and tracking all those places seems rather painful (effectively we'd have to implement garbage collection for persistable URI permissions).


Another problem I've just realised: Opening a local file via a content URI will break HTML files using relative paths (just try it in Chrome and see it yourself). In theory FLAG_GRANT_PREFIX_URI_PERMISSION [2] looks like it would cover that, but
a) the calling app that provides the content URI with its ContentProvider needs to correctly implement that in the first place
b) I'm not sure if you can actually grant an additional file permission for the necessary (although see c) for that) directories at the same time as passing the content URI to the actually intended target file
c) relative paths could go all over the place both down and *up* in the directory structure, so e.g. a file explorer app would have to grant permissions for potentially the whole storage card in case some relative path in that HTML file is referencing it

So from that point of view, sadly it seems like implementing something like [3] and only falling back to the theoretically proper way of using ContentResolver.openInputStream() if we cannot retrieve a working file URI would be the way to go :-(

[1] https://stackoverflow.com/questions/30802971/android-max-number-of-persistable-uri-permission-granted-to-an-app-is-limited-to
[2] https://developer.android.com/reference/android/content/Intent.html#FLAG_GRANT_PREFIX_URI_PERMISSION
[3] https://github.com/iPaulPro/aFileChooser/blob/master/aFileChooser/src/com/ipaulpro/afilechooser/utils/FileUtils.java, if necessary extending it for whatever content URI schemes other popular file managers might be using [4]
[4] Assuming/hoping/asking them nicely that their content URI scheme/Intent allows reverse engineering the file path in some way or other

Comment 8

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #2)
> Another problem I've just realised: Opening a local file via a content URI
> will break HTML files using relative paths (just try it in Chrome and see it
> yourself).

Out of curiosity I filed https://bugs.chromium.org/p/chromium/issues/detail?id=827801 for that. Let's see what - if anything - they'd do.

(In reply to Jan Henning [:JanH] from comment #7)
> So from that point of view, sadly it seems like implementing something like
> [3] and only falling back to the theoretically proper way of using
> ContentResolver.openInputStream() if we cannot retrieve a working file URI
> would be the way to go :-(

So if in the end we still have to preferentially attempt accessing the original file URI (where possible) to preserve handling of relative URIs, and attempting to persist content URIs has its own problems because not every app might allow that and there might be an upper limit on the number of URIs we can persist, we might as well give up and just

(In reply to Jan Henning [:JanH] from comment #2)
> access the data and store it in a temporary file that
> could then be accessed via a normal file:// URI

In that case we could implement this via the ContentDispatchChooser [1], which we already use to forward unknown protocols first to our Android front end and then to the OS. We could intercept content://-URIs, either retrieve the original file://-URI or create a temporary file ourselves and then return that fixed-up URI to Gecko.

[1] https://dxr.mozilla.org/mozilla-central/rev/0405f6006f3a3f653dd42d587c3eefe08cffa37d/mobile/android/components/ContentDispatchChooser.js

Comment 9

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 10

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #8)
> (In reply to Jan Henning [:JanH] from comment #2)
> > Another problem I've just realised: Opening a local file via a content URI
> > will break HTML files using relative paths (just try it in Chrome and see it
> > yourself).
> 
> Out of curiosity I filed
> https://bugs.chromium.org/p/chromium/issues/detail?id=827801 for that. Let's
> see what - if anything - they'd do.

And https://issuetracker.google.com/issues/77406791, although I fully expect nothing to happen for two or three years before it'll simply be closed as "Won't Fix (Obsolete)", even though nothing has changed about the situation.

Comment 12

[Andrei Lazar]

Attachment: Bug 1406903 Part 1: Import code for dealing with content:// URIs from aFileChooser. r=JanH

Added support for opening HTML files from internal storage when user is choosing fennec in the dialog picker.

Comment 13

[Vlad Baicu]

Comment on attachment 9012854 [details]
Bug 1406903 Part 1: Import code for dealing with content:// URIs from aFileChooser. r=JanH

Vlad Baicu has approved the revision.

Comment 14

[Andrei Lazar]

Attachment: Bug 1406903 cannot view files using content:// scheme r=JanH

Added support for opening HTML files from internal storage when user is choosing fennec in the dialog picker.

Comment 15

[Andrei Lazar]

Attachment: Bug 1406903 Part 2: Added support for opening HTML files. r=JanH

Added a part from FileUtils class with commit hash: 48d65e6 from https://github.com/iPaulPro/aFileChooser/blob/master/aFileChooser/src/com/ipaulpro/afilechooser/utils/FileUtils.java

Depends on D7489

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05d8e65ff651
Part 1: Import code for dealing with content:// URIs from aFileChooser. r=JanH,VladBaicu
https://hg.mozilla.org/integration/autoland/rev/8a510f2052d3
Part 2: Added support for opening HTML files. r=JanH

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/05d8e65ff651
https://hg.mozilla.org/mozilla-central/rev/8a510f2052d3

Comment 18

[Jan Henning [:JanH]]

This wasn't a regression, we just never got round to implementing this until now.

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from beta as requested by Marcia for causing Bug 1502721:

https://hg.mozilla.org/releases/mozilla-beta/rev/194d3585f3d567c047245c842ac5ee6b6390b474

Comment 20

[Andrei Lazar]

This is safe to land now, all defects caused by this feature have been fixed (see Bug 1502721, Bug 1499618 and Bug 1501688). Ryan, should I make an uplift request for this?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

There's nothing to uplift here AFAICT (it was backed out from 64 and 65 is already on Beta). Please do request uplift on bug 1501688 after it gets merged to m-c, though.

Comment 22

[Cosmin Sabou [:CosminS]]

Removing the checkin-needed until there's something to be uplifted here. Andrei feel free to put it back if you have something to be landed on integration branches or on beta. Thanks