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

[Jacob Wisor]

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)
>>>
>>> Ouch. See my reply to patch.
>>
>> Didn't see any mail, but we discussed on IRC. Here's a recap:
>> - The reflection hack is horrible. It doesn't quite work, and it's super ugly.
>> - The process not exiting is definitely a problem. We should not be playing
>> around with reflection on the policytool like this.
>> - Calling PolicyTool.main() directly breaks the automated build(?). We don't
>> want to make it a build dep, and it's too minor to bother with a configure
>> step/flag(?).
>> - Solution: Runtime#exec() makes its return, with a fallback method of
>> reflectively invoking PolicyTool.main() if for any reason the exec fails
>> (non-POSIX system, executable not on PATH, etc)
>>
>> So here's a patch implementing this solution, with lots of extra added
>> documentation.
>>
>
> And this time with better respect for event dispatch thread, I hope. (thanks
> omajid)

+    private static void policyToolLaunchHelper(final JFrame frame, final String 
filePath) {
+        final String[] launchCommand = new String[] { "policytool", "-file", 
filePath };

Ah, the caveats of anonymous classes and threads again: The launchCommand local 
constant is not guaranteed to be available when Runnable.run() is scheduled to run.

+        new Thread(new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    final Process ptool = Runtime.getRuntime().exec(launchCommand);

The launchCommand local constant referenced here which lives on the stack of the 
anonymous class's enclosing policyToolLaunchHelper() method is not guaranteed to 
be valid or to exist after the thread had been started. policyToolLaunchHelper() 
may terminate and thus its local stack be cleaned up /before/ Runnable.run() is 
scheduled to run, leaving no constant for reference. I am not entirely sure 
about what the Java Programming Language Spec and/or JVM Spec define in this 
case but I would bet that it has intentionally been left undefined. There is 
certainly no problem when anonymous class's methods and their enclosing methods 
share local variables running on the same thread. This case has been defined. 
But, in the case of threads, I would be rather careful. Sorry, I do not have the 
time to look it up in the specs. :-/

To get thread-safe, you would have to pass launchCommand as a parameter to 
Runnable's constructor and store its reference in a member variable.

Btw, the preferred way to construct Process objects since JDK 1.5 is to use 
ProcessBuilder. ProcessBuilder would also allow you to set policytool's current 
working directory to user.home, which is advisable.

+                    if (ptool.waitFor() != 0) {
+                        reflectivePolicyToolLaunch(filePath);
+                    }
+                } catch (Exception e) {
+                    OutputController.getLogger().log(e);
+                    showCouldNotOpenFileDialog(frame, filePath, 
R("CPPolicyEditorNotFound"));
+                }
+            }
+        }).start();
+    }

[...]

      private static OpenFileResult canOpenPolicyFile(final File policyFile) {

You may want to consider moving this method and its dependencies into 
net.sourceforge.jnlp.util.FileUtils?

Jacob

p.s.: Oh, one more thing: Please post rfc patches always based on tip, not based 
on previously posted rfc patches. Otherwise they are difficult to track. Thank you.