1176018 - Firefox 41 won't open downloaded files anymore

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[alessandro]

Attachment: Screenshot_2015-06-18-19-50-30.jpg

User Agent: Mozilla/5.0 (Android; Mobile; rv:41.0) Gecko/41.0 Firefox/41.0
Build ID: 20150617030205

Steps to reproduce:

Firefox nightly41 wont open apk downloaded  anymore
i m absolutely sure that until a week ago Firefox nighty 41 could open apk downloaded it was so convenient over. chrome i could just tap the notification in lollipop and the all file will install
.
now that wont happen anymore Firefox says no app if found to open all see screenshot.

Comment 1

[Loic]

Any link to download apk and test?

Comment 2

[alessandro]

every app i download from apkmirror.com the only site i trust and i ve ever used for comparison when i state that last week i could install all tapping download complete notification banner

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

It seems like this affects more than APK files. In addition to that I'm unable to open PDF files from about:downloads. Beta is not affected.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

W/GeckoAppShell( 6420): android.content.ActivityNotFoundException: No Activity found to handle Intent { act=android.intent.action.VIEW cat=[android.intent.category.BROWSABLE] dat=file:///storage/emulated/0/Download/history-foxkeh(2).pdf typ=application/pdf flg=0x4000000 (has extras) }
W/GeckoAppShell( 6420): 	at android.app.Instrumentation.checkStartActivityResult(Instrumentation.java:1781)
W/GeckoAppShell( 6420): 	at android.app.Instrumentation.execStartActivity(Instrumentation.java:1501)
W/GeckoAppShell( 6420): 	at android.app.Activity.startActivityForResult(Activity.java:3745)
W/GeckoAppShell( 6420): 	at android.app.Activity.startActivityForResult(Activity.java:3706)
W/GeckoAppShell( 6420): 	at android.support.v4.app.FragmentActivity.startActivityForResult(Unknown Source)
W/GeckoAppShell( 6420): 	at org.mozilla.gecko.GeckoActivity.startActivityForResult(GeckoActivity.java:68)
W/GeckoAppShell( 6420): 	at android.app.Activity.startActivity(Activity.java:4016)
W/GeckoAppShell( 6420): 	at android.app.Activity.startActivity(Activity.java:3984)
W/GeckoAppShell( 6420): 	at org.mozilla.gecko.GeckoActivity.startActivity(GeckoActivity.java:62)
W/GeckoAppShell( 6420): 	at org.mozilla.gecko.GeckoAppShell.openUriExternal(GeckoAppShell.java:1112)
W/GeckoAppShell( 6420): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
W/GeckoAppShell( 6420): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:385)
W/GeckoAppShell( 6420): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:188)

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Regression introduced here:
http://hg.mozilla.org/mozilla-central/rev/cb07547fd9ab

Previously Intent.CATEGORY_BROWSABLE was only added to Intents that contained an intent:// URI. Now the category is added to all outgoing intents. This seems to break intent resolution for these cases here.

NI, Michael: I can't really trace back why exactly this was added for all intents? From the documentation it makes sense somehow: "Activities that can be safely invoked from a browser must support this category.". But it seems to break all kinds of stuff here. We probably want to have this for all intents fired up from website links but not for about:downloads, which happens to be a website too.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1176018_file_uri.patch

Comment 7

[:Margaret Leibovic]

Comment on attachment 8627250 [details] [diff] [review]
1176018_file_uri.patch

Review of attachment 8627250 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable to me, but we should chat with mcomella when he gets back from PTO to make sure there isn't any nuance we're missing here.

::: mobile/android/base/GeckoAppShell.java
@@ +1197,5 @@
>          final Intent intent = getOpenURIIntentInner(context, targetURI, mimeType, action, title);
>  
>          if (intent != null) {
> +            // Setting category on file:// URIs breaks about:downloads (Bug 1176018)
> +            if (targetURI == null || !targetURI.startsWith("file:")) {

If targetURI is expected to be null, we would already be vulnerable to an NPE here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#1221

So I don't think we need this null check.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #7)
> This seems reasonable to me, but we should chat with mcomella when he gets
> back from PTO to make sure there isn't any nuance we're missing here.

Yeah, I agree -> leave-open.

(In reply to :Margaret Leibovic from comment #7)
> If targetURI is expected to be null, we would already be vulnerable to an
> NPE here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> GeckoAppShell.java#1221

Oh, yeah, I'll remove the check.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1176018_file_uri_v2.patch

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/b7f643093ce7

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7f643093ce7

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Sebastian Kaspari from comment #5)
> NI, Michael: I can't really trace back why exactly this was added for all
> intents? From the documentation it makes sense somehow: "Activities that can
> be safely invoked from a browser must support this category.".

This was my assumption.

> But it seems to break all kinds of stuff here.

And what I hoped would not happen - in theory, it shouldn't.

If there was a good way to trace where the calls were coming from (i.e. is this from an external URI?), this would be easy but it seems all the calls are funneled through that IntentHelper method (which specifically opens external URIs where my assumption about requiring the BROWSABLE category seemed to make sense).

I'm open to suggestions though! :)

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #13)
> If there was a good way to trace where the calls were coming from (i.e. is
> this from an external URI?), this would be easy but it seems all the calls
> are funneled through that IntentHelper method (which specifically opens
> external URIs where my assumption about requiring the BROWSABLE category
> seemed to make sense).

Yeah, that's how I wanted to solve it too - at first. I guess a proper fix would be to distinguish between links from websites and web UI (mostly about:foo) but I ended up in the same dead-end in IntentHelper. The workaround to exclude file:// uris works for about:downloads and I don't know about any other broken Intents so far. But now a website could still contain links using the file scheme and therefore fire Intents without CATEGORY_BROWSABLE. I can't come up with a scenario where this might be used to exploit something but I would like to avoid it nevertheless.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Argh. It seems like Nightly update downloads are also affected by this - resulting in Nightly not being able to update itself.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Sebastian Kaspari from comment #16)
> Argh. It seems like Nightly update downloads are also affected by this -
> resulting in Nightly not being able to update itself.

Okay, it's not related. That's bug 1179382. Phew. Panic! :)

Comment 18

[Ioana Chiorean]

This migrated to Aurora now ( taking nightly out from title)

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]:

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

NI: Mike: Should we uplift the fix to Aurora or do you think we can come up with a better patch? Maybe uplifting and filing a follow-up to explore ways to improve our intent handling is the best we can do for now?

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Sebastian Kaspari from comment #20)
> NI: Mike: Should we uplift the fix to Aurora or do you think we can come up
> with a better patch? Maybe uplifting and filing a follow-up to explore ways
> to improve our intent handling is the best we can do for now?

Let's uplift - whatever better patch we come up with will be more complex and less tested, and thus riskier to uplift.

If we're really concerned the BROWSABLE change is breaking more features, we should back it out.

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8627271 [details] [diff] [review]
1176018_file_uri_v2.patch

Approval Request Comment
[Feature/regressing bug #]: #1176018
[User impact if declined]: Downloaded files cannot be opened
[Describe test coverage new/current, TreeHerder]: Patch has been in Nightly for about a week.
[Risks and why]: Low risk, restoring old behavior for handling of file:// uris
[String/UUID change made/needed]: -

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(Optimistic Mike:) Over in bug 1173200, file:// urls seem to need special handling (they need explicit mimetypes while other uris do not). Notably, Chrome lets the Downloads Manager handle it's files so maybe it's necessary and we shouldn't feel so bad about all this special-casing.

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

But this still needs an uplift to Aurora/41.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Sebastian Kaspari from comment #24)
> But this still needs an uplift to Aurora/41.

Bugs get marked fixed if they land on m-c - we use the "Tracking flags", specifically "status-firefox41", to track whether or not this has been uplifted. The "tracking-fennec" flag, in conjunction with the auto-fill in the weekly meetings notes, can help you find bugs you have yet to uplift.

Comment 26

[Kate Glazko]

Comment on attachment 8627271 [details] [diff] [review]
1176018_file_uri_v2.patch

Low risk, restoring old behaviors. Patch has been in nightly for about a week.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8627271 [details] [diff] [review]
1176018_file_uri_v2.patch

Actually, I'm going to fix the underlying issue that caused this bug for 41 only in bug 1182328.

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

41 fixed by bug 1182328.