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

[Sergey Bylokhov]

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


-- 
Best regards, Sergey.