1189957 - "Address wasn't understood" with Delta link

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Video: Loading Delta page fails

I attempted to open a link to a Delta flight confirmation, and Firefox claimed it couldn't open it.

This opens fine on desktop, and also Chrome Android, but on Firefox for Android, it only partly loads before bailing (I was able to see a glimpse of the loaded page with my flight info on a previous attempt).

I've replaced my confirmation code here, but this was the link:

http://e.delta.com/a/hBVtoNHB8LP6IB8$tL8NvvZp7.B8LP6IIo/dl44?RECORD_LOCATOR=*****&TRIP.1.SEGMENT.1.ORIGIN=SFO

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Log of page load

Comment 2

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

In bug 1162372, I added a path for intent:// URIs to open this about:neterror page if no applications were installed to handle the uri and this could be related. The patch is on 40+ so if this can be repro'd on Beta+, there is a high probability of correlation.

Comment 3

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

When opening on 39, we see the "Could not find an app to open this link" toast, which my patches removed in favor of the error page so I blame bug 1162372.

Presumably, 40-42 are also affected.

Debugging the page...

Comment 4

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

It looks like the page opens the intent:// uri in an iframe:
  b = 'flydelta-checkin://?origin=' + delta.utils.getParameterByName('airport_code') + '&confirmationNumber=' + delta.utils.getParameterByName('confirmation_no');
  $('<iframe id="deeplinkAppFrame" src="' + b + '" height="0"></iframe>').appendTo($('body'));

Chrome is not affected by this bug because they take no action when you visit a url to which no applications are registered.

Comment 5

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

This is a pretty slick user interaction - open the page and if it's the app is available, it opens, otherwise it opens in the browser - and it'd be a shame to break it. I think our options are:
  1) Do nothing and notify Delta about the compatibility issue. They should be able to use fallback uris [1] to produce the same result. Perhaps the reason they didn't implement this in the first place is because we only started supporting fallback uris in FF41 (bug 1168980).
  2) Copy chrome and take no action when an intent url without Android handlers is entered. This works more poorly for users when there are poorly constructed uris, however, this is on the web devs to do a better job (e.g. provide fallback uris).

I vote #1, however:
* The linking as-is is broken in 40+
* The fallback uris only work in 41+ so if Delta moves to fallback uris, linking will broken in 39 and more than broken in 40: if the intent uri w/ the fallback uri is loaded directly, we'll hit about:neterror, which is worse than taking no action.

I say we back bug 1162372 out of 40 and tell Delta to fix it by the 41 merge to release (9/21).

Margaret, what do you think?

[1]: https://developer.chrome.com/multidevice/android/intents

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

I don't think Delta will change their links in 6 weeks. It is a big web property.

Comment 7

[:Margaret Leibovic]

I definitely think we should back bug 1162372 out of 40. However, I also think we should consider option #2 until we can vet that websites are actually doing the right thing. Web compat is a hard battle, and Firefox for Android isn't web devs' #1 priority, so we may have to compromise.

Comment 8

[Mike Taylor [:miketaylr]]

Just to be clear (assuming we can get ahold of the right people at Delta), the recommendation for them is to change from the "flydelta-checking://" iframe to an intent:// link with a "S.browser_fallback_url=delta.com/whatever" a lá [1]?

[1] <https://developer.chrome.com/multidevice/android/intents#syntax>

Comment 9

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

(In reply to Mike Taylor [:miketaylr] from comment #8)
> Just to be clear (assuming we can get ahold of the right people at Delta),
> the recommendation for them is to change from the "flydelta-checking://"
> iframe to an intent:// link with a
> "S.browser_fallback_url=delta.com/whatever" a lá [1]?

Right. To add to that, my understanding is they're currently loading the `delta.com/<my-boarding-pass>` (now "boarding-pass-page"), which opens `flydelta-checking://...` in an iframe in the background. If the app is installed, the application will open over the browser and if the app is not installed, their expected flow is that no action occurs so that boarding-pass-page remains visible. However, we changed the code to go to an error web page when intent uris are loaded, so the page loads an error uri.

The Intent URI syntax is built to handle this case. Instead of loading boarding-pass-page, they can load the Intent URI directly with a fallback uri to boarding-pass-page.

Note that such links come from email so in that case, the page should load a webpage which redirects to the intent uri with the fallback uri.

To be clear, the new behavior was backed out of 40 and this problem will only take affect starting in FF41 (unless we make our changes).

---
Margaret, I realized that we could in theory load the error page in that non-visible iframe since that's where the intent uri is loaded from - do you have any idea how we might do that?

Comment 10

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

To be clear, we do our handling here:
  http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentDispatchChooser.js?rev=7c445f47c4d6#52

And load the url by sending Intent:OpenNoHandler to Java and opening the error page here:
  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/IntentHelper.java?rev=d43113d59aab#196

Comment 11

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #9)

> Margaret, I realized that we could in theory load the error page in that
> non-visible iframe since that's where the intent uri is loaded from - do you
> have any idea how we might do that?

Oh, I didn't realize that an error in an iframe was leading to a top-level error page, that feels wrong. I think we should only load an error page in the frame where there was an error.

As I mentioned in the bug where this was landed, I think we may want to follow up with some platform developers to see if there's a better way to be loading error pages. I guess the rough part is that we're handing this in Java, and Java only knows about tabs, not about frames.

Is there a way for us to know in Java which window this came from, and pass a message back to Gecko to have it load the proper error page? Maybe this is also something that snorp's team could help with.

Comment 12

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

Snorp, the issue here is that an intent URI gets loaded in an iframe, I initially get that in ContentDispatchChooser.ask [1], which I then push to Java [2] as it's more easily handled there. However, when I attempt to load pages (either for a fallback urls associated with the intent uri or an error page), I call Tabs.getInstance().loadUrl, which loads the page in the current tab, rather than the iframe that initially loaded the URI.

Is there a way that I can load these pages in the iframe instead?

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentDispatchChooser.js?rev=7c445f47c4d6#52
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/IntentHelper.java?rev=d43113d59aab#196

Comment 13

[Catalin Suciu [:csuciu]]

Verified on FF 40 RC build 1 that the changeset from Bug #1162372 was backed out and the old functionality was restored
Opening the link from description will show the "Couldn't find an app to open this link | Search" 
toast notification instead of the about:neterror page.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Michael Comella (:mcomella) from comment #12)
> Snorp, the issue here is that an intent URI gets loaded in an iframe, I
> initially get that in ContentDispatchChooser.ask [1], which I then push to
> Java [2] as it's more easily handled there. However, when I attempt to load
> pages (either for a fallback urls associated with the intent uri or an error
> page), I call Tabs.getInstance().loadUrl, which loads the page in the
> current tab, rather than the iframe that initially loaded the URI.
> 
> Is there a way that I can load these pages in the iframe instead?

Not easily. My suggestion would be to send a response message from IntentHelper ('Intent:OpenNoHandler:Response'), which would allow ContentDispatchChooser to load the fallback URI itself.

Comment 15

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #14)
> Not easily. My suggestion would be to send a response message from
> IntentHelper ('Intent:OpenNoHandler:Response'), which would allow
> ContentDispatchChooser to load the fallback URI itself.

What is the benefit of loading the url through ContentDispatchChooser? Do you mean to say it'd open in the iframe (that doesn't seem to difficult!). If so, how would I load a link from ContentDispatchChooser? I see aHandler.launchWithURI - is that what I'm looking for?

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Michael Comella (:mcomella) from comment #15)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #14)
> > Not easily. My suggestion would be to send a response message from
> > IntentHelper ('Intent:OpenNoHandler:Response'), which would allow
> > ContentDispatchChooser to load the fallback URI itself.
> 
> What is the benefit of loading the url through ContentDispatchChooser? Do
> you mean to say it'd open in the iframe (that doesn't seem to difficult!).
> If so, how would I load a link from ContentDispatchChooser? I see
> aHandler.launchWithURI - is that what I'm looking for?

Yes, in ContentDispatchChooser, you presumably have the requesting iframe in aWindowContext, which gets converted into a nsIDOMWindow. You can just set window.location.href to the fallback URI.

Comment 17

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

Attachment: MozReview Request: Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

Before we loaded the url into the open tab - this approach is more correct. I
verified it fixed the issues on the Delta website mentioned in this bug.

Comment 18

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

The patch that caused the issue got backed out of 40, and I can safely assume it's in 41.

Comment 19

[:Margaret Leibovic]

Comment on attachment 8645225 [details]
MozReview Request: Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

https://reviewboard.mozilla.org/r/15447/#review14037

Nice.

::: mobile/android/base/IntentHelper.java:151
(Diff revision 1)
> -    private void openNoHandler(final JSONObject msg) {
> +    private void openNoHandler(final NativeJSObject msg, final EventCallback callback) {

Could be good to document what this callback parameter expects to be called with.

::: mobile/android/base/IntentHelper.java:218
(Diff revision 1)
> -        final String errorUri = UNKNOWN_PROTOCOL_URI_PREFIX + encodedUri;
> +        return UNKNOWN_PROTOCOL_URI_PREFIX + encodedUri;

We know what URI we sent with the "Intent:OpenNoHandler" message, so technically we could use that to construct the error URI, right? Is there any reason to be doing this in Java and sending the URI as a message, as opposed to sending no data and constructing the error URI on the gecko side?

Looking through the logic a bit more, I now see that fallback URL handling above as a reason to do this, so maybe that answers my question :)

Comment 20

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

https://reviewboard.mozilla.org/r/15447/#review14037

> We know what URI we sent with the "Intent:OpenNoHandler" message, so technically we could use that to construct the error URI, right? Is there any reason to be doing this in Java and sending the URI as a message, as opposed to sending no data and constructing the error URI on the gecko side?
> 
> Looking through the logic a bit more, I now see that fallback URL handling above as a reason to do this, so maybe that answers my question :)

> Looking through the logic a bit more, I now see that fallback URL handling above as a reason to do this, so maybe that answers my question :)

Yep! :)

Comment 21

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

url:        https://hg.mozilla.org/integration/fx-team/rev/4a151c79684431a30b43770e696de98613f8e224
changeset:  4a151c79684431a30b43770e696de98613f8e224
user:       Michael Comella <[e-mail]>
date:       Fri Aug 07 16:19:23 2015 -0700
description:
Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

Before we loaded the url into the open tab - this approach is more correct. I
verified it fixed the issues on the Delta website mentioned in this bug.

url:        https://hg.mozilla.org/integration/fx-team/rev/a5d7ee79a10ce44cc108f649cbe3d4cd4946e6bd
changeset:  a5d7ee79a10ce44cc108f649cbe3d4cd4946e6bd
user:       Michael Comella <[e-mail]>
date:       Tue Aug 11 15:59:58 2015 -0700
description:
Bug 1189957 - review: Add comment describing callback parameter parameters in openNoHandler. r=me

Comment 22

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

Comment on attachment 8645225 [details]
MozReview Request: Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

This request applies to the two checked in patches (one of which was not pushed to reviewboard).

Approval Request Comment
[Feature/regressing bug #]: Intent URIs, which started with bug 851693. Bug 1162372 caused this regression.

[User impact if declined]: Users on some websites will have unexpected behavior compared to Chrome where Chrome takes no action (or performs unrelated actions) and we will take users to an error page (since we load the error page in the selected tab rather than the context in which the link is loaded – e.g. iframe).

[Describe test coverage new/current, TreeHerder]: None, tested locally
[Risks and why]: We're largely using the same code paths so there is minimal concern. However, I moved the listener from a JSON listener to a native listener, which I could have messed up, and we started passing messages back to JS, which I could have also messed up. However, there aren't many edge cases (it's pretty straightforward) so considering it works, I don't see many issues.

[String/UUID change made/needed]: None

Comment 23

[Flaviu Cos]

The same "Address wasn't understood" error message appears when I try to watch a video on youtube.
Tested with Asus Transformer Pad (Android 4.2.1) running Firefox 41 Beta 1.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a151c796844
https://hg.mozilla.org/mozilla-central/rev/a5d7ee79a10c

Comment 25

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

Comment on attachment 8645225 [details]
MozReview Request: Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 26

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

See comment 22 for uplift approval request.

(In reply to Flaviu Cos, QA [:flaviu] from comment #23)
> The same "Address wasn't understood" error message appears when I try to
> watch a video on youtube.
> Tested with Asus Transformer Pad (Android 4.2.1) running Firefox 41 Beta 1.

Seems like the patches had not landed in 43 yet – can you reverify?

Comment 27

[Flaviu Cos]

Verified as fixed in build 43.0a1 2015-08-13.
Device: Asus Transformer Pad (Android 4.2.1).

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8645225 [details]
MozReview Request: Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

Seems safe to uplift to Aurora given that the patch has baked in m-c for a few days. The fix was also verified on Nightly so seems safe. Will approve for Beta after letting it sit on Aurora for a day or two.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf8717b2a57c

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8645225 [details]
MozReview Request: Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

Beta+

Comment 31

[Flaviu Cos]

Verified as fixed in build 42.0a2 2015-08-16.
Device: Asus Transformer Pad (Android 4.2.1).

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eca173e6072e

Comment 33

[Flaviu Cos]

The issue is no longer reproducible on Firefox 41 Beta 2 using the link provided at comment 0, but "Address wasn't understood" message is still displayed when trying to watch a youtube video.

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Michael, do you think we should open a separate bug to address comment 33 or re-open this one? Thanks.

Comment 35

[Mike Taylor [:miketaylr]]

Please open a new bug with a youtube URL and STR, thanks!

Comment 36

[Flaviu Cos]

Verified as fixed in Firefox 41 Beta 2.
Device: Asus Transformer Pad (Android 4.2.1).

Comment 37

[Flaviu Cos]

I logged bug 1196212 for the "Address wasn't understood" issue on youtube videos.