/hg/icedtea-web: Improve PolicyTool launch method in PolicyPanel

[Andrew Azores]

On 01/27/2014 09:39 AM, Jiri Vanek wrote:
> ...
>>> +     * @param filePath a {@link String} representing the path of 
>>> the file to attempt to open
>>> +     * @throws Exception if any sort of exception occurs during 
>>> reflective launch of policytool
>>> +     */
>>> +    private static void reflectivePolicyToolLaunch(final String 
>>> filePath) throws Exception {
>>> +        Class<?> policyTool = 
>>> Class.forName("sun.security.tools.policytool.PolicyTool");
>>
>> What about JRE 6? You could catch a ClassNotFoundException here and 
>> then try to get
>> sun.security.tools.PolicyTool. And, if that fails, well then... Blow 
>> up! :-D
>
> Well - why yes? The jdk6 is dead. We are dealing with slowly, but its 
> true. The command exec is what matters. The second one is fallback.
>
> If you have non-policytool command, non jdk7 system. Please go on and 
> fix. Otherwise I do not believe it is worthy. And if we  will nit pick 
> -what about gnu classpath ? ;)
>
> Maybe some  better error message then
>      JOptionPane.showMessageDialog(frame, message, R("Error"), 
> JOptionPane.ERROR_MESSAGE); ?
>
> except that I agree with all what Jacob  pointed out.

What better error message are you looking for? If both launch methods 
fail, the error message says something like "could not find system 
policy editor. Check that policytool is in your PATH".

>>
>>> +        Class<?>[] signature = new Class<?>[] { String[].class };
>>
>> Redundant. No need to create a new instance here at run-time.
>>
>>> +        Method main = policyTool.getDeclaredMethod("main", signature);
>>
>> Just substitute "signature" with "String[].class".
>>
>>> +        Object args = new String[] { "-file", filePath };
>>
>> Why is "args" of type Object? String[] should be fine and 
>> Method.invoke() won't complain because
>> String[] inherits from Object. ;-)
>>
> Especially this.. My overlook.

Method#invoke has signature(Object, Object...) - the first being the 
object the method is to be invoked on, and the second being a varargs 
list for the method call. Yes, String[] inherits from Object, and it 
also confuses the varargs invocation. Hopefully this notation will make 
sense, but what I am doing here results in this method call being invoked:

PolicyTool.main({"-file", filePath})

and what you are suggesting results in this:

PolicyTool.main("-file", filePath)

These are not the same and in fact the second call will fail because 
there is no PolicyTool.main(String, String) method, so you get 
IllegalArgumentException: wrong number of arguments.

The "signature" variable is indeed not necessary but I'm really 
surprised that we're getting to the level of nitpicking on inlining 
variables here. Does this matter so much that it's worth patching?

>
> And yes - the  invoke later around the  joptionpane is redundant.
>
> Thanx jacob!
>
> >
>
>
>
>
> J.
>

Should we not be ensuring that we only invoke 
JOptionPane.showMessageDialog from the EDT? We have two different 
convenience methods that make this call, and *usually* these methods are 
all called from the main thread, but there is an occasion where we call 
showCouldNotOpenFileDialog from policyToolLaunchHelper, and this time 
it's occurring in a new thread. So we can either wrap this one 
particular call in an invokeLater, or just be completely sure that these 
two convenience methods are always used safely and put the invokeLater 
directly within them. In most cases, yes, it ends up being redundant, 
but it also means in every case it's done safely, doesn't it?

Here's a small StackOverflow discussion I found while double checking 
myself on this.

https://stackoverflow.com/questions/13863713/which-thread-to-launch-joptionpane

Thanks,

-- 
Andrew A