Closed Bug 1189957 Opened 4 years ago Closed 4 years ago

"Address wasn't understood" with Delta link

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 --- verified
firefox42 --- verified
firefox43 --- verified
fennec 41+ ---

People

(Reporter: liuche, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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
Attached file Log of page load
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.
tracking-fennec: --- → ?
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...
Assignee: nobody → michael.l.comella
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.
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
Flags: needinfo?(margaret.leibovic)
I don't think Delta will change their links in 6 weeks. It is a big web property.
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.
Flags: needinfo?(margaret.leibovic)
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>
Flags: needinfo?(michael.l.comella)
(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?
Flags: needinfo?(michael.l.comella) → needinfo?(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.
Flags: needinfo?(margaret.leibovic)
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
Flags: needinfo?(snorp)
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.
(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.
Flags: needinfo?(snorp)
tracking-fennec: ? → 40+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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?
Flags: needinfo?(snorp)
(In reply to Michael Comella (:mcomella) from comment #15)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(snorp)
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.
Attachment #8645225 - Flags: review?(margaret.leibovic)
The patch that caused the issue got backed out of 40, and I can safely assume it's in 41.
tracking-fennec: 40+ → 41+
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 :)
Attachment #8645225 - Flags: review?(margaret.leibovic) → review+
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! :)
url:        https://hg.mozilla.org/integration/fx-team/rev/4a151c79684431a30b43770e696de98613f8e224
changeset:  4a151c79684431a30b43770e696de98613f8e224
user:       Michael Comella <michael.l.comella@gmail.com>
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 <michael.l.comella@gmail.com>
date:       Tue Aug 11 15:59:58 2015 -0700
description:
Bug 1189957 - review: Add comment describing callback parameter parameters in openNoHandler. r=me
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
Attachment #8645225 - Flags: approval-mozilla-aurora?
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.
https://hg.mozilla.org/mozilla-central/rev/4a151c796844
https://hg.mozilla.org/mozilla-central/rev/a5d7ee79a10c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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?
Flags: needinfo?(flaviu.cos)
Verified as fixed in build 43.0a1 2015-08-13.
Device: Asus Transformer Pad (Android 4.2.1).
Flags: needinfo?(flaviu.cos)
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.
Attachment #8645225 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8645225 [details]
MozReview Request: Bug 1189957 - Open noHandler results in the context they were loaded in. r=margaret

Beta+
Attachment #8645225 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed in build 42.0a2 2015-08-16.
Device: Asus Transformer Pad (Android 4.2.1).
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.
Michael, do you think we should open a separate bug to address comment 33 or re-open this one? Thanks.
Flags: needinfo?(michael.l.comella)
Please open a new bug with a youtube URL and STR, thanks!
Flags: needinfo?(michael.l.comella)
Verified as fixed in Firefox 41 Beta 2.
Device: Asus Transformer Pad (Android 4.2.1).
Status: RESOLVED → VERIFIED
I logged bug 1196212 for the "Address wasn't understood" issue on youtube videos.
You need to log in before you can comment on or make changes to this bug.