[rfc][icedtea-web] policytool in itweb-settings

[Jacob Wisor]

On 01/21/2014 12:25 AM, Jiri Vanek wrote:
> On 01/20/2014 09:11 PM, Andrew Azores wrote:
>> On 01/20/2014 02:50 PM, Andrew Azores wrote:
>>> On 01/20/2014 07:27 AM, Jiri Vanek wrote:
>>>> On 01/17/2014 10:08 PM, Andrew Azores wrote:
>>>>> (snip)
>>>>
>>>> [...]
>>>
>>> [...]
>>
>> [...]
>
> Only one *the* nit :)
>
>
>>       /**
>> +     * We do this so we can asynchronously monitor the exit status of the
>> policytool process.
>> +     * If it fails immediately and we need to fall back to using
>> reflectivePolicyToolLaunch,
>> +     * then the Thread spawned here is very short lived. However, if the
>> policytool process
>> +     * successfully launches, then it takes some unknown amount of time
>> before it closes
>> +     * again. We do not want to call waitFor() on the ptool process in the
>> main thread
>> +     * because this will cause the itweb-settings window to lock up.
>> +     * @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
>> +     * @throws IOException if an IOException occurs during the exec of the
>> policytool process
>> +     */
>> +    private static void policyToolLaunchHelper(final JFrame frame, final
>> String filePath) {
>> +        final String[] launchCommand = new String[] { "policytool", "-file",
>> filePath };
>> +        new Thread(new Runnable() {
>> +            @Override
>> +            public void run() {
>> +                try {
>> +                    final Process ptool =
>> Runtime.getRuntime().exec(launchCommand);
>> +                    if (ptool.waitFor() != 0) {

You do realize that Process.waitFor() may never return and thus block the 
current thread indefinitely? So, either some other thread has to interrupt it 
and kill that sub process (or better yet, this thread's InterruptedException 
handler). Or, the sub process needs to be polled by Process.exitValue() after 
some timeout and then perhaps be Process.destroy()'ed.

>> +                        reflectivePolicyToolLaunch(filePath);
>> +                    }
>> +                } catch (Exception e) {

I don't see any InterruptedException handling here, do I? Well, that's one 
reason why you should not "milly nilly" catch all exceptions into one handler. 
In case of an InterruptedException Process.destroy() should be called here. 
(head shake) :-(

>> +                    OutputController.getLogger().log(e);
>> +                    showCouldNotOpenFileDialog(frame, filePath,
>> R("CPPolicyEditorNotFound"));
>> +                }
>> +            }
>> +        }).start();
>> +    }
>> +
>
> [...]

Jacob