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

[Jacob Wisor]

On 01/24/2014 04:48 PM, aazores at icedtea.classpath.org wrote:
> [...]
> -import sun.security.tools.policytool.PolicyTool;
>
>   public class PolicyPanel extends NamedBorderPanel {

Please add at least one sentence documenting this class' purpose. Thank you.

> [...]
>       /**
>        * Launch the policytool for a specified file path
> -     * @param filePath the policy file path to be opened with policytool
> +     * @param frame a {@link JFrame} to act as parent to warning dialogs which may appear
> +     * @param filePath a {@link String} representing the path to the file to be opened
>        */
>       private static void launchPolicyTool(final JFrame frame, final String filePath) {
>           try {
>               final File policyFile = new File(filePath).getCanonicalFile();
>               OpenFileResult result = canOpenPolicyFile(policyFile);

Please consider declaring "result" final.

>               if (result == OpenFileResult.SUCCESS) {
> -                PolicyTool.main(new String[] { "-file", policyFile.getPath() });
> +                policyToolLaunchHelper(frame, filePath);
>               } else if (result == OpenFileResult.CANT_WRITE) {
>                   showReadOnlyDialog(frame);
> -                PolicyTool.main(new String[] { "-file", policyFile.getPath() });
> +                policyToolLaunchHelper(frame, filePath);
>               } else {
>                   showCouldNotOpenFileDialog(frame, policyFile.getPath(), result);
>               }
> [...]
>       /**
> +     * This executes a new process for policytool using ProcessBuilder, with the new process'
> +     * working directory set to the user's home directory. policytool then attempts to
> +     * open the provided policy file path, if policytool can be run. ProcessBuilder does
> +     * some verification to ensure that the built command can be executed - if not, it
> +     * throws an IOException. In this event, we try our reflective fallback launch.
> +     * We do this in a new {@link Thread} to ensure that the fallback launch does not
> +     * block the AWT thread, and neither does ProcessBuilder#start() in case it happens
> +     * to be synchronous on the current system.
> +     * @param frame a {@link JFrame} to act as parent to warning dialogs which may appear
> +     * @param filePath a {@link String} representing the path to the file to be opened
> +     */
> +    private static void policyToolLaunchHelper(final JFrame frame, final String filePath) {
> +        new Thread(new Runnable() {
> +            @Override
> +            public void run() {
> +                ProcessBuilder pb = new ProcessBuilder("policytool", "-file", filePath)
> +                        .directory(new File(System.getProperty("user.home")));

Please consider declaring "pb" final.

> +                try {
> +                    pb.start();
> +                } catch (IOException ioe) {
> +                    OutputController.getLogger().log(ioe);
> +                    try {
> +                        reflectivePolicyToolLaunch(filePath);
> +                    } catch (Exception e) {
> +                        OutputController.getLogger().log(e);
> +                        showCouldNotOpenFileDialog(frame, filePath, R("CPPolicyEditorNotFound"));

Hmm, this looks interesting. The dialog will be modal to IcedTea-Web's JFrame 
from a different than the AWT thread. I hope this doesn't break. :-D

> +                    }
> +                }
> +            }
> +        }).start();
> +    }
> +
> +    /**
> +     * This is used as a fallback in case launching the policytool by executing a new process
> +     * fails. This probably happens because we are running on a system where the policytool
> +     * executable is not on the PATH, or because we are running on a non-POSIX compliant system.
> +     * We do this reflectively to avoid needing to add PolicyTool as build dependency simply for
> +     * this small edge case.
> +     * @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

> +        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. ;-)

> +        main.invoke(null, args);
> +    }
> +
> [...]
>       /**
>        * Show a dialog informing the user that the policy file could not be opened
> -     * @param frame the parent frame for this dialog
> -     * @param filePath the path to the file we tried to open
> -     * @param message the specific reason the file could not be opened
> +     * @param frame a {@link JFrame} to act as parent to this dialog
> +     * @param filePath a {@link String} representing the path to the file we failed to open
> +     * @param message a {@link String} giving the specific reason the file could not be opened
>        */
>       private static void showCouldNotOpenFileDialog(final JFrame frame, final String filePath, final String message) {
>           OutputController.getLogger().log(OutputController.Level.ERROR_ALL, "Could not open user JNLP policy");
> -        JOptionPane.showMessageDialog(frame, message, R("Error"), JOptionPane.ERROR_MESSAGE);
> +        SwingUtilities.invokeLater(new Runnable() {
> +            @Override
> +            public void run() {
> +                JOptionPane.showMessageDialog(frame, message, R("Error"), JOptionPane.ERROR_MESSAGE);

??? Why do you queue up a modal dialog on the AWT thread? I mean, it's not wrong 
and IcedTea-Web's JFrame will surely be happy to process all events in queue 
before its thread gets blocked by that modal dialog anyway. So, it is not really 
necessary here. Modal dialogs may or perhaps should be executed on the AWT 
thread because they are supposed to block all other UI components they refer to.

> +            }
> +        });
>       }
>
>       /**
>        * Show a dialog informing the user that the policy file is currently read-only
> -     * @param frame the parent frame for this dialog
> +     * @param frame a {@link JFrame} to act as parent to this dialog
>        */
>       private static void showReadOnlyDialog(final JFrame frame) {
>           OutputController.getLogger().log(OutputController.Level.WARNING_ALL, "Opening user JNLP policy read-only");
> -        JOptionPane.showMessageDialog(frame, R("RFileReadOnly"), R("Warning"), JOptionPane.WARNING_MESSAGE);
> +        SwingUtilities.invokeLater(new Runnable() {
> +            @Override
> +            public void run() {
> +                JOptionPane.showMessageDialog(frame, R("RFileReadOnly"), R("Warning"), JOptionPane.WARNING_MESSAGE);

Same goes here.

> +            }
> +        });
>       }
> [...]

Nice, untiring zeal. Keep it up! :-)

Jacob