[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8236840: Memory leak when switching ButtonSkin
From:       Ambarish Rapte <arapte () openjdk ! java ! net>
Date:       2020-03-28 6:42:25
Message-ID: DVCYc_dpCkBVCVPL7a5uI04Bi2-vJM-Y2vVeBWI2bjY=.1c1f0677-685f-43cc-9c32-144c381979ba () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]