[RFC][icedtea-web]: Fix for PR1040, PR1041, PR1042 w/ reproducers
Adam Domurad
adomurad at redhat.com
Wed Jun 27 13:36:07 PDT 2012
Will look at the reproducers shortly. Sorry to delay this further but I
still have 2 comments. Otherwise, looks good!
> 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
> @@ -183,6 +183,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
> */
> @@ -193,9 +196,25 @@
> *
> * @param file the JNLP file
> */
> - protected JNLPClassLoader(JNLPFile file, UpdatePolicy policy)
> throws LaunchException {
> + private JNLPClassLoader(JNLPFile file, UpdatePolicy policy)
> throws LaunchException {
> super(new URL[0], JNLPClassLoader.class.getClassLoader());
>
> + initializeJNLPClassLoader(file, policy, null);
1) You have changed the behaviour of JNLPClassLoader here by making the
constructor private instead of protected, prohibiting subclassing.
2) You did not need to introduce a method 'initializeJNLPClassLoader'.
What you do in this scenario:
- Have JNLPClassLoader(JNLPFile file, UpdatePolicy policy, String
mainName) with the full implementation
- call 'this(file, policy, null);' in JNLPClassLoader(JNLPFile file,
UpdatePolicy policy). This will call the other constructor without the
need for a separate initalize procedure.
> + }
> +
> + private JNLPClassLoader(JNLPFile file, UpdatePolicy policy,
> String mainName) throws LaunchException {
> + super(new URL[0], JNLPClassLoader.class.getClassLoader());
> +
> + initializeJNLPClassLoader(file, policy, mainName);
> + }
> +
> + /**
> + * Initialize the JNLPClassLoader
> + *
> + * @param file the JNLP file
> + */
> + private void initializeJNLPClassLoader(JNLPFile file,
> UpdatePolicy policy, String mainName) throws LaunchException {
> +
> if (JNLPRuntime.isDebug())
> System.out.println("New classloader: " +
> file.getFileLocation());
>
> @@ -203,6 +222,9 @@
> this.updatePolicy = policy;
> this.resources = file.getResources();
>
> + if (mainName != null)
> + this.mainClass = mainName;
> +
> // initialize extensions
> initializeExtensions();
>
> @@ -309,6 +331,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();
> @@ -325,7 +358,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.
> @@ -343,14 +376,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);
> @@ -380,13 +413,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;
> }
> @@ -405,7 +442,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("")) {
If I'm not mistaken, you can drop the mainClass.equals("") here.
> + 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();
> @@ -444,8 +494,25 @@
> */
> 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
> + foundMainJar = foundMainJar || hasMainInExtensions();
> +
> return;
> + }
> /*
> if (jars == null || jars.length == 0) {
> throw new LaunchException(null, null, R("LSFatal"),
> @@ -507,6 +574,10 @@
> while (!foundMainJar && available != null &&
> available.size() != 0)
> addNextResource();
>
> + // If the jar with main class was not found, check
> extension
> + // jnlp's resources
> + foundMainJar = foundMainJar || hasMainInExtensions();
> +
> // If jar with main class was not found and there are
> no more
> // available jars, throw a LaunchException
> if (file.getLaunchInfo() instanceof AppletDesc ||
> @@ -593,17 +664,18 @@
> */
> private void checkForMain(List<JARDesc> jars) throws
> LaunchException {
>
> - Object obj = file.getLaunchInfo();
> - String mainClass;
> + if (mainClass == null) {
> + 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
>
> @@ -698,6 +770,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