On Fri, 27 Mar 2020 12:12:30 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> 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. The one thing I would ask you to >> take a look at is whether it would matter if the old skin didn't call >> `setDefaultButton(oldScene, false)` when removed >> (and similarly `setCancelButton`). > >> >> >> 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. > > > The listener does not get early GCed here. I did verify this by creating > large number of `ButtonSkin`s. The latest > `ButtonSkin` set on the `Button` is never GCed >> >> >>The one thing I would ask you to take a look at is whether it would matter if >>the old skin didn't call >>`setDefaultButton(oldScene, false)` when removed (and similarly >>`setCancelButton`). > > > This seems to be a bigger issue. > If we create multiple `ButtonSkin`s for a given `Button`, then all the > `ButtonSkin` register listeners with the > properties of that `Button`. And it results in various listeners being added > to same property of the `Button`. and this > should be a common issue across all skins. Hi Kevin, Please take a look at the updated changes. Added tests as we discussed and an explicit call to remove the listener. ------------- PR: https://git.openjdk.java.net/jfx/pull/147
