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.

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

Commit messages:
 - No longer use ObservableList as keys
 - JDK-8283551 ControlAcceleratorSupport menu items listener causes memory leak

Changes: https://git.openjdk.org/jfx/pull/1037/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1037&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8283551
  Stats: 343 lines in 4 files changed: 323 ins; 1 del; 19 mod
  Patch: https://git.openjdk.org/jfx/pull/1037.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/1037/head:pull/1037

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

Reply via email to