[RFC][icedtea-web]: Fix for PR1040, PR1041, PR1042 w/ reproducers
Adam Domurad
adomurad at redhat.com
Thu Jun 21 12:44:21 PDT 2012
Thanks for looking into this issue, and providing reproducers.
Looks overall good functionality-wise, but the way this is currently
implemented strikes me as a little intrusive, as it adds a field to a
class (JNLPFile) that is really only used in another class
(JNLPClassLoader). A less intrusive implementation might be to keep
track of the main class in each JNLPClassLoader, as the main class is
checked on the construction of any JNLPClassLoader.
Individual comments inline.
On Mon, 2012-06-18 at 16:52 -0400, Saad Mohammad wrote:
> The attached patch resolves PR1040, PR1041, PR1042:
> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1040
> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1041
> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1042
>
> diff --git a/netx/net/sourceforge/jnlp/JNLPFile.java b/netx/net/sourceforge/jnlp/JNLPFile.java
> --- a/netx/net/sourceforge/jnlp/JNLPFile.java
> +++ b/netx/net/sourceforge/jnlp/JNLPFile.java
> @@ -111,6 +111,9 @@
> /** A signed JNLP file is missing from the main jar */
> private boolean missingSignedJNLP = false;
>
> + /** Name of the main class**/
> + private String mainClassName;
> +
> /** JNLP file contains special properties */
> private boolean containsSpecialProperties = false;
>
> @@ -743,4 +746,13 @@
> missingSignedJNLP = true;
> }
>
> + public String getMainClassName() {
> + return mainClassName;
> + }
> +
> + public void setMainClassName(final String mainClassName) {
> + if(mainClassName != null && mainClassName != "")
> + this.mainClassName = mainClassName;
> + }
final String here is not necessary. Doesn't hurt, and I don't feel too
strongly for/against this specification, but the rest of the code does
not use final parameters in this way.
> +
> }
> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> @@ -377,18 +377,32 @@
> * @param location the file's location
> * @param version the file's version
> * @param policy the update policy to use when downloading resources
> + * @param mainName Overrides the main class name of the application
> */
> - public static JNLPClassLoader getInstance(URL location, String uniqueKey, Version version, UpdatePolicy policy)
> + public static JNLPClassLoader getInstance(URL location, String uniqueKey, Version version, UpdatePolicy policy, String mainName)
> throws IOException, ParseException, LaunchException {
> JNLPClassLoader loader = urlToLoader.get(uniqueKey);
>
> - if (loader == null || !location.equals(loader.getJNLPFile().getFileLocation()))
> - loader = getInstance(new JNLPFile(location, uniqueKey, version, false, policy), policy);
> + if (loader == null || !location.equals(loader.getJNLPFile().getFileLocation())) {
> + JNLPFile tempFile = new JNLPFile(location, uniqueKey, version, false, policy);
A naming note - I would not name this tempFile as it implies, at least
to me, that it does not outlive the function, which is does, since the
JNLPClassLoader holds onto it.
> +
> + if (mainName != null && mainName != "")
> + tempFile.setMainClassName(mainName);
> +
> + loader = getInstance(tempFile, policy);
> + }
>
> return loader;
> }
>
> /**
> + * Returns true if this loader has the main jar
> + */
> + public boolean hasMainJar() {
> + return this.foundMainJar;
> + }
> +
> + /**
> * Load the extensions specified in the JNLP file.
> */
> void initializeExtensions() {
> @@ -402,7 +416,23 @@
> for (int i = 0; i < ext.length; i++) {
> try {
> String uniqueKey = this.getJNLPFile().getUniqueKey();
> - JNLPClassLoader loader = getInstance(ext[i].getLocation(), uniqueKey, ext[i].getVersion(), updatePolicy);
> + String mainName = null;
> +
> + if (file.getMainClassName() != null)
> + mainName = file.getMainClassName();
> + else {
> + Object obj = file.getLaunchInfo();
> +
> + if (obj instanceof ApplicationDesc) {
> + ApplicationDesc ad = (ApplicationDesc) file.getLaunchInfo();
> + mainName = ad.getMainClass();
> + } else if (obj instanceof AppletDesc) {
> + AppletDesc ad = (AppletDesc) file.getLaunchInfo();
> + mainName = ad.getMainClass();
> + }
> + }
> +
> + JNLPClassLoader loader = getInstance(ext[i].getLocation(), uniqueKey, ext[i].getVersion(), updatePolicy, mainName);
> loaderList.add(loader);
> } catch (Exception ex) {
> ex.printStackTrace();
> @@ -441,8 +471,24 @@
> */
> void initializeResources() throws LaunchException {
> JARDesc jars[] = resources.getJARs();
> - if (jars == null || jars.length == 0)
> +
> + if (jars == null || jars.length == 0) {
> +
> + for (int i = 1; i < loaders.length; i++) {
> + if (loaders[i].getSigning() == false)
> + break;
> + if (loaders[i].getSigning() == true && i == (loaders.length - 1)) {
> + this.signing = true;
== false && == true aren't necessary here. Simply
!loaders[i].getSigning(), loaders[i].getSigning() are sufficient. Their
use as a condition implies they are booleans already.
It took me a bit to parse what this was doing.
This loop would be IMO simpler read as:
boolean allSigning = true;
for ( ... ) {
if (!loaders[i].getSigning())
allSigning = false;
}
if (allSigning) {
this.signing = true;
}
> + }
> + }
> +
> + //Check if main jar is found within extensions
> + if (!foundMainJar)
> + for (int i = 1; i < loaders.length && !foundMainJar; i++)
> + foundMainJar = loaders[i].hasMainJar();
Using curly brackets around the for loop is more legible here in my
opinion. Also, its not strictly necessary, as the for loop has the
condition !foundMainJar, and thus won't run at all if foundMainJar is
true.
> +
> return;
> + }
> /*
> if (jars == null || jars.length == 0) {
> throw new LaunchException(null, null, R("LSFatal"),
> @@ -504,6 +550,12 @@
> while (!foundMainJar && available != null && available.size() != 0)
> addNextResource();
>
> + // If the jar with main class was not found, check extension
> + // jnlp's resources
> + if (!foundMainJar)
> + for (int i = 1; i < loaders.length && !foundMainJar; i++)
> + foundMainJar = loaders[i].hasMainJar();
If functionality that encapsulates a loop really must be duplicated, I
would make a private method. Otherwise changes to one copy may miss its
duplication somewhere else.
> +
> // If jar with main class was not found and there are no more
> // available jars, throw a LaunchException
> if (file.getLaunchInfo() instanceof AppletDesc ||
> @@ -589,18 +641,24 @@
> * @throws LaunchException Thrown if the signed JNLP file, within the main jar, fails to be verified or does not match
> */
> private void checkForMain(List<JARDesc> jars) throws LaunchException {
> -
> - Object obj = file.getLaunchInfo();
> String mainClass;
>
> - if (obj instanceof ApplicationDesc) {
> - ApplicationDesc ad = (ApplicationDesc) file.getLaunchInfo();
> - mainClass = ad.getMainClass();
> - } else if (obj instanceof AppletDesc) {
> - AppletDesc ad = (AppletDesc) file.getLaunchInfo();
> - mainClass = ad.getMainClass();
> - } else
> - return;
> + if (file.getMainClassName() != null && file.getMainClassName() != "")
> + mainClass = file.getMainClassName();
> + else {
> + Object obj = file.getLaunchInfo();
> +
> + if (obj instanceof ApplicationDesc) {
> + ApplicationDesc ad = (ApplicationDesc) file.getLaunchInfo();
> + mainClass = ad.getMainClass();
> + file.setMainClassName(mainClass);
> + } else if (obj instanceof AppletDesc) {
> + AppletDesc ad = (AppletDesc) file.getLaunchInfo();
> + mainClass = ad.getMainClass();
> + file.setMainClassName(mainClass);
> + } else
> + return;
> + }
>
> // The main class may be specified in the manifest
More information about the distro-pkg-dev
mailing list