957059 - figure out why URL.sameFile doesn't ignore query

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

We should figure out why URL.sameFile() doesn't seem to ignore the query part of the URL when called by InstallListener.isCorrectManifest.

Comment 1

[Martyn Haigh (:mhaigh)]

http://developer.android.com/reference/java/net/URL.html#sameFile%28java.net.URL%29

"Returns true if this URL refers to the same resource as otherURL. All URL components except the reference field are compared. "

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: 0001-957059-re-write-URL-matching-code-to-prevent-trying-.patch

Have rewritten the isCorrectManifest method to strip off the querystring before comparing the manifest URLs

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8359873 [details] [diff] [review]
0001-957059-re-write-URL-matching-code-to-prevent-trying-.patch

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

This seems like a reasonable approach.

::: mobile/android/base/webapp/InstallListener.java
@@ +83,5 @@
>      }
>  
>      public boolean isCorrectManifest(String manifestUrl) {
> +        // Don't use URL and the sameFile method as this also includes the query which
> +        // we want to ignore 

Nits: trailing space and missing period.

@@ +90,3 @@
>          try {
> +            registeredUrl = mManifestUrl.indexOf("?") > 0 ? mManifestUrl.split("\\?")[0] : mManifestUrl;
> +            observedUrl = manifestUrl.indexOf("?") > 0 ? manifestUrl.split("\\?")[0] : manifestUrl;

Comparing the indexes to 0 is highly likely to work, but technically they should be compared to -1, since 0 is the first index of the string, so "?".indexOf("?") == 0.

Nevertheless, I don't think you need this comparison at all, since String.split returns an array with a single item containing the entire string if you call String.indexOf("?") on a string that doesn't contain a question mark.

So this can be simply:

    registeredUrl = mManifestUrl.split("\\?")[0];
    observedUrl = manifestUrl.split("\\?")[0];

@@ +90,5 @@
>          try {
> +            registeredUrl = mManifestUrl.indexOf("?") > 0 ? mManifestUrl.split("\\?")[0] : mManifestUrl;
> +            observedUrl = manifestUrl.indexOf("?") > 0 ? manifestUrl.split("\\?")[0] : manifestUrl;
> +        } catch (NullPointerException e) {
> +            Log.e(LOGTAG, "One or both of the manifest URLs is null", e);

Can we simply fall through to the return statement below at this point, or will that statement then throw an exception?

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Changes based on feedback

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8362524 [details] [diff] [review]
Changes based on feedback

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

Looks good, but we'll need a Fennec peer like WesJ to do the review.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8362524 [details] [diff] [review]
Changes based on feedback

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

I should have caught this earlier, but we're trying not to use URL anymore. See bug 920855. Thankfully, that's what this does!

::: mobile/android/base/webapp/InstallListener.java
@@ +89,3 @@
>          try {
> +            registeredUrl = mManifestUrl.split("\\?")[0];
> +            observedUrl = manifestUrl.split("\\?")[0];

Just for simplicity, I'd rather you created your Strings and returned in here. i.e. make this:

try {
  String s1 = ...
  String s2 = ...
  return s1.equals(s2);
} catch(ex) { }
return false;

Comment 7

[Martyn Haigh (:mhaigh)]

Attachment: Refactor isCorrectManifest function to not match querystring and beyond in provided URLs

Updated code based on feedback

Comment 8

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f777e85ac442

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f777e85ac442