RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]

[Kevin Rushforth]

On Tue, 2 Nov 2021 08:16:41 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17.
>> To fix it, I've made the Value of the WeakHashMap also weak. 
>> We only keep this value to remove it as a listener later on. Therefore there shouldn't be issues by making this value weak.
>> 
>> 
>> I've seen this Bug very very often, in the last weeks. Most of the applications I've seen are somehow affected by this bug.
>> 
>> It basically **breaks every application with menu bars and multiple stages** - which is the majority of enterprise applications. It's especially annoying because it takes some time until the application gets unstable.
>> 
>> Therefore **I would recommend** after this fix is approved, **to make a new version for JavaFX17** with this fix because this bug is so severe.
>
> Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8274022
>   Simplified code related to WeakHashMaps

The fix looks good, although I left one question inline (also there is an unused import).

The test looks good. I verified that it fails without your fix and passes with your fix.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 31:

> 29: import javafx.beans.property.ReadOnlyObjectProperty;
> 30: import javafx.beans.value.ChangeListener;
> 31: import javafx.beans.value.WeakChangeListener;

Unused import.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 291:

> 289:                 WeakReference<ChangeListener<KeyCombination>> listenerW = changeListenerMap.get(menuitem);
> 290:                 ChangeListener<KeyCombination> listener = listenerW == null ? null : listenerW.get();
> 291:                 if (listener != null) {

This will fail to remove a weak reference to a listener that has been collected. It is a `WeakHashMap`, so the `WeakReference` to the listener will be reclaimed as soon as the `menuItem` is no longer reachable, but if you used the same logic as you did for the scene listener (e.g., on lines 254-261), it would be removed sooner. I don't know whether it matters in this case.

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

PR: https://git.openjdk.java.net/jfx/pull/659