On Thu, 16 Feb 2023 15:15:11 GMT, Dean Wookey <dwoo...@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. This pull request has now been integrated. Changeset: 8fd2943c Author: Dean Wookey <dwoo...@openjdk.org> Committer: Ambarish Rapte <ara...@openjdk.org> URL: https://git.openjdk.org/jfx/commit/8fd2943c52cb47ec247ce0e6657afdc9bdc725ae Stats: 426 lines in 4 files changed: 406 ins; 1 del; 19 mod 8283551: ControlAcceleratorSupport menu items listener causes memory leak Reviewed-by: angorya, mstrauss ------------- PR: https://git.openjdk.org/jfx/pull/1037
