1431456 - send tab to device url is truncated

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[rdz_bug]

Attachment: _20180118_135912.JPG

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

1. opened the url "http://t-melbourne:8080/correicaovirtual-1.0.0/f/login.xhtml" on Nightly for Android 
2. used the send tab option to Firefox on Windows 7 32bit desktop.





Actual results:

3. opened said Firefox on desktop and it loaded a tab with the url "login.xhtml"


Expected results:

4. It should have opened "http://t-melbourne:8080/correicaovirtual-1.0.0/f/login.xhtml"

The example url is a local page that will only work on my job intranet. Don't even bother trying to open it. I don't know if it's a valid url. Maybe it isn't and that's why Firefox can't properly send it across devices.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Confirmed that Android -> other device is broken as described in comment 0. Desktop -> Android functions correctly.

Comment 2

[:Grisha Kruglov]

Taking a quick look: yup, in [0] we somehow parse URL in Comment 0 into just login.xhtml. Seems that the regex expression used (see [1]) isn't quite right.

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/overlays/ui/ShareDialog.java?q=path%3AShareDialog.java&redirect_type=single#223
[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/setup/activities/WebURLFinder.java#35

Comment 3

[:Grisha Kruglov]

Switching to use an equivalent of Patterns.WEB_URL_WITH_PROTOCOL improves the situation. I'll take a closer look at a broader context to make sure this won't regress anything and put together a patch.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #3)
> Switching to use an equivalent of Patterns.WEB_URL_WITH_PROTOCOL improves
> the situation. I'll take a closer look at a broader context to make sure
> this won't regress anything and put together a patch.

The exact regexes in Patterns.* vary by Android version; I think, at one time, we had copied some version for consistency across devices.  Updating the regex is not a bad idea... but add this as a test, and then run the tests, since the regex regresses some behaviour every time it is bumped.

Comment 5

[:Grisha Kruglov]

Attachment: Bug 1431456 - Pre: junit4-ify weburlfinder tests, add a few extra test cases

Review commit: https://reviewboard.mozilla.org/r/214718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214718/

Comment 6

[:Grisha Kruglov]

Attachment: Bug 1431456 - Bump url-matching regex patterns to their Android O counterparts

Review commit: https://reviewboard.mozilla.org/r/214720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214720/

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8944549 [details]
Bug 1431456 - Pre: junit4-ify weburlfinder tests, add a few extra test cases

https://reviewboard.mozilla.org/r/214718/#review220378

Thank you, sir.

::: mobile/android/services/src/test/java/org/mozilla/gecko/sync/setup/activities/WebURLFinderTest.java:37
(Diff revision 1)
> +
> +    @Test
> +    public void testFullURL() {
> +        assertEquals("http://scheme.com:8080/inner#anchor&arg=1", find("test.com http://scheme.com:8080/inner#anchor&arg=1"));
> +        assertEquals("http://s-scheme.com:8080/inner#anchor&arg=1", find("test.com http://s-scheme.com:8080/inner#anchor&arg=1"));
> +        assertEquals("http://t-example:8080/appversion-1.0.0/f/action.xhtml", find("test.com http://t-example:8080/appversion-1.0.0/f/action.xhtml"));

This fails now, right?  I would have prefered the Pre: to convert, and the test added with the code that makes it pass, but not worth re-slicing the series.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8944550 [details]
Bug 1431456 - Bump url-matching regex patterns to their Android O counterparts

https://reviewboard.mozilla.org/r/214720/#review220380

If it's green for you, it's good for me.  Stamp!

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8944550 [details]
Bug 1431456 - Bump url-matching regex patterns to their Android O counterparts

https://reviewboard.mozilla.org/r/214720/#review220382

Comment 10

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #7)
> This fails now, right?

In more ways than one. IIRC some existing tests also failed once I ran them a few days ago with the old regex setup.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c354536fd7f1
Pre: junit4-ify weburlfinder tests, add a few extra test cases r=nalexander
https://hg.mozilla.org/integration/autoland/rev/e8e57700030a
Bump url-matching regex patterns to their Android O counterparts r=nalexander

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c354536fd7f1
https://hg.mozilla.org/mozilla-central/rev/e8e57700030a