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

List:       openjdk-openjfx-dev
Subject:    Integrated: 8283551: ControlAcceleratorSupport menu items listener causes memory leak
From:       Dean Wookey <dwookey () openjdk ! org>
Date:       2023-06-28 7:01:15
Message-ID: aOA7lxcay_svyYH1k23z8Tq43gmYYmUqT22YhaMmw2Q=.83d0c80f-d168-4a5b-b537-e36e514911f0 () github ! com
[Download RAW message or body]

On Thu, 16 Feb 2023 15:15:11 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.

This pull request has now been integrated.

Changeset: 8fd2943c
Author:    Dean Wookey <dwookey@openjdk.org>
Committer: Ambarish Rapte <arapte@openjdk.org>
URL:       https://git.openjdk.org/jfx/commit/8fd2943c52cb47ec247ce0e6657afdc9bdc725ae
                
Stats:     426 lines in 4 files changed: 406 ins; 1 del; 19 mod

8283551: ControlAcceleratorSupport menu items listener causes memory leak

Reviewed-by: angorya, mstrauss

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

PR: https://git.openjdk.org/jfx/pull/1037


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