1280794 - Sharing a link with Sync not working with new TLD

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[timo vn]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160609214634
Firefox for Android

Steps to reproduce:

I use Firefox 47 on Android 4.4.4.

Step to reproduce :
– go to URL with "new" TLDs, like : http://practical.engineering/
– Try to share (from android) over Mozilla Sync


Actual results:

I get a popup saying "no URL to share has been found" (own translation from the french message)


Expected results:

Link should be shared like any other .com, .net… link.

It might that not all TLDs are causing this.

Comment 1

[Théo Chevalier]

Trying to reproduce, it works for me using both Firefox 47 and latest Nightly on Android 5.1.1 (CM 12.1). 

Could it be related to Android version?

Comment 2

[timo vn]

Indeed, it worked on Android 5.0.1.

Comment 3

[Nick Alexander :nalexander [he/him]]

timo vn, thanks for the bug report!  tchevalier, this does indeed depend on the Android version.  We use a built-in to match URLs; see https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/setup/activities/WebURLFinder.java#93.

If we want to standardize this, we can extract the regular expression from a particular version of the Android source code (latest, I guess) and use it directly.  I'm not going to work on this myself, but I'll mentor this ticket.

Comment 4

[Nick Alexander :nalexander [he/him]]

To address this, we'd want to extract the regular expression from around http://androidxref.com/6.0.1_r10/xref/frameworks/base/core/java/android/util/Patterns.java#146 into Fennec's source code, or include the whole Patterns.java file into the tree, and reference that.

This isn't a good first bug, but it is a good next bug.

Comment 6

[Nicholas Rosbrook [:enr0n]]

@nalexander: I'll work on this bug. If we take the route of snagging the regex itself from the android source, where would we want to put it so that we could use it as a string resource?

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Nicholas Rosbrook [:enr0n] from comment #6)
> @nalexander: I'll work on this bug.

Great!

 If we take the route of snagging the
> regex itself from the android source, where would we want to put it so that
> we could use it as a string resource?

I'd just copy the regex strings you need into the existing WebURLFinder.java file.  No sense having them be Android string resources -- they'll never be translated, etc.

Comment 8

[Nicholas Rosbrook [:enr0n]]

Attachment: Patch attempt 1

Here's my first try at it. I'm currently android-less, so if someone has the capability to test on 4.4.4, that would be appreciated.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8764805 [details] [diff] [review]
Patch attempt 1

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

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/setup/activities/WebURLFinder.java
@@ +15,5 @@
>  
>  import android.util.Patterns;
>  
>  public class WebURLFinder {
> +  public static final String WEB_URL_REGEX = "((?:(http|https|Http|Https|rtsp|Rtsp):\\/\\/(?:(?:[a-zA-Z0-9\\$\\-\\_\\.\\+\\!\\*\\'\\(\\)"

This is a good approach but we need to lift the underlying `Patterns.DOMAIN_NAME` and `Patterns.GOOD_IRI_CHAR` as well.  The DOMAIN_NAME in particular, since that's what is changing underneath us.

Comment 10

[Nicholas Rosbrook [:enr0n]]

Attachment: Patch attempt 2

In order to not rely on the Patterns class, I had to add all of these strings/patterns.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8765092 [details] [diff] [review]
Patch attempt 2

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

This part looks good.  I'd like to see the class comment before we land.

Also, there are some tests at https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/sync/TestWebURLFinder.java.  Do those tests still pass?  Could you try to add tests for a new TLD (that failed before)?

Looking good!

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/setup/activities/WebURLFinder.java
@@ +15,3 @@
>  
>  public class WebURLFinder {
> +  public static final String GOOD_IRI_CHAR = "a-zA-Z0-9\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF";

Looks good!  Add a class comment explaining how these are all lifted from Patterns, and why we inlined them; a link to the exact version, etc, would be helpful too.

Comment 12

[Nicholas Rosbrook [:enr0n]]

Attachment: Patch 3

Here's the patch with the class comment. I'm in the process of replacing my android device--so I'll test as soon as possible.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8766633 [details] [diff] [review]
Patch 3

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

This looks good!  Thanks for picking this up, Nicholas.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/setup/activities/WebURLFinder.java
@@ +15,5 @@
>  
>  public class WebURLFinder {
> +  /**
> +   * These regular expressions are taken from Android's Patterns.java.
> +   * We brought them in to standardize URL matching, instead of relying

s/matching/matching across Android versions/

s/built-ins/built-ins that can vary across Android versions/

Comment 14

[Nicholas Rosbrook [:enr0n]]

Attachment: patch

Updated the class comment.

I tried running the tests using mach test TestWebURLFinder, but mach said that it couldn't run an instrumentation test yet.

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8767781 [details] [diff] [review]
patch

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

ahunt, would you mind taking over this patch?  It's r=me, but could you push this to try and coordinate landing?  I don't have a tree, etc.  Thanks!

Comment 16

[Andrzej Hunt :ahunt]

(In reply to Nicholas Rosbrook [:enr0n] from comment #14)
> Created attachment 8767781 [details] [diff] [review]
> patch
> 
> Updated the class comment.
> 
> I tried running the tests using mach test TestWebURLFinder, but mach said
> that it couldn't run an instrumentation test yet.

With these tests, I *think* the only way to run them is within Android Studio.

I.e. open TestWebURLFinder.java, right-click on the filename (either in the file-tree, or the editor tab), select "Run TestWebURLFinder.java".

That's failing with the following error, but it seems that this failure happens even without your changes (that's a side-effect of these tests not being run in automation). I think it's therefore probably safe to push your changes - could you maybe file a bug to follow up that failure

> junit.framework.AssertionFailedError: Expected: <null> but was: test.js
> at org.mozilla.gecko.background.sync.TestWebURLFinder.testNoBadScheme(TestWebURLFinder.java:40)

at
> assertNull(find("file:///test javascript:///test.js"));

Comment 17

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/95c5d3617db2355aed67724a9d8180d9c6cd0467
Bug 1280794 - Updated WebURLFinder to use new WebURL regular expression that supports new TLDs r=nalexander,ahunt

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/95c5d3617db2

Comment 19

[Andrzej Hunt :ahunt]

Comment on attachment 8767781 [details] [diff] [review]
patch

And I forgot to add the r+ here!

Comment 20

[Sorina Florean [:sflorean]]

Verified as fixed in build 50.0a1 2016-07-14;
Device: Motorola Razr (Android 4.4.4).