RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak

[Dean Wookey]

On Wed, 22 Feb 2023 19:21:41 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Each time a menu would change scenes, a new set of ListChangeListeners would be added to the items in the menu. The bigger problem however is that these list change listeners have a strong reference to the scene which is potentially a much bigger leak.
>> 
>> The first commit was more straightforward, but there are 2 things that might be of concern:
>> 
>> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but it'll remove all the ListChangeListeners attached to it, regardless of which scene was passed in. Something similar happens with changeListenerMap already, so I think it's fine.
>> 2. I made the remove method public so that external calls from skins to remove the accelerators would remove the ListChangeListener and also because all the remove methods are public. 
>> 
>> After I wrote more tests I realised using the ObservableLists as keys in the WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would be simple. The fix is in the second commit.
>> 
>> There are still more issues that stem from the fact that for each anchor there could be multiple menus and the current code doesn't account for that. For example, swapping context menus on a tab doesn't register change listeners on the new context menu because the TabPane itself had a scene change listener already. These other issues could relate to JDK-8268374 https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to the fact that there's no logic to remove listeners when Tab/TableColumn's are removed from their associated control (TabPane, TableView, TreeTableView).
>> 
>> I'm looking at these issues, but I think they're dependent on this fix. Either I can add to this PR or I can wait to see what comes out of this and fix them subsequently.
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 160:
> 
>> 158:         };
>> 159: 
>> 160:         menuListChangeListenerMap.put(listChangeListener, new WeakReference<>(listChangeListener));
> 
> is it possible this method will be called twice with the same (items, scene) arguments?  i.e. is it possible to clobber an already registered listChangeListener?

Maybe. I could check if one's there and remove it? If it happened, there would be more change listeners on the list, but only 1 in the HashMap, and when removed, only one of the copies would be removed.

I'm not sure it can happen because of the checks in the methods that call this method. It hasn't come up in my tests, but I also don't think there's harm in removing one if there is before.

-------------

PR: https://git.openjdk.org/jfx/pull/1037