resource://android is not usable with extension code

RESOLVED FIXED in Firefox 67

Status

enhancement
P1
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: agi, Assigned: agi)

Tracking

(Blocks 1 bug)

Trunk
mozilla67
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Our current substitution code always returns a nsIFileURL even for cases where the URI might actually point to a jar: URI.

e.g., on android, resource://android/assets/FILENAME resolves to jar:file://path/to/apk!/assets/FILENAME but the nsIURI object returned from newURI does not implement nsIJARURI and instead implements nsIFileURL with the caveat that accessing it's .file member will crash.

This clashes with our Extension code that assumes that a nsIFileURL's .file accessor doesn't crash.

We need this to work if we want to offer GV customers a shorthand for accessing WebExtensions bundled with the apk.

I've done some investigation in the area, I'll summarize the situation a bit in following comments.

The ideal solution for us here would be for resource://android/* to return a nsIJARURI instead of a nsIFileURL, this however is not as straightforward as it may initially seem.

The resource: protocol handler collects a mapping of hosts -> base URIs and it resolves them lazily whenever needed to access the resource pointed by the URI, so when calling NewURI the nsIProtocolHandler instance doesn't actually know which URI will eventually be resolved.

We could hook in NewURI, create a nsIURI object and and resolve the URI and check what protocol is actually being resolved, the problem though is that in case of a jar: URI we would need to build a whole new nsIURI object to return, which looks not completely ideal.

We could do this just for resource://android by adding a new flag that controls whether the returned URI get's converted to a nsIJARURI or not if needed, so that the only extra cost is checking for the host of the URI, which should be acceptable.

Another option would be to piggyback on the android:// protocol which is currently only used for resolving icons at the moment.

We could add a new host called apk or similar that resolves to folders inside the apk. So e.g. android://apk/assets/FILENAME would resolve to the file /assets/FILENAME in the apk, by calling newURI on the resolved URI directly.

The biggest problem with this approach is that there are a lot of places where we check if the protocol is resource: and add some special handling. E.g. when running tests we allow content to load a page pointed by resource: but disallow for any other jar: URI. So we would either need to

(1) Add even more special handling for android: (which would still need a new nsSubstitutingJARURI similar to the existing nsSubstitutingURL) or
(2) Keep both android://apk and resource://android which basically do the same thing but return different interfaces

neither of these options are particularly appealing to me. Especially given that (1) would get us 90% of the way there for our ideal case described in #1.

Finally a last option would be to not support this at all. Let our embedders load extensions straight from a raw jar: URI. This is not ideal as

  • We expect this to be a very common cases, as most apps won't support user-defined WebExtensions
  • WebView has a short-hand for loading files from the APK
  • The way to define the correct jar: URI is not intuitive at all and not completely straightforward to figure out.

My plan is to go with #1.

agi has admirably summarized the situation. A few historical notes:

  1. The jar: URI is truly not intuitive but it's also not something we ever want to expose. It's a really strange Gecko implementation detail. I'd hate to not do something to make this simpler.

  2. The real reason for resource:// is, as agi noted, that resource:// is semi-privileged. Trying to work through all the privilege details for a new scheme (android://) wasn't feasible at the time I implemented Bug 948465.

Depends on: 948465
OS: All → Android
Priority: -- → P1

Ok from my limited testing I finally have a working version (see try here). I'm still considering whether to expose the jar interface to all resource:// URIs or just for android:, from what I can see it works in both cases.

I also found a precedence for this approach here where we use a nsSimpleURI to figure out where to dispatch the NewURI call.

Before this change, HasSubstitution would return false for "gre" or "app" which
is incorrect, since these handlers exist.

This is needed to make the handler to avoid race conditions where some code
tries to access a resource://android URI before the handler has been
registered.

Depends on D18739

resource://android URIs always point to a "jar:" URI so it doesn't make sense
that the returned URL object implements nsIFileURL.

This also makes it so extensions can be loaded from a resource://android URI.

Before this change, HasSubstitution would return false for "gre" or "app" which
is incorrect, since these handlers exist.

This is needed to make the handler to avoid race conditions where some code
tries to access a resource://android URI before the handler has been
registered.

Depends on D18757

Attachment #9041588 - Attachment description: Bug 1522137 - Fix HasSubstitution for special cases. r?mayhemer → Bug 1522137 - Fix HasSubstitution for special cases. r=mayhemer
Attachment #9041589 - Attachment description: Bug 1522137 - Move resource://android handler to C++. r?mayhemer! → Bug 1522137 - Move resource://android handler to C++. r=mayhemer!

Please cleanup the review queue, there seems to be duplicate patches r?me. Thanks.

I had marked them as abandoned but they are still showing up here. I've removed you from the reviewers hopefully they won't show up on your queue anymore!

Also thanks for the review :)

(FWIW, jar is a Gecko-only thingie.)

I'm still trying to figure out what this bug is about. We want to expose certain files inside some jars in some special way? And this isn't about nested jars, right?

Why does accessing .file crash?

No, this not about nested jars.

The motivation behind this is loading a web extension from the APK using a resource://android URL, e.g. resource://android/assets/ublock/.

Right now all resource:// URLs behave like they mapped to file:// URLs, and they expose .file from nsIFileURL.

But not all resource:// URLs actually map to file:, so the .file method fails for those URLs, see here, where we return NS_ERROR_NO_INTERFACE if the URL doesn't map to a File.

Problem is, on android resource:// always maps to a jar: URL, so exposing .file is not helpful and makes our WebExtension code unusable with resource:// URLs.

This bug is about making resource://android urls expose the jar: interface bits (aka nsIJARURI) instead of file:.

Does that help?

Attachment #9041633 - Attachment is obsolete: true
Attachment #9041632 - Attachment is obsolete: true

(still trying to get my mind over this all to understand why we'd need something special on Android.)

(In reply to Olli Pettay [:smaug] (PTO-ish Feb 16-23) from comment #16)

(still trying to get my mind over this all to understand why we'd need something special on Android.)

An Android package is basically a ZIP. Internally, there are multiple types of assets. GeckoView consumers pack assets into the assets/ directory at the top-level of the ZIP. GeckoView itself packs the omnijar there, too:

/assets
/assets/omni.ja (from GeckoView)
/assets/index.html (from consuming App)
...

This is about letting GeckoView access /assets/index.html in a "reasonable" way from within the Gecko web context. It's not reasonable to ask consumers of GeckoView to modify the omni.ja, so that Gecko's very special omnijar handling can be used: the omnijar is wholly controlled by GV itself.

So this is about mapping android://assets/index.html through to the /assets/index.html in the APK, not in the omnijar.

So this is about mapping android://assets/index.html through to the /assets/index.html in the APK, not in the omnijar.

minor nitpick, the URL is actually resource://android/assets/index.html.

To add to Nick's response, there's nothing android-specific in principle, we could expose the jar bits for all resource:// URLs that map to jar:. I'm doing it just for android since no-one seems to need it right now (and it can be easily enabled by adding RESOLVE_JAR_URI to the substitution).

oh, this is about accessing as web content, not as some internal thing.
And does something protect index.html accessing omni.ja?

And does something protect index.html accessing omni.ja?

Not really, index.html can read stuff from anywhere in the APK. Note that index.html is shipped by the GeckoView consumer, who can pretty much already do anything since it is in charge of packaging GeckoView with the consuming app.

This behavior is not changing with this bug FWIW.

I'm going to add some information that may be not clear already:

resource://android/assets/index.html maps today to jar:file://path/to/apk!/assets/index.html. This bug is not about adding new mappings. In fact, you can type resource://android/package-name.txt in the URL bar in fennec and it opens the /package-name.txt file in the APK.

This bug is about code like this not working today:

    if (this.rootURI instanceof Ci.nsIFileURL) {
      let uri = Services.io.newURI("./" + path, null, this.rootURI);
(*)   let fullPath = uri.QueryInterface(Ci.nsIFileURL).file.path;
      // ...
      return ...;
    }

    let uri = this.rootURI.QueryInterface(Ci.nsIJARURI);
    let file = uri.JARFile.QueryInterface(Ci.nsIFileURL).file;

Imagine this.rootURI = resource://android/path/to/webextension.

Today the rootURI instanceof Ci.nsIFileURL check returns true (incorrectly) and the code fails when it tries to access uri.QueryInterface(Ci.nsIFileURL).file at line (*).

After this bug is fixed, rootURI instanceof Ci.nsIFileURL would be false, and the code would correctly go to the following branch which handles the .QueryInterface(Ci.nsIJARURI) case.

I think because of bug 1161831 bholley should take a look at this.

Attachment #9041588 - Attachment description: Bug 1522137 - Fix HasSubstitution for special cases. r=mayhemer → Bug 1522137 - Fix HasSubstitution for special cases.
Attachment #9041589 - Attachment description: Bug 1522137 - Move resource://android handler to C++. r=mayhemer! → Bug 1522137 - Move resource://android handler to C++.
Attachment #9041592 - Attachment description: Bug 1522137 - Make resource://android return a nsIJARURI. r=mayhemer! → Bug 1522137 - Make resource://android return a nsIJARURI.

Comment 22

3 months ago
Pushed by asferro@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9a1181a0f15
Fix HasSubstitution for special cases. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/c79d6bd85dd8
Move resource://android handler to C++. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/54102692d385
Make resource://android return a nsIJARURI. r=snorp,mayhemer,bzbarsky

Comment 23

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.