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

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
From:       Ambarish Rapte <arapte () openjdk ! org>
Date:       2023-06-28 6:28:13
Message-ID: oQoQAxLXh7HCyjdGfbjdGM8JLjDwANjVrw3LfQZ-prA=.1fd46c1e-5fd8-45d9-bafc-23446ae4d66b () github ! com
[Download RAW message or body]

On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey <dwookey@openjdk.org> wrote:

> > 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.
> 
> Dean Wookey has updated the pull request incrementally with one additional commit \
> since the last revision: 
> Added more comments and fixed IdentityWrapper hashcode.

Apologies for the delay.
This looks good to me. Have no comments. please integrate.

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

PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1610827994


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