Closed Bug 920855 Opened 6 years ago Closed 6 years ago

Stop using java.net.URL

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: ckitching, Assigned: accounts)

Details

(Whiteboard: [mentor=rnewman][lang=java][good first bug])

Attachments

(1 file, 5 obsolete files)

It's indescribably awful. We should never, ever be using this class - it does madness like resolving hostnames via DNS when computing hashCode...

java.net.URI should be used instead.
Assignee: chriskitching → nobody
Hardware: ARM → All
Summary: Stop using java.net.URL. → Stop using java.net.URL
Whiteboard: [mentor=rnewman][lang=java][good first bug]
Hi Chris,

I am new to open source and only have an academic background in development(Java, Android and some C). I was hoping for some guidance on getting starting. Thanks.
Flags: needinfo?(rnewman)
rahulb_2010: here's a good place to get started.

https://wiki.mozilla.org/Mobile/Get_Involved

Once you have Fennec building, you should be able to start wandering through mobile/android/base, looking for usages of URL and changing them (and method calls as appropriate) to URI.

Let us know (see the link above for contact methods) if you get stuck!
Flags: needinfo?(rnewman)
Hi Everyone,

Sorry for the delay in getting back to you, my VM crashed and its taken me a while to get it back to working order which is why I have been a bit quiet. Thanks for the tips Richard. I have a couple of follow-up questions.

1) I have my android device plugged in (Samsung Captivate running cynaogen rom 1.0.1.3 on android os 4.2.2) and built Fennec but right at the end I received this error message:

Adding client.mk options from /home/wolverine/Documents/Mozilla_Contribute/src/.mozconfig:
    CONFIG_GUESS=i386-linux-android
    FOUND_MOZCONFIG := /home/wolverine/Documents/Mozilla_Contribute/src/.mozconfig
make -j1 -C obj-i386-linux-android install
make[1]: Entering directory `/home/wolverine/Documents/Mozilla_Contribute/src/obj-i386-linux-android'
No devices are connected.  Connect a device or start an emulator.
make[1]: *** [install] Error 1
make[1]: Leaving directory `/home/wolverine/Documents/Mozilla_Contribute/src/obj-i386-linux-android'
make: *** [install] Error 2

I'm not sure why because I the device is plugged in and usb developer debugging is turned on.

2) I've gone through the /mobile/android/base folder and using grep found these files that use java.net.URL, is there any that I should not be editing since they reside in folders other than /mobile/android/base (none of the files showed up from the main folder)?

At the command prompt I typed: "grep -r java.net.URL *"

apache/commons/codec/net/URLCodec.java: * {@link java.net.URLEncoder} and {@link java.net.URLDecoder} 
background/announcements/AnnouncementsFetcher.java:import java.net.URLEncoder;
BrowserApp.java:import java.net.URLEncoder;
CrashReporter.java:import java.net.URL;
GeckoApp.java:import java.net.URL;
GeckoAppShell.java:import java.net.URL;
gfx/BitmapUtils.java:import java.net.URL;
home/SuggestClient.java:import java.net.URL;
home/SuggestClient.java:import java.net.URLEncoder;
httpclientandroidlib/client/utils/URLEncodedUtils.java:import java.net.URLDecoder;
httpclientandroidlib/client/utils/URLEncodedUtils.java:import java.net.URLEncoder;
ReferrerReceiver.java:import java.net.URLDecoder;
sync/Utils.java:import java.net.URLDecoder;
sync/repositories/domain/BookmarkRecord.java:import java.net.URLEncoder;
updater/UpdateServiceHelper.java:import java.net.URL;
updater/UpdateService.java:import java.net.URL;
updater/UpdateService.java:import java.net.URLConnection;
util/GeckoJarReader.java:import java.net.URL;
util/JSONUtils.java:import java.net.URL;
VideoPlayer.java:import java.net.URL;
VideoPlayer.java:import java.net.URLConnection;
WebAppImpl.java:import java.net.URL;

On another note I noticed that irc is the best way to contact the team for help, I haven't used irc that much before and wasn't sure and so I decided to post here. If there is the incorrect thing to do I apologize. Thanks for your help everyone.
(In reply to rahulb_2010 from comment #3)
> Adding client.mk options from
> /home/wolverine/Documents/Mozilla_Contribute/src/.mozconfig:
>     CONFIG_GUESS=i386-linux-android
>     FOUND_MOZCONFIG :=
> /home/wolverine/Documents/Mozilla_Contribute/src/.mozconfig
> make -j1 -C obj-i386-linux-android install
> make[1]: Entering directory
> `/home/wolverine/Documents/Mozilla_Contribute/src/obj-i386-linux-android'
> No devices are connected.  Connect a device or start an emulator.
> make[1]: *** [install] Error 1
> make[1]: Leaving directory
> `/home/wolverine/Documents/Mozilla_Contribute/src/obj-i386-linux-android'
> make: *** [install] Error 2
> 
> I'm not sure why because I the device is plugged in and usb developer
> debugging is turned on.

This problem is caused by adb not being able to detect your device.

If you're developing in a VM, it's most likely that this is caused by the host not sharing the USB device with the VM, so as far as the VM is concerned your phone does not exist.
While mucking about with VM settings trying to make this work, you can check for success by running `adb devices`.

> 2) I've gone through the /mobile/android/base folder and using grep found
> these files that use java.net.URL, is there any that I should not be editing
> since they reside in folders other than /mobile/android/base (none of the
> files showed up from the main folder)?
> 
> At the command prompt I typed: "grep -r java.net.URL *"

In general, -rsI might be a good choice.

> apache/commons/codec/net/URLCodec.java: * {@link java.net.URLEncoder} and
> {@link java.net.URLDecoder} 

This has matched a javadoc comment.

> httpclientandroidlib/client/utils/URLEncodedUtils.java:import
> java.net.URLDecoder;
> httpclientandroidlib/client/utils/URLEncodedUtils.java:import
> java.net.URLEncoder;

httpclientandroidlib is a library we're using. Unclear if it's wise to change it in this way.


> ReferrerReceiver.java:import java.net.URLDecoder;
> sync/Utils.java:import java.net.URLDecoder;
> sync/repositories/domain/BookmarkRecord.java:import java.net.URLEncoder;

Sync code needs to be edited via the Sync GitHub repository, instead of directly through hg for a variety of reasons rnewman can tell you more about.


The others seem to be okay...

> On another note I noticed that irc is the best way to contact the team for
> help, I haven't used irc that much before and wasn't sure and so I decided
> to post here. If there is the incorrect thing to do I apologize. Thanks for
> your help everyone.

IRC is usually more effective for getting quick responses from people - you'll want the #mobile channel.
That said, much of the team is in the US, so depending on where you live you might have been trying to contact people while everyone was asleep, which may explain your lack of success.
URLEncoder and URL are different classes.
I won't touch the URLEncoder classes at all, thanks Richard. 

I have been having a hard time trying to get my android device to connect through my vm (using VMWare player 6.0.0 and Ubuntu 12.04 LTS), my Windows 7 host machine is picking up the phone but I can't figure out how to get the vm to do it. Is anyone able to assist with this (I know it is unrelated to the Mozilla domain but I haven't really been able to find anything useful on the net as yet). Thanks.
Thanks for explaining this Chris

(In reply to Chris Kitching [:ckitching] from comment #4)
> (In reply to rahulb_2010 from comment #3)
> > Adding client.mk options from
> > /home/wolverine/Documents/Mozilla_Contribute/src/.mozconfig:
> >     CONFIG_GUESS=i386-linux-android
> >     FOUND_MOZCONFIG :=
> > /home/wolverine/Documents/Mozilla_Contribute/src/.mozconfig
> > make -j1 -C obj-i386-linux-android install
> > make[1]: Entering directory
> > `/home/wolverine/Documents/Mozilla_Contribute/src/obj-i386-linux-android'
> > No devices are connected.  Connect a device or start an emulator.
> > make[1]: *** [install] Error 1
> > make[1]: Leaving directory
> > `/home/wolverine/Documents/Mozilla_Contribute/src/obj-i386-linux-android'
> > make: *** [install] Error 2
> > 
> > I'm not sure why because I the device is plugged in and usb developer
> > debugging is turned on.
> 
> This problem is caused by adb not being able to detect your device.
> 
> If you're developing in a VM, it's most likely that this is caused by the
> host not sharing the USB device with the VM, so as far as the VM is
> concerned your phone does not exist.
> While mucking about with VM settings trying to make this work, you can check
> for success by running `adb devices`.
> 
> > 2) I've gone through the /mobile/android/base folder and using grep found
> > these files that use java.net.URL, is there any that I should not be editing
> > since they reside in folders other than /mobile/android/base (none of the
> > files showed up from the main folder)?
> > 
> > At the command prompt I typed: "grep -r java.net.URL *"
> 
> In general, -rsI might be a good choice.
> 
> > apache/commons/codec/net/URLCodec.java: * {@link java.net.URLEncoder} and
> > {@link java.net.URLDecoder} 
> 
> This has matched a javadoc comment.
> 
> > httpclientandroidlib/client/utils/URLEncodedUtils.java:import
> > java.net.URLDecoder;
> > httpclientandroidlib/client/utils/URLEncodedUtils.java:import
> > java.net.URLEncoder;
> 
> httpclientandroidlib is a library we're using. Unclear if it's wise to
> change it in this way.
> 
> 
> > ReferrerReceiver.java:import java.net.URLDecoder;
> > sync/Utils.java:import java.net.URLDecoder;
> > sync/repositories/domain/BookmarkRecord.java:import java.net.URLEncoder;
> 
> Sync code needs to be edited via the Sync GitHub repository, instead of
> directly through hg for a variety of reasons rnewman can tell you more about.
> 
> 
> The others seem to be okay...
> 
> > On another note I noticed that irc is the best way to contact the team for
> > help, I haven't used irc that much before and wasn't sure and so I decided
> > to post here. If there is the incorrect thing to do I apologize. Thanks for
> > your help everyone.
> 
> IRC is usually more effective for getting quick responses from people -
> you'll want the #mobile channel.
> That said, much of the team is in the US, so depending on where you live you
> might have been trying to contact people while everyone was asleep, which
> may explain your lack of success.
I've found all the files in android/mobile/base that use java.net.URL and changed them to java.net.URI and the calls as appropriate. To test it on my device do I build using make -f client.mk build_and_deploy and just play with it to see if it works(is there a better way to test as there were quite a few files to change/update)? Thanks.
Great progress!

You should:

* Build and run on your device.

  ./mach build && ./mach package && ./mach install

You'll see "Fennec rahul" (or similar) appear in your app launcher... or you'll see compile errors.

Run the app while running `adb logcat`, and make sure that things work and you don't see errors (that aren't StrictMode errors, anyway).

When your build completes and seems to work…

* If you have appropriate access, push a build to Try.

https://wiki.mozilla.org/Try#How_to_push_to_try


* Upload a patch with your changes so that one of us can review it. If you don't have Try access, we can do that part for you.
Hi Everyone,

I've been having some issues and am not entirely sure on how to go about solving this. I've made the changes from java.net.URL to java.net.URI now every bug I fix some more prop up. I just wanted to ensure that I should continue with this or if there is something else I should be doing?

Also regarding uploading a patch I'm not entirely sure how I should do that since I have had to go through many files to make corrections. How should I upload the patch? Thanks guys.
(In reply to Rahul from comment #10)
> Hi Everyone,
> 
> I've been having some issues and am not entirely sure on how to go about
> solving this. I've made the changes from java.net.URL to java.net.URI now
> every bug I fix some more prop up. I just wanted to ensure that I should
> continue with this or if there is something else I should be doing?

This shouldn't be a troublesome bug -- there are only a handful of uses of URL. We can give some guidance after taking a look at a patch.

> Also regarding uploading a patch I'm not entirely sure how I should do that
> since I have had to go through many files to make corrections. How should I
> upload the patch? Thanks guys.

You need to read this document, particularly the linked section:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Hey, guys. I'm a noob here, but figure I'd like to get involved if this is still available. After going through some 'mock' bug-fix scenarios I'm fairly comfortable with building/packaging and pushing fennec to my device. I went through GeckoAppShell.java and removed the java.net.URL import and changed the references/calls to work with java.net.URI. If this is open I'd like to take it.

Also I'd like to bounce some questions off you guys to make sure I'm following good practices here. Thanks!
(In reply to Tyler Coles from comment #12)
> Hey, guys. I'm a noob here, but figure I'd like to get involved if this is
> still available. After going through some 'mock' bug-fix scenarios I'm
> fairly comfortable with building/packaging and pushing fennec to my device.
> I went through GeckoAppShell.java and removed the java.net.URL import and
> changed the references/calls to work with java.net.URI. If this is open I'd
> like to take it.
> 
> Also I'd like to bounce some questions off you guys to make sure I'm
> following good practices here. Thanks!

There was someone else working on this a little while ago, but he seems no longer to be assigned, so looks like this is once again free.
In any case, two people solving the same problem as their first bug isn't a big deal if it gets you both up to speed and able to contribute more.

IRC is handy for getting lower-latency replies to quick questions. If you're confused, uploading a patch here and asking what's wrong with it might prove useful.

Sounds like you're already making some progress - keep going and ask questions when it goes wrong. You can't possibly break more things than I did ;).
Thanks, Chris! Hopefully I can be of some use here. 

Dumb question: in instances of the use of url.openStream() and url.openConnection(), we are looking for InputStream and URLConnection objects respectively. Is it valid to use a URI object and perform 'uri.toURL().openStream() or is that skirting around the problem as I am still utilizing a URL method?
(In reply to Tyler Coles from comment #14)
> Thanks, Chris! Hopefully I can be of some use here. 
> 
> Dumb question: in instances of the use of url.openStream() and
> url.openConnection(), we are looking for InputStream and URLConnection
> objects respectively. Is it valid to use a URI object and perform
> 'uri.toURL().openStream() or is that skirting around the problem as I am
> still utilizing a URL method?

In short, I don't think it matters.

The issue with URL is the way its equals and hashCode methods perform a DNS lookup on every call. Synchronously.
This is almost unfathomably stupid, but there we go. Check out this blog post for the full story:
http://blog.nirav.name/2011/12/avoid-storing-references-of-javaneturl.html

For more information about the difference between URLs, URIs, and URNs, check out the closing paragraph of the preamble of the URI javadoc:
http://docs.oracle.com/javase/7/docs/api/java/net/URI.html


Approximately, it seems that a sensible approach is to use URIs when you want a reference to a remote resource you do not yet want to interact with, but when you want to actually go download that remote resource you'll need to get a URL for it.

You may also want to read the appropriate RFCs:
http://www.ietf.org/rfc/rfc1738.txt
http://www.ietf.org/rfc/rfc2396.txt
http://www.ietf.org/rfc/rfc3986.txt
http://www.ietf.org/rfc/rfc2141.txt


Who'd have thought there'd me so much literature about conventions for referring to stuff?
Also, since you're apparently working on this I guess I should assign you to this bug and see if anyone complains...
Assignee: nobody → tcoles1
In short: what Chris said. Here we're only looking to avoid the use of URL where it's being used as URI -- as an identifier, wrapping some string rather than making network requests.
Status: NEW → ASSIGNED
Attached file GeckoAppShell.java (obsolete) —
I'm adding an attachment for .../mobile/android/base/GeckoAppShell.java. You guys can tell me if I'm totally off base here :)
Comment on attachment 833185 [details]
GeckoAppShell.java

Hey Tyler, could you follow the instructions here:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

to create a *patch*, rather than uploading the whole file?

Thanks!
Attachment #833185 - Attachment is obsolete: true
Attachment #833185 - Attachment mime type: text/x-java → text/plain
You may also be interested to read http://hginit.com - it has an excellent explanation of Mercurial and why it's useful.

Taking a diff, it seems that the only change in this version is this one:

@@ -1033,8 +1030,8 @@
                 // We are dealing with a URL
                 InputStream is = null;
                 try {
-                    URL url = new URL(aSrc);
-                    is = url.openStream();
+                    URI uri = URI.create(aSrc);
+                    is = uri.toURL().openStream();
                     byte[] buf = new byte[2048];
                     int length;

In this particular example, the URL object is only being used to get the stream. This is, annoyingly, the only use-case when we want to keep the use of URL. (Otherwise the extra allocation of a URI object is just redundant).
The use-cases you're looking to eliminate are, as Richard mentioned in Comment 17, ones where we're doing something with the URL except getting the stream and using it.
Sorry for the confusion! I guess my comments above haven't been entirely consistent with this point. Thanks for persevering in spite of apparently contradictory instructions!
Hi Everyone my apologies for being quiet for a bit had to re-do the changes I made earlier, messed up while trying to use hg commands to get the patch and kind of edited the .path file which added to my mess. I should have a patch for you to review in the next couple of days.

I have no issues with sharing the bug :). Thanks guys.
Attached patch Bug_920855.patch (obsolete) — Splinter Review
Hi Everyone,

Please find a copy of my patch attached for your review in comment 22. Sorry for the delay in getting it out to you. Thanks Guys.
Comment on attachment 8344424 [details] [diff] [review]
Bug_920855.patch

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

r- for the following reasons:

* You're touching files that you shouldn't -- .classpath, R.java, project.properties, etc.
* You're making changes that won't compile, and also won't allow the system to function.

I suggest you start from scratch and follow my advice inline for fixing the bug. And please make sure to compile and test the patch you submit!

::: mobile/android/base/CrashReporter.java
@@ +16,5 @@
>  import java.io.IOException;
>  import java.io.OutputStream;
>  import java.net.HttpURLConnection;
>  import java.net.URL;
> +import java.net.URI;

There are no other changes in this file, so this can be removed.

::: mobile/android/base/GeckoApp.java
@@ +143,5 @@
>      private static final String LOGTAG = "GeckoApp";
>  
>      private static enum StartupAction {
>          NORMAL,     /* normal application start */
> +        URI,        /* launched with a passed URL */

Find And Replace is not a good thing to do, for this reason.

@@ +994,1 @@
>                  is = url.openStream();

You need to make sure that valid uses of URL continue to be URL. This is one of them; there is no `openStream` method on URI.

You can't just s/URI/URL in the entire codebase and expect things to work; you need to find uses of URL, assess them, make the appropriate changes to the ones that should be changed... and then make sure that the system actually builds, which it will not with this patch applied.
Attachment #8344424 - Flags: review-
Hi everyone,

since there was no activitiy on this bug for several months I thought I will give it a try.
I have changed following files, but I'm not sure if the changes make always sense:

mobile/android/base/mobile/android/base/CrashReporter.java
URL is only used to open a connection.

mobile/android/base/GeckoApp.java

mobile/android/base/GeckoAppShell.java
Deleted unused imports only.

mobile/android/base/VideoPlayer.java
URL is only used to open a connection.

mobile/android/base/WebappImpl.java

mobile/android/base/gfx/BitmapUtils.java
public static method decodeUrl is changed. Is this allowed or might this break other functionality?

mobile/android/base/home/SuggestClient.java

mobile/android/base/updater/UpdateService.java
mobile/android/base/updater/UpdateServiceHelper.java
public static method in USH is changed. Again, this might break external functionality.

mobile/android/base/util/GeckoJarReader.java

mobile/android/base/util/JSONUtils.java

mobile/android/base/webapp/InstallListener.java
Deleted unused imports only.

mobile/android/base/webapp/WebappImpl.java

I have not changed thirdparty/com/squareup/picasso/UrlConnectionDownloader.java as it is in the thirdparty folder.

Could you please have a look at it? 

Thank you in advance and kind regards, Clemens
Attached patch 920855.patch (obsolete) — Splinter Review
See comment above.
Attached patch 920855_2.patch (obsolete) — Splinter Review
I've re-read the comments again and changed my patch (added a new one and obsoleted the other). I've removed all the changes where the URL is only used to open a connection.
Attachment #8402667 - Attachment is obsolete: true
Comment on attachment 8402762 [details] [diff] [review]
920855_2.patch

If you have questions about a patch you should use the feedback or review flags with the mentor's name. See https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
Attachment #8402762 - Flags: feedback?(rnewman)
Thanks Kevin. I totally forgot about the flags.
Comment on attachment 8402762 [details] [diff] [review]
920855_2.patch

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

Thanks for the patch!

Please undo the changes in BitmapUtils and UpdateService -- those both legitimately use URL, so let's save the creation of the intermediate URI.

With those gone this is probably a safe change, but let's make sure that tests continue to pass -- upload an updated patch, and I'll run it through Try.

::: mobile/android/base/gfx/BitmapUtils.java
@@ +200,3 @@
>      }
>  
> +    public static Bitmap decodeUrl(URI uri) {

There's no point making the changes in this file -- all of the URIs are just used to make URLs.

::: mobile/android/base/updater/UpdateService.java
@@ +277,5 @@
>              mNotificationManager.notify(NOTIFICATION_ID, notification);
>          }
>      }
>  
> +    private URLConnection openConnectionWithProxy(URI uri) throws java.net.URISyntaxException, java.io.IOException {

No need for this change.

@@ +294,5 @@
>      }
>  
>      private UpdateInfo findUpdate(boolean force) {
>          try {
> +            URI uri = UpdateServiceHelper.getUpdateUri(this, force);

No need for this, given above.

@@ +413,5 @@
>          mNotificationManager.notify(NOTIFICATION_ID, notification);
>      }
>  
>      private File downloadUpdatePackage(UpdateInfo info, boolean overwriteExisting) {
> +        File downloadFile = new File(Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS), new File(info.uri).getName());

I don't think this is correct.

The URI is something like

  https://aus4.mozilla.org/update/4/a/b/c/.../update.xml

URL#getFile():

  Gets the file name of this URL.
  The returned file portion will be the same as getPath(), plus the concatenation of the value of getQuery(), if any. If there is no query portion, this method and getPath() will return identical results.

... so

   new File(info.url.getFile()).getName()

is just a roundabout way of getting the string "update.xml". By contrast:


new File(URI):
  
  Creates a new File instance by converting the given file: URI into an abstract pathname. 


So I doubt that this change works at all. You might consider just reverting this whole file.

::: mobile/android/base/util/JSONUtils.java
@@ +17,5 @@
>      private static final String LOGTAG = "JSONUtils";
>  
>      private JSONUtils() {}
>  
> +    public static URI getURI(String name, JSONObject json) {

These two methods aren't called at all. Just delete them.
Attachment #8402762 - Flags: feedback?(rnewman) → feedback+
Attached patch patch for Bug 920855 (obsolete) — Splinter Review
Hi Richard,

thank you very much for your feedback, I've updated the code to match your feedback and created a new patch. Could you please review this and push it to the try server?

Thanks, Clemens
Attachment #8402762 - Attachment is obsolete: true
Attachment #8404876 - Flags: review?(rnewman)
Attachment #8404876 - Flags: review?(rnewman) → review+
Assignee: tcoles1 → rnewman
Comment on attachment 8407287 [details] [diff] [review]
Replace java.net.URL with java.net.URI wherever possible.

Fixed commit message.

Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=7d3ce7978bf0
Attachment #8407287 - Flags: review+
Attachment #8344424 - Attachment is obsolete: true
Attachment #8404876 - Attachment is obsolete: true
Assignee: rnewman → clemens.wilding+firefox
Landed:

https://hg.mozilla.org/integration/fx-team/rev/1ebef0fc77de

Thanks for picking this up, Clemens! I've given you a shout-out in next week's mobile team meeting:

https://wiki.mozilla.org/Mobile/Notes/23-Apr-2014
Target Milestone: --- → Firefox 31
https://hg.mozilla.org/mozilla-central/rev/1ebef0fc77de
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.