Closed Bug 957059 Opened 6 years ago Closed 6 years ago

figure out why URL.sameFile doesn't ignore query

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: myk, Assigned: mhaigh)

References

Details

Attachments

(1 file, 2 obsolete files)

We should figure out why URL.sameFile() doesn't seem to ignore the query part of the URL when called by InstallListener.isCorrectManifest.
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. "
Assignee: nobody → mhaigh
Priority: -- → P2
Have rewritten the isCorrectManifest method to strip off the querystring before comparing the manifest URLs
Attachment #8359873 - Flags: feedback?(myk)
Attachment #8359873 - Flags: feedback?(jhugman)
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?
Attachment #8359873 - Flags: feedback?(myk) → feedback+
Attached patch Changes based on feedback (obsolete) — Splinter Review
Attachment #8359873 - Attachment is obsolete: true
Attachment #8359873 - Flags: feedback?(jhugman)
Attachment #8362524 - Flags: review?(myk)
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.
Attachment #8362524 - Flags: review?(wjohnston)
Attachment #8362524 - Flags: review?(myk)
Attachment #8362524 - Flags: feedback+
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;
Attachment #8362524 - Flags: review?(wjohnston) → review+
Updated code based on feedback
Attachment #8362524 - Attachment is obsolete: true
Attachment #8363593 - Flags: review?(wjohnston)
Attachment #8363593 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/f777e85ac442
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.