Sharing a link with Sync not working with new TLD

VERIFIED FIXED in Firefox 50

Status

()

Firefox for Android
General
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: timo vn, Unassigned, Mentored)

Tracking

unspecified
Firefox 50
Points:
---

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
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.
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?
Component: Untriaged → Android Sync
Product: Firefox → Android Background Services
Version: 47 Branch → Firefox 47
(Reporter)

Comment 2

a year ago
Indeed, it worked on Android 5.0.1.
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.
Mentor: nalexander@mozilla.com, michael.l.comella@gmail.com
Status: UNCONFIRMED → NEW
Component: Android Sync → General
Ever confirmed: true
Product: Android Background Services → Firefox for Android
Whiteboard: [lang=java][good next bug]
Version: Firefox 47 → unspecified
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.
Duplicate of this bug: 1280611
@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?
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
Created attachment 8764805 [details] [diff] [review]
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.
Attachment #8764805 - Flags: review?(nalexander)
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.
Attachment #8764805 - Flags: review?(nalexander) → feedback+
Created attachment 8765092 [details] [diff] [review]
Patch attempt 2

In order to not rely on the Patterns class, I had to add all of these strings/patterns.
Attachment #8764805 - Attachment is obsolete: true
Attachment #8765092 - Flags: review?(nalexander)
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.
Attachment #8765092 - Flags: review?(nalexander) → review+
Created attachment 8766633 [details] [diff] [review]
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.
Attachment #8765092 - Attachment is obsolete: true
Attachment #8766633 - Flags: review?(nalexander)
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/
Attachment #8766633 - Flags: review?(nalexander) → review+
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.
Attachment #8766633 - Attachment is obsolete: true
Flags: needinfo?(nalexander)
Attachment #8767781 - Flags: review?(nalexander)
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!
Attachment #8767781 - Flags: review?(nalexander)
Attachment #8767781 - Flags: review?(ahunt)
Attachment #8767781 - Flags: review+
Flags: needinfo?(nalexander)
(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"));
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/95c5d3617db2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment on attachment 8767781 [details] [diff] [review]
patch

And I forgot to add the r+ here!
Attachment #8767781 - Flags: review?(ahunt) → review+
Verified as fixed in build 50.0a1 2016-07-14;
Device: Motorola Razr (Android 4.4.4).
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.