On Mon, 23 Mar 2020 23:45:55 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> I have basically the same comment / question as I asked in #147 >> >> In general, there are two approaches to avoiding listener-related memory >> leaks. One is to use a WeakListener; the other >> is to explicitly remove the listener when the object is removed or otherwise >> no longer needed. >> Using a WeakListener is certainly easier, but runs the risk of the listener >> being removed too early and not cleaning up >> after itself. I'm not suggesting that's the case here, but it is worth >> looking at. > > @arapte @aghaisas - Can you both review this? I would like to widen the issue (and change this pull request accordingly) to include memory leaks of introduced selection- and focusModels in other controls that are caused by the basically same reason, that is strong references via listeners to controls' properties. The fix would be the same as well, that is wrap the listeners into weakXXListeners and register these. The models suggested to include here: - TreeView: selection- and focusModel - TreeTableView: focusModel - TabPane: selectionModel Not included would be memory leaks introduced by whacky listening of ChoiceBoxSkin and TabPaneSkin to properties of the selectionModel. The former will (most probably) be fixed as a side-effect of https://bugs.openjdk.java.net/browse/JDK-8087555 (which I intend to take, if Ajit agrees :), will open a separate issue for the latter. The extended fix here will have tests for replacing selection/focusModels with/out a skin registered for all controls having the respective models. Not entirely certain about the formal procedure (locally fix and tests are - nearly, pending some cleanup - ready). Simply change titles, rebase and push again? ------------- PR: https://git.openjdk.java.net/jfx/pull/148
