[RFC][icedtea-web:netx]: findbugs "bug"fixes
Omair Majid
omajid at redhat.com
Wed Mar 9 09:20:51 PST 2011
On 03/09/2011 10:17 AM, Denis Lila wrote:
> Hi.
>
> The attached patch includes some more fixes for issues
> found by findbugs.
>
> Mostly they're just making inner classes static,
> making a few members of Serializable objects transient,
> removing unused code, and in one case adding type
> parameters to a Map.
>
I know next to nothing about serialization, but have you tested if
serialization works with these objects? The fact that ApplicationEvent
and DownloadEvent have no fields that are serialized seems rather odd.
I am wondering if we want anything serialized at all.
> Some non-trivial changes are:
> + * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> + Remove unused import.
> + (getNativeDir): Improve random int computation.
> + (CodeBaseClassLoader): Make it a static class.
> + * netx/net/sourceforge/jnlp/JNLPFile.java
> + (JNLPFile): Improve random positive int computation.
>
> I replaced new Random().nextInt() with Math.random()*max_int
> and (Math.random()*2-1)*max_int. The latter was to avoid
> creating the needless Random object, which doesn't produce
> good random numbers anyway. The former was because we used
> to have abs(new Random().nextInt()), so supposedly we were
> trying to get a random positive int. This doesn't work because
> abs is not guaranteed to return a positive number, and if we
> ever tried to parse the string we were creating we could have
> been in trouble. It might have been better to keep a static
> Random() object, but for so few uses, it didn't seem worth it
> to add a member.
>
Sounds good.
> + * netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> + (lock): Initialize to Object() instead of Integer(0).
>
> Not sure why we were creating an Integer(0) for a lock. An Object
> should be enough. Also, I made it final, because most of the time
> it does not make sense to have a lock whose only reference can change.
>
Makes sense.
> + * netx/net/sourceforge/jnlp/services/ServiceUtil.java
> + (checkAccess): Replace new Boolean with Boolean.valueOf.
>
> We weren't comparing the Boolean using ==, so getting a cached
> object should be fine.
>
Ok.
> This patch contains changes that I thought would be nice to have
> but would trivially not break anything (I built and tested it and
> it works). However, they only solve about 30% of findbugs' complaints.
> Most of the rest are false positives, except perhaps
> 6 of "Inefficient use of keySet iterator instead of entrySet iterator",
I suppose that's purely a performance change. Fixing this should be ok.
If you are actually looking to fix performance bugs, you might want to
profile first.
> 8 of "Maps and sets of URLs can be performance hogs",
I am not sure how easy this is to fix. We need some way to refer to
valid URLs, but without the performance issues. The obvious candidate,
URI, accepts some input (such as socket:// URIs) as valid which URL will
reject.
> 3 of "Method concatenates strings using + in a loop", and
This should be rather trivial, right? If you want to, go ahead and fix.
> 10 of "Method ignores exceptional return value" on delete() and mkdirs()
> calls.
>
Interesting. These might be real bugs hiding. It would certainly be nice
to fix them.
> Should these be handled (in particular the last one, since the
> other three are just performance related)?
>
The last one looks like it could result in problems, but I am not so
much concerned about the others.
> diff -r 67ae546b6605 ChangeLog
> --- a/ChangeLog Tue Mar 08 15:02:45 2011 -0500
> +++ b/ChangeLog Wed Mar 09 09:44:57 2011 -0500
> @@ -1,3 +1,45 @@
> +2011-03-09 Denis Lila<dlila at redhat.com>
> +
> + * netx/net/sourceforge/jnlp/Parser.java
> + (getJAR): Remove unused variable.
> + * netx/net/sourceforge/jnlp/cache/Resource.java
> + (connection): Remove unused member.
> + * netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> + (lock): Initialize to Object() instead of Integer(0). Also,
> + make final.
> + * netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> + (SettingsPanel): Make static class.
> + * netx/net/sourceforge/jnlp/event/ApplicationEvent.java
> + (application): Make member transient.
> + * netx/net/sourceforge/jnlp/event/DownloadEvent.java
> + (tracker, resource): Make members transient.
> + * netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
> + (appletInstance): Remove unused member.
> + (parameters): Add parameters to its type (a map).
> + * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> + Remove unused import.
> + (getNativeDir): Improve random int computation.
> + (CodeBaseClassLoader): Make it a static class.
> + * netx/net/sourceforge/jnlp/JNLPFile.java
> + (JNLPFile): Improve random positive int computation.
> + * netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> + (activeApplication): Remove unused member.
> + (checkExit): Remove dead code resulting from activeApplication
> + always being null.
> + * netx/net/sourceforge/jnlp/security/NotAllSignedWarningPane.java
> + Remove unused import.
> + (addComponents): Remove unused variable.
> + * netx/net/sourceforge/jnlp/security/SecurityDialogPanel.java
> + (SetValueHandler): Make it a static class.
> + * netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java
> + (CertificateType): Make it a static class.
> + * netx/net/sourceforge/jnlp/services/ServiceUtil.java
> + (checkAccess): Replace new Boolean with Boolean.valueOf.
> + * netx/net/sourceforge/jnlp/tools/JarSigner.java
> + (storeHash): Remove unused member.
> + * netx/net/sourceforge/jnlp/util/XDesktopEntry.java
> + (getContentsAsReader): Remove unused variable pathToJavaws.
> +
> 2011-03-08 Denis Lila<dlila at redhat.com>
>
> * netx/net/sourceforge/jnlp/runtime/RhinoBasedPacEvaluator.java
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/JNLPFile.java
> --- a/netx/net/sourceforge/jnlp/JNLPFile.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/JNLPFile.java Wed Mar 09 09:44:57 2011 -0500
> @@ -188,7 +188,7 @@
> this.fileLocation = location;
>
> this.uniqueKey = Calendar.getInstance().getTimeInMillis() + "-" +
> - Math.abs(((new java.util.Random()).nextInt())) + "-" +
> + ((int)(Math.random()*Integer.MAX_VALUE)) + "-" +
> location;
>
> if (JNLPRuntime.isDebug())
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/Parser.java
> --- a/netx/net/sourceforge/jnlp/Parser.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/Parser.java Wed Mar 09 09:44:57 2011 -0500
> @@ -336,7 +336,6 @@
> String part = getAttribute(node, "part", null);
> boolean main = "true".equals(getAttribute(node, "main", "false"));
> boolean lazy = "lazy".equals(getAttribute(node, "download", "eager"));
> - int size = Integer.parseInt(getAttribute(node, "size", "0"));
>
> if (nativeJar&& main)
> if (strict)
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/cache/Resource.java
> --- a/netx/net/sourceforge/jnlp/cache/Resource.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/cache/Resource.java Wed Mar 09 09:44:57 2011 -0500
> @@ -79,9 +79,6 @@
> /** the version downloaded from server */
> Version downloadVersion;
>
> - /** connection to resource */
> - URLConnection connection;
> -
> /** amount in bytes transferred */
> long transferred = 0;
>
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Wed Mar 09 09:44:57 2011 -0500
> @@ -94,7 +94,7 @@
> // lock, prefetch, this.resources, each resource, listeners
>
> /** notified on initialization or download of a resource */
> - private static Object lock = new Integer(0); // used to lock static structures
> + private static final Object lock = new Object(); // used to lock static structures
>
> // shortcuts
> private static final int UNINITIALIZED = Resource.UNINITIALIZED;
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java Wed Mar 09 09:44:57 2011 -0500
> @@ -71,7 +71,7 @@
> * @author @author Andrew Su (asu at redhat.com,andrew.su at utoronto.ca)
> *
> */
> - private class SettingsPanel {
> + private static class SettingsPanel {
> final String value;
> final JPanel panel;
>
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/event/ApplicationEvent.java
> --- a/netx/net/sourceforge/jnlp/event/ApplicationEvent.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/event/ApplicationEvent.java Wed Mar 09 09:44:57 2011 -0500
> @@ -29,7 +29,7 @@
> public class ApplicationEvent extends EventObject {
>
> /** the application instance */
> - private ApplicationInstance application;
> + transient private ApplicationInstance application;
>
> /**
> * Creates a launch event for the specified application
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/event/DownloadEvent.java
> --- a/netx/net/sourceforge/jnlp/event/DownloadEvent.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/event/DownloadEvent.java Wed Mar 09 09:44:57 2011 -0500
> @@ -31,10 +31,10 @@
> public class DownloadEvent extends EventObject {
>
> /** the tracker */
> - private ResourceTracker tracker;
> + transient private ResourceTracker tracker;
>
> /** the resource */
> - private Resource resource;
> + transient private Resource resource;
>
> /**
> * Creates a launch event for the specified application
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
> --- a/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java Wed Mar 09 09:44:57 2011 -0500
> @@ -43,14 +43,11 @@
> /** the JNLP file */
> private JNLPFile file;
>
> - /** the applet instance */
> - private AppletInstance appletInstance;
> -
> /** the applet */
> private Applet applet;
>
> /** the parameters */
> - private Map parameters;
> + private Map<String, String> parameters;
>
> /** the applet container */
> private Container cont;
> @@ -70,7 +67,6 @@
> */
> public AppletEnvironment(JNLPFile file, final AppletInstance appletInstance, Container cont) {
> this.file = file;
> - this.appletInstance = appletInstance;
> this.applet = appletInstance.getApplet();
>
> parameters = file.getApplet().getParameters();
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Wed Mar 09 09:44:57 2011 -0500
> @@ -39,7 +39,6 @@
> import java.util.LinkedList;
> import java.util.List;
> import java.util.Map;
> -import java.util.Random;
> import java.util.TreeSet;
> import java.util.Vector;
> import java.util.jar.JarEntry;
> @@ -853,10 +852,10 @@
> * calls.
> */
> protected File getNativeDir() {
> + final int rand = (int)((Math.random()*2 - 1) * Integer.MAX_VALUE);
> nativeDir = new File(System.getProperty("java.io.tmpdir")
> + File.separator + "netx-native-"
> - + (new Random().nextInt()& 0xFFFF));
> -
> + + (rand& 0xFFFF));
> File parent = nativeDir.getParentFile();
> if (!parent.isDirectory()&& !parent.mkdirs()) {
> return null;
> @@ -1328,7 +1327,7 @@
> * Helper class to expose protected URLClassLoader methods.
> */
>
> - public class CodeBaseClassLoader extends URLClassLoader {
> + public static class CodeBaseClassLoader extends URLClassLoader {
>
> JNLPClassLoader parentJNLPClassLoader;
>
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java Wed Mar 09 09:44:57 2011 -0500
> @@ -98,9 +98,6 @@
> private WeakList<ApplicationInstance> weakApplications =
> new WeakList<ApplicationInstance>();
>
> - /** weak reference to most app who's windows was most recently activated */
> - private WeakReference activeApplication = null;
> -
> /** Sets whether or not exit is allowed (in the context of the plugin, this is always false) */
> private boolean exitAllowed = true;
>
> @@ -449,13 +446,7 @@
> // but when they really call, stop only the app instead of the JVM
> ApplicationInstance app = getApplication(stack, 0);
> if (app == null) {
> - // should check caller to make sure it is JFrame.close or
> - // other known System.exit call
> - if (activeApplication != null)
> - app = (ApplicationInstance) activeApplication.get();
> -
> - if (app == null)
> - throw new SecurityException(R("RExitNoApp"));
> + throw new SecurityException(R("RExitNoApp"));
> }
>
> app.destroy();
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/security/NotAllSignedWarningPane.java
> --- a/netx/net/sourceforge/jnlp/security/NotAllSignedWarningPane.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/security/NotAllSignedWarningPane.java Wed Mar 09 09:44:57 2011 -0500
> @@ -53,8 +53,6 @@
> import javax.swing.JPanel;
> import javax.swing.SwingConstants;
>
> -import net.sourceforge.jnlp.JNLPFile;
> -
> public class NotAllSignedWarningPane extends SecurityDialogPanel {
>
> public NotAllSignedWarningPane(SecurityDialog x) {
> @@ -66,7 +64,6 @@
> * Creates the actual GUI components, and adds it to this panel
> */
> private void addComponents() {
> - JNLPFile file = parent.getFile();
>
> String topLabelText = R("SNotAllSignedSummary");
> String infoLabelText = R("SNotAllSignedDetail");
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/security/SecurityDialogPanel.java
> --- a/netx/net/sourceforge/jnlp/security/SecurityDialogPanel.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/security/SecurityDialogPanel.java Wed Mar 09 09:44:57 2011 -0500
> @@ -100,7 +100,7 @@
> * Creates a handler that sets a dialog's value and then disposes it when activated
> *
> */
> - private class SetValueHandler implements ActionListener {
> + private static class SetValueHandler implements ActionListener {
>
> Integer buttonIndex;
> SecurityDialog dialog;
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java
> --- a/netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java Wed Mar 09 09:44:57 2011 -0500
> @@ -299,7 +299,7 @@
> }
>
> /** Allows storing KeyStores.Types in a JComponent */
> - private class CertificateType {
> + private static class CertificateType {
> private final KeyStores.Type type;
>
> public CertificateType(KeyStores.Type type) {
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/services/ServiceUtil.java
> --- a/netx/net/sourceforge/jnlp/services/ServiceUtil.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/services/ServiceUtil.java Wed Mar 09 09:44:57 2011 -0500
> @@ -282,7 +282,7 @@
> public Boolean run() {
> boolean b = SecurityDialogs.showAccessWarningDialog(tmpType,
> tmpApp.getJNLPFile(), tmpExtras);
> - return new Boolean(b);
> + return Boolean.valueOf(b);
> }
> });
>
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/tools/JarSigner.java
> --- a/netx/net/sourceforge/jnlp/tools/JarSigner.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/tools/JarSigner.java Wed Mar 09 09:44:57 2011 -0500
> @@ -425,9 +425,6 @@
> details.add(detail);
> }
>
> - Hashtable<Certificate, String> storeHash =
> - new Hashtable<Certificate, String>();
> -
> /**
> * signature-related files include:
> * . META-INF/MANIFEST.MF
> diff -r 67ae546b6605 netx/net/sourceforge/jnlp/util/XDesktopEntry.java
> --- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Tue Mar 08 15:02:45 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Wed Mar 09 09:44:57 2011 -0500
> @@ -72,8 +72,6 @@
> */
> public Reader getContentsAsReader() {
>
> - String pathToJavaws = System.getProperty("java.home") + File.separator + "bin"
> - + File.separator + "javaws";
> String cacheDir = JNLPRuntime.getConfiguration()
> .getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
> File cacheFile = CacheUtil.urlToPath(file.getSourceLocation(), cacheDir);
The patch looks fine to me.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list