1064152 - Properly handle intent:// URIs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[:Margaret Leibovic]

For example, if you get a google maps result, you can click a link for directions, which launches an intent:// URI. Right now this doesn't even load properly in Fennec, so the experience is busted.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Were we working on Intent URIs at one point? Might have been security issues, I forget.

Comment 2

[Wesley Johnston (:wesj)]

Yeah. We decided we needed a prompt of some sort before handling them (even if they only match one installed app). We have more machinery in place for something like that now.

Comment 3

[:Margaret Leibovic]

Well, for the search activity, I think it makes sense to just launch the app chooser (or default to whatever the user set to handle that intent). It makes sense to default to Fennec for regular URLs, but there's no point opening Fennec just to handle an intent URI.

Also, you two should both be asleep.

Comment 4

[:Margaret Leibovic]

Looking into this, just launching an intent with these intent:// URIs isn't working. Looking more closely at these URIs, it looks like they include some meta data about launching the intent:

intent://maps.google.com/maps?um=1&ie=UTF-8&fb=1&sll=52.54114,13.44009&sspn=5.146615,11.2856986&q=Prenzlauer+Berg,+Germany&entry=s#Intent;scheme=http;package=com.google.android.apps.maps;end

I think this is actually something that's just handled by the WebView, so if we just avoid loading an intent at all in this case, the WebView will take care of it for us.

I found that if I get rid of the WebViewClient, this is the case. However, if change our link-intercepting code to not launch a new intent for an intent:// URI, it creates a page load error in the WebView. So maybe we do need to add special code to handle this ourselves.

I have a WIP here: https://github.com/mozilla/fennec-search/pull/84

I took this opportunity to do some refactoring to use shouldOverrideUrlLoading instead of canceling the page load in onPageStarted, which seems like the more correct thing to do. I was hoping that would fix this problem, but it didnt :(

Comment 5

[:Margaret Leibovic]

wesj, do you have any ideas what we should do here? Do you know if there's a simple way to parse these URIs to actually launch the right intent?

Comment 6

[Wesley Johnston (:wesj)]

You github link shows nothing to me. The Intent class has a built in way to parse these.
http://developer.android.com/reference/android/content/Intent.html#parseUri%28java.lang.String,%20int%29

Comment 7

[:Margaret Leibovic]

Attachment: Properly handle intent:// URIs

So there are really two issues here.

The first, more critical issue, is that we aren't properly parsing intent URIs, so Fennec is just failing to do anything useful when we try to load them. Using Intent.parseUri solves this problem.

The second, less important issue, is that I think we should honor the package name set in the intent URI, rather than forcing everything to open in Fennec. The maps URL I liked to above, for example, will open fine in Fennec if we force it to, but I think it's nicer to open it in the maps app since that's what it requested.

I also decided to add a different telemetry probe for this case, since this is different from us loading a URL in the browser. It would also be interesting to know how often users actually hit this. It might be a lot if they're using Google.

Asking mfinkle for feedback on the probe.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8492366 [details] [diff] [review]
Properly handle intent:// URIs

Probe looks fine. (LAUNCH, INTENT, "search-result") is unique enough for us to know where it originated from even though the Session might not be set to SEARCH at the time the probe fires.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8492366 [details] [diff] [review]
Properly handle intent:// URIs

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

Seems neat. A couple nits.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +95,3 @@
>  
> +        @Override
> +        public boolean shouldOverrideUrlLoading(WebView view, String url) {

Nothing you can do, but I kinda hate this method name.

@@ +108,5 @@
> +                // If the intent URI didn't specify a package, open this in Fennec.
> +                if (i.getPackage() == null) {
> +                    i.setClassName(AppConstants.ANDROID_PACKAGE_NAME, AppConstants.BROWSER_INTENT_CLASS_NAME);
> +                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL,
> +                            TelemetryContract.Method.CONTENT, "search-result");

Just curious, if you call startActivity in here and return true only in this if block (i.e. if we're not forcing this to open in Fennec, just let the WebView handle it), do things work. Is there any reason to do this over that?

@@ +117,5 @@
>  
> +                startActivity(i);
> +                return true;
> +            } catch (URISyntaxException e) {
> +                Log.e(LOG_TAG, "Error parsing intent URI: " + url, e);

Careful showing urls in logcat. Can be a security breach. Does this catch need to wrap so much code? I guess the exception is specific so it won't leak out. Up to you I guess :)

@@ +118,5 @@
> +                startActivity(i);
> +                return true;
> +            } catch (URISyntaxException e) {
> +                Log.e(LOG_TAG, "Error parsing intent URI: " + url, e);
> +                return false;

I'd move this return outside the catch.

Comment 10

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #9)

> @@ +108,5 @@
> > +                // If the intent URI didn't specify a package, open this in Fennec.
> > +                if (i.getPackage() == null) {
> > +                    i.setClassName(AppConstants.ANDROID_PACKAGE_NAME, AppConstants.BROWSER_INTENT_CLASS_NAME);
> > +                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL,
> > +                            TelemetryContract.Method.CONTENT, "search-result");
> 
> Just curious, if you call startActivity in here and return true only in this
> if block (i.e. if we're not forcing this to open in Fennec, just let the
> WebView handle it), do things work. Is there any reason to do this over that?

Just tried, and unfortunately no, the WebView gives an error about how it can't load the webpage at intent://...

> @@ +117,5 @@
> >  
> > +                startActivity(i);
> > +                return true;
> > +            } catch (URISyntaxException e) {
> > +                Log.e(LOG_TAG, "Error parsing intent URI: " + url, e);
> 
> Careful showing urls in logcat. Can be a security breach. Does this catch
> need to wrap so much code? I guess the exception is specific so it won't
> leak out. Up to you I guess :)

Oh, good point, I'll remove the URL. I thought it would help with debugging if we ever hit problems, but it's not worth it for the privacy implications.

And the catch wraps that much code since the Intent.parseUri call is what throws the exception. I suppose I could declare the variable above the try/catch, then return early if it doesn't get set, but I feel like I like keeping the logic all together like this.

Comment 11

[:Margaret Leibovic]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05645d479d89