[RFC][icedtea-web]: Fix for PR1040, PR1041, PR1042 w/ reproducers
Adam Domurad
adomurad at redhat.com
Tue Jun 26 08:55:53 PDT 2012
Thanks for the update. I think the new changeset is clearer in intent.
JNLPClassLoader is growing quite large, but that's neither here nor
there. It might be good to break down its functionality sometime in the
future (I have two patches waiting review, for example, that add to its
length somewhat).
Some minor comments inline.
>
On Fri, 2012-06-22 at 17:51 -0400, Saad Mohammad wrote:
> Oops, my reply fell of this thread :(
>
> Here it is:
> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2012-June/019186.html
>
> Sorry!
>
> 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
> @@ -180,6 +180,9 @@
> * */
> private boolean foundMainJar= false;
>
> + /** Name of the application's main class */
> + private String mainClass = null;
> +
> /**
> * Variable to track how many times this loader is in use
> */
> @@ -190,7 +193,7 @@
> *
> * @param file the JNLP file
> */
> - protected JNLPClassLoader(JNLPFile file, UpdatePolicy policy) throws LaunchException {
> + protected JNLPClassLoader(JNLPFile file, UpdatePolicy policy, String... mainName) throws LaunchException {
You should not use ... to represent a 0-or-1 relationship.
> super(new URL[0], JNLPClassLoader.class.getClassLoader());
>
> if (JNLPRuntime.isDebug())
> @@ -200,6 +203,9 @@
> this.updatePolicy = policy;
> this.resources = file.getResources();
>
> + if (mainName != null && mainName.length > 0)
> + this.mainClass = mainName[0];
> +
> // initialize extensions
> initializeExtensions();
>
> @@ -306,6 +312,17 @@
> * @param policy the update policy to use when downloading resources
> */
> public static JNLPClassLoader getInstance(JNLPFile file, UpdatePolicy policy) throws LaunchException {
> + return getInstance(file, policy, null);
> + }
> +
> + /**
> + * Returns a JNLP classloader for the specified JNLP file.
> + *
> + * @param file the file to load classes for
> + * @param policy the update policy to use when downloading resources
> + * @param mainName Overrides the main class name of the application
> + */
> + public static JNLPClassLoader getInstance(JNLPFile file, UpdatePolicy policy, String mainName) throws LaunchException {
> JNLPClassLoader baseLoader = null;
> JNLPClassLoader loader = null;
> String uniqueKey = file.getUniqueKey();
> @@ -322,7 +339,7 @@
> (file.isApplication() &&
> !baseLoader.getJNLPFile().getFileLocation().equals(file.getFileLocation()))) {
>
> - loader = new JNLPClassLoader(file, policy);
> + loader = new JNLPClassLoader(file, policy, mainName);
>
> // New loader init may have caused extentions to create a
> // loader for this unique key. Check.
> @@ -340,14 +357,14 @@
> // loader is now current + ext. But we also need to think of
> // the baseLoader
> if (baseLoader != null && baseLoader != loader) {
> - loader.merge(baseLoader);
> + loader.merge(baseLoader);
> }
>
> } else {
> // if key is same and locations match, this is the loader we want
> if (!file.isApplication()) {
> // If this is an applet, we do need to consider its loader
> - loader = new JNLPClassLoader(file, policy);
> + loader = new JNLPClassLoader(file, policy, mainName);
>
> if (baseLoader != null)
> baseLoader.merge(loader);
> @@ -377,13 +394,17 @@
> * @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 jnlpFile = new JNLPFile(location, uniqueKey, version, false, policy);
> +
> + loader = getInstance(jnlpFile, policy, mainName);
> + }
>
> return loader;
> }
> @@ -402,7 +423,20 @@
> 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);
> +
> + if (mainClass == null || mainClass.equals("")) {
> + Object obj = file.getLaunchInfo();
> +
> + 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();
> + }
> + }
> +
> + JNLPClassLoader loader = getInstance(ext[i].getLocation(), uniqueKey, ext[i].getVersion(), updatePolicy, mainClass);
> loaderList.add(loader);
> } catch (Exception ex) {
> ex.printStackTrace();
> @@ -441,8 +475,26 @@
> */
> void initializeResources() throws LaunchException {
> JARDesc jars[] = resources.getJARs();
> - if (jars == null || jars.length == 0)
> +
> + if (jars == null || jars.length == 0) {
> +
> + boolean allSigned = true;
> + for (int i = 1; i < loaders.length; i++) {
> + if (!loaders[i].getSigning()) {
> + allSigned = false;
> + break;
> + }
> + }
> +
> + if(allSigned)
> + signing = true;
> +
> + //Check if main jar is found within extensions
> + if (!foundMainJar && hasMainInExtensions())
> + foundMainJar = true;
Just a note: You can use simply
foundMainJar |= hasMainInExtensions();
If you want to keep the current behaviour of not always running
hasMainInExtensions, you can use:
foundMainJar = foundMainJar || hasMainInExtensions();
If you don't like either of those feel free to
leave it :) (just pointing out alternatives)
> +
> return;
> + }
> /*
> if (jars == null || jars.length == 0) {
> throw new LaunchException(null, null, R("LSFatal"),
> @@ -504,6 +556,11 @@
> while (!foundMainJar && available != null && available.size() != 0)
> addNextResource();
>
> + // If the jar with main class was not found, check extension
> + // jnlp's resources
> + if (!foundMainJar && hasMainInExtensions())
> + foundMainJar = true;
See above (but go with your preference :)
> +
> // If jar with main class was not found and there are no more
> // available jars, throw a LaunchException
> if (file.getLaunchInfo() instanceof AppletDesc ||
> @@ -590,17 +647,18 @@
> */
> private void checkForMain(List<JARDesc> jars) throws LaunchException {
>
> - Object obj = file.getLaunchInfo();
> - String mainClass;
> + if (mainClass == null || mainClass.equals("")){
This check is a bit ugly, will mainClass ever be "" ? If "" is already an established default, I
would suggest only using "" as the default value.
> + Object obj = file.getLaunchInfo();
>
> - 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 (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;
> + }
>
> // The main class may be specified in the manifest
>
> @@ -695,6 +753,26 @@
> }
>
> /**
> + * Returns true if this loader has the main jar
> + */
> + public boolean hasMainJar() {
> + return this.foundMainJar;
> + }
> +
> + /**
> + * Returns true if extension loaders have the main jar
> + */
> + private boolean hasMainInExtensions() {
> + boolean foundMain = false;
> +
> + for (int i = 1; i < loaders.length && !foundMain; i++) {
> + foundMain = loaders[i].hasMainJar();
> + }
> +
> + return foundMain;
> + }
> +
> + /**
> * Is called by checkForMain() to check if the jar file is signed and if it
> * contains a signed JNLP file.
> *
More information about the distro-pkg-dev
mailing list