RFR: 8236840: Memory leak when switching ButtonSkin

[Ambarish Rapte]

On Fri, 27 Mar 2020 12:12:30 GMT, Ambarish Rapte <arapte at 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