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

[Alexander Zvegintsev]

Looks fine.

Thanks,
Alexander.

On 03/09/2017 07:01, Sergey Bylokhov wrote:
> Any volunteers to review?
> Thank you =)
>
> On 8/27/17 14:19, Sergey Bylokhov wrote:
>> 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
>>>>
>>>>
>>>>
>
>