<Swing Dev> [10] RFR JDK-8175968: The javax.swing.filechooser.FileSystemView constructor consumes memory by adding a PropertyChangeListener that is never removed.

[Luke]

Hi all,

 > but imho it still beats adding an extra method to the API
 > and shifting the burden to the API user.

Just a question, would you be an "API user" of this method
directly? It seems to me that dispose() could be called somewhere
knowing more about the lifetime of this object (possibly from
BasicFileChooserUI) so that it remains an implementation
detail to most users of JFileChooser.

The WeakReference trick would then just be a (poor) fallback
mechanism.

So this trick (introduced somewhere between Java 6 and 8)
never worked. As mentioned, the key is to ensure the listeneris
a static inner class. For example, this works:

     private PropertyChangeListener pcl;

     public FileSystemView() {
         pcl = createPropertyChangeListener();
     }

     private static createPropertyChangeListener() {
          // current content of constructor goes here
     }

The 'static' is all-important, so that the anonymous listener class
becomes static also, and doesn't have a synthetic reference to
the outer class.


 > Thanks to the weak listener, the FileSystemView will get GC-ed as
 > the listener no longer keeps a hard reference to it.

I don't think a Weak Listener helps - the code you referenced earlier
suffers the same limitation as the current code: The reference
isn't freedunless a new property change event comes along. Property
changeevents are very infrequent from UIManager - typically never.


I've also been struggling to see a good alternative to dispose().
Finalizers came to mind also but ... (UPDATE: deprecated now huh?
OK forget it then.)

An alternative could be a timer, but that's a lot of extra baggage.

IF THIS WERE APPLICATION CODE a trick like this might work,
removing the need for both the listener and dispose():

     private Boolean useSystemExtensionHiding = null;

     private boolean getUseSystemExtensionHiding() {

         assert SwingUtilities.isEventDispatchThread();

         if (useSystemExtensionHiding == null) {
             useSystemExtensionHiding =
                 UIManager.getDefaults().getBoolean("FileChooser.useSystemExtensionHiding");

             SwingUtilities.invokeLater(() -> useSystemExtensionHiding = null);
         }

         return useSystemExtensionHiding;
     }

After all, the caching is just an optimisation to avoid repeated
map-lookups during painting (right?) The above reduces it to
once per full component paint, which is a small cost.

But being library code I'm not sure if it's safe to make such
assumptions (that the EDT event loop is running, for example).
(Expert opinion?)


ACTUALLY if avoiding dispose() is preferred - then I wonder if the
optimisation is "past its expiry date"? The "free lunch may be
over" now, but it was still being delivered since Java 1.1 :-)

What is the cost of one map lookup (per file) compared to the cost
of painting an icon for each file, for example? I'd guess they're
about on-par.

Would it be so bad to just drop the caching of this boolean altogether?

Kind regards,
Luke



On 07/07/2017 09:21, Robin Stevens wrote:
> I still do not think you need a public dispose method.
> If you use the weak listener (the correct implementation), the 
> FileSystemView instance can be GC-ed, even when the 
> PropertyChangeListener is still attached to the UIManager.
>
> What you can do is:
> - Store a reference to the PropertyChangeListener in the 
> FileSystemView class as a field.
> - Override the finalize method of the FileSystemView class to 
> de-register the PropertyChangeListener from the UIManager. Thanks to 
> the weak listener, the FileSystemView will get GC-ed as the listener 
> no longer keeps a hard reference to it.
>
> Not the nicest way to do such cleanup (which should probably happen on 
> the EDT) in a finalizer, but imho it still beats adding an extra 
> method to the API and shifting the burden to the API user.
>
> Robin
>
> On Thu, Jul 6, 2017 at 3:15 PM, Prasanta Sadhukhan 
> <prasanta.sadhukhan at oracle.com 
> <mailto:prasanta.sadhukhan at oracle.com>>wrote:
>
>     Hi Sergey,
>
>     I tried with the proposed WeakPropertyChangeListener but as far I
>     understand & tested, it seems also to rely on propertyChange() API
>     to be called, so the scenario mentioned in the bug will still fail.
>     I cannot find any other way rather than calling dispose() for the
>     scenario mentioned there.
>
>     Regards
>     Prasanta
>
>     On 7/5/2017 4:35 AM, Sergey Bylokhov wrote:
>
>         Hi, Prasanta.
>         Probably it is possible to implement it without users
>         interaction and new api?
>
>         ----- prasanta.sadhukhan at oracle.com
>         <mailto:prasanta.sadhukhan at oracle.com>wrote:
>
>             Hi All,
>
>             Please review a fix for a memory leak issue where
>             PropertyChangeListener
>             object added by FileSystemView constructor is never removed.
>             Proposed fix is to add dispose() method to be called by
>             app when they
>
>             want to remove this resource.
>
>             Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
>             <https://bugs.openjdk.java.net/browse/JDK-8175968>
>             webrev:
>             http://cr.openjdk.java.net/~psadhukhan/8175968/webrev.00/
>             <http://cr.openjdk.java.net/%7Epsadhukhan/8175968/webrev.00/>
>
>             Regards
>             Prasanta
>
>
>