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

[Sergey Bylokhov]

Hi,
I would like to propose a new review as a solution which I think should solve the problem:
Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
Webrev can be found at: http://cr.openjdk.java.net/~serb/8175968/webrev.00

----- ldubox-coding101 at yahoo.co.uk wrote:

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