Reviewer needed - fix for Re: Strange behaviour during javaws -about

[Omair Majid]

Hi,

I am attaching a patch that makes net.sourceforge.jnlp.runtime.Boot use 
the location of about.jnlp as determined at build time. There are a few 
other things that I will be adding to build.properties.

The rest of this email assumes this patch is applied before applying 
your patch, and where there are conflicts (such as the codebase of 
about.jnlp, this patch takes precedence).

On 02/24/2011 12:04 PM, Dr Andrew John Hughes wrote:
> On 17:43 Thu 24 Feb     , Jiri Vanek wrote:
>> On 02/24/2011 12:36 AM, Omair Majid wrote:
>>
>> So, this patch expects fixed about.jnlp to have code base of
>> file://$DEST_DIR/share
>>

The attached patch takes care of this (but nothing else). More changes 
(including some that you have in this patch are needed to make it work 
properly.

>> Then removes all-permissions, and lunched fixed window without them. The
>> window is not running in invoke later(which probably caused the hang)
>> but runs in javaws's own thread. Also points directly to correct jnlp
>> local file and runs offline.
>>
>
> Comments inline.
>

Likewise.

> Please separate any formatting changes to another patch, though I don't
> see why these are necessary at all.
>

Agreed. It just makes applying the patch and reviewing it more painful.

>> diff -r d7fee305bd4f -r e2f1a23ade09
>> extra/net/sourceforge/jnlp/about/Main.java
>> --- a/extra/net/sourceforge/jnlp/about/Main.java	Wed Feb 23 13:37:10
>> 2011 -0500
>> +++ b/extra/net/sourceforge/jnlp/about/Main.java	Thu Feb 24 17:33:23
>> 2011 +0100
>> @@ -41,19 +41,15 @@
>>    import java.awt.Dimension;
>>    import java.awt.Toolkit;
>>    import java.io.IOException;
>> -import java.net.URL;
>> +
>>
>>    import javax.swing.JFrame;
>>    import javax.swing.JPanel;
>>    import javax.swing.JTabbedPane;
>>    import javax.swing.UIManager;
>> -import javax.swing.event.HyperlinkEvent;
>> -import javax.swing.event.HyperlinkListener;
>>
>> -import net.sourceforge.jnlp.Launcher;
>> -import net.sourceforge.jnlp.runtime.JNLPRuntime;
>>
>> -public class Main extends JPanel implements HyperlinkListener {
>> +public class Main extends JPanel{
>>
>
> Why these changes?
>

 From what I can tell, these changes disable the hyperlinks from 
working. Access to net.sourceforge.jnlp package is required do make that 
functional. So if we dont want to grant all-permissions to 
about.jnlp/about.jar, then we need to remove all access to the 
net.sourceforge.jnlp package. The changes to this file do exactly that.

Unfortunately, that's not enough. The code in about.jar is also in 
net.sourceforge.jnlp package. So disallowing access to 
net.sourceforge.jnlp also breaks about.jar

 From a build where I applied my patch and then Jiri's changes:

$ ./javaws -about
[snip]
Caused by: java.security.AccessControlException: access denied 
(java.lang.RuntimePermission 
accessClassInPackage.net.sourceforge.jnlp.about)
[snip]
net.sourceforge.jnlp.runtime.JNLPClassLoader.loadClass(JNLPClassLoader.java:981)
	at net.sourceforge.jnlp.about.Main.main(Main.java:110)
	... 6 more

The only way to make this work without granting about.jar 
all-permissions might be to move it to another package.

>> @@ -112,24 +105,8 @@
>>        }
>>
>>    	public static void main(String[] args) {
>> -		javax.swing.SwingUtilities.invokeLater(new Runnable() {
>> -			public void run() {
>> -				createAndShowGUI();
>> -			}
>> -		});
>> -	}
>> +					createAndShowGUI();
>> +		}
>>

I dont agree with this change. createAndShowGUI() create a JFrame; 
running this code on the main thread is a violation of the Swing 
threading guidelines [1]. Not saying that there's no bug here (I haven't 
really looked) but the removal of invokeLater looks wrong.

>> diff -r d7fee305bd4f -r e2f1a23ade09
>> netx/net/sourceforge/jnlp/resources/about.jnlp
>> --- a/netx/net/sourceforge/jnlp/resources/about.jnlp	Wed Feb 23 13:37:10
>> 2011 -0500
>> +++ b/netx/net/sourceforge/jnlp/resources/about.jnlp	Thu Feb 24 17:33:23
>> 2011 +0100
>> @@ -1,5 +1,5 @@
>>    <?xml version="1.0" encoding="utf-8"?>
>> -<jnlp spec="1.0" href="about.jnlp"
>> codebase="http://icedtea.classpath.org/netx/">
>> +<jnlp spec="1.0" href="about.jnlp"
>> codebase="file:///home/jvanek/icedtea-web-image/share/icedtea-web/">
>>      <information>
>>        <title>About window for NetX</title>
>>        <vendor>NetX</vendor>

Well, if you expect this code to be fixed by someone else's (not posted) 
patch, please dont change it :) It makes patches conflict.

>> @@ -12,7 +12,7 @@
>>        <jar href="about.jar"/>
>>      </resources>
>>      <security>
>> -<all-permissions/>
>> +
>>      </security>
>>      <application-desc main-class="net.sourceforge.jnlp.about.Main">
>>      </application-desc>

Again, removing all-permissions and accessing the net.sourceforge.jnlp 
package is currently not possible :(

>> @@ -218,8 +223,16 @@
>>                cl = ClassLoader.getSystemClassLoader();
>>            }
>>            try {
>> -            return
>> cl.getResource("net/sourceforge/jnlp/resources/about.jnlp").toString();
>> +            //exctracts full path to about.jnlp
>
> Typo; extracts
>
>> +            String s =
>> cl.getResource("net/sourceforge/jnlp/runtime/Boot.class").toString();
>> +            s=s.substring(0,s.indexOf("!"));
>> +            s=s.substring(s.indexOf(":")+1);
>> +            s=s.substring(s.indexOf(":")+1);
>> +            s="file://"+s.replace("netx.jar","about.jnlp");
>> +            System.out.println(s);
>> +            return s;
>
> What does this do?
>

If I understand this properly, it uses the installed location of 
netx.jar to compute the location of about.jnlp. That is, if netx.jar is 
at /usr/local/share/icedtea-web/netx.jar, then it finds about.jnlp to be 
located at /usr/local/share/icedtea-web/about.jnlp.

The attached patch determines the paths at build time and puts them in a 
build.properties file.

Cheers,
Omair

[1] 
http://download.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: find-about-jnlp-at-build-time.patch
Type: text/x-patch
Size: 3645 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110224/a8d8363b/find-about-jnlp-at-build-time.patch