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

[Lukasz Dracz]

Hello,

> > 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.

Okay, sounds good.

> >
> >>       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.

The patch looks good to me. +0.5.

Thank you,
Lukasz Dracz