[rfc][icedtea-web][policyeditor] Reduced usage of weak references

[Andrew Azores]

On 06/11/2014 02:04 PM, Omair Majid wrote:
> Hi,
>
> Thanks for the cleanup!
>
> * Andrew Azores <aazores at redhat.com> [2014-06-11 13:55]:
>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> @@ -173,6 +173,13 @@ public class PolicyEditor extends JPanel
>>               addCodebaseButton = new JButton(), removeCodebaseButton = new JButton();
>>       private final JFileChooser fileChooser;
>>       private CustomPolicyViewer cpViewer = null;
>> +
>> +    /**
>> +     * See showChangesSavedDialog. This weak reference is needed because there is a modal child
>> +     * dialog which can sometimes appear after the editor has been closed and disposed. In this
>> +     * case, its parent should be set to 'null', but otherwise the parent should be the editor
>> +     * so that the dialog is modal.
>> +     */
>>       private final WeakReference<PolicyEditor> weakThis = new WeakReference<>(this);
> In this case, please call it something more appropriate, Like
> 'parentDialog', and change the reference type to Window. It makes sense
> that the code that expects a parent uses null whereas `weakThis` seems
> like a really odd sort of a name.

Well, PolicyEditor is actually a JPanel right now, so changing the weak 
reference type to Window is not trivial. I'm going to start on a proper 
refactor though to separate the actual policy editing from the 
Window/Dialog/Panel stuff, so that change could be bundled with that as 
well.

>
>>       private void showChangesSavedDialog() {
>> +        // This dialog is often displayed when closing the editor, and so PolicyEditor
>> +        // may already be disposed when this dialog appears. Give a weak reference so
>> +        // that this dialog doesn't prevent GC of the editor
> Is preventing GC in this such a bad thing? Presumably, this will only be
> an issue for a few seconds, right? Because then the dialog goes away.
>
> Thanks,
> Omair
>

I should probably rewrite that comment actually. It isn't just 
preventing GC of the parent until the modal dialog goes away, it 
actually causes the JVM to never (or at least not within several 
minutes) exit, even after the modal "Changes Saved" dialog has 
disappeared. I'm still no Swing expert so I don't know exactly what's 
going on here but it seems something isn't being disposed correctly or 
some thread stays blocked, I don't know. I suspect it's because the 
parent window has already been disposed by the time the child modal 
appears, but I don't know what else to do about this other than give it 
a weak reference so that it just doesn't have a parent (or rather has 
whatever default parent) if the PolicyEditorWindow is already gone.

Anyway, patch attached has weakThis renamed to parentPolicyEditor. 
showCouldNotSaveDialog also uses it for the same reason that 
showChangesSavedDialog does, since I noticed that it had also been 
changed to use PolicyEditor.this even though this dialog can also appear 
the same way after the editor has been disposed. The comment explaining 
the use of the weak reference also says it prevents JVM exit, not GC.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: policyeditor-weakthis-4.patch
Type: text/x-patch
Size: 12791 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140611/1afddf0d/policyeditor-weakthis-4-0001.patch>