On Tue, 2 Nov 2021 08:16:41 GMT, Florian Kirmaier <fkirma...@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