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

[Florian Kirmaier]

On Sun, 31 Oct 2021 16:32:49 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8274022
>>   Simplified code related to WeakHashMaps
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 88:
> 
>> 86:         // Remove previously added listener if any
>> 87:         if (sceneChangeListenerMap.containsKey(anchor)) {
>> 88:             ChangeListener<Scene> listener = sceneChangeListenerMap.get(anchor).get();
> 
> It seems unusual to check for the existence of a weak key (containsKey), and then just assume it still exists (get). You should probably get() the value directly without checking containsKey() first, and then check whether the returned value is null.

Yes, you are right. 
This issue also existed in the previous version. 
It can't cause problems, because the key is held as a parameter in the method, and the keys equal method is implemented by comparing references. But that's an unnecessarily complicated argument. 
It's way easier to just make one call without any timing issue.

> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 254:
> 
>> 252:             // at the time of installing the accelerators.
>> 253:             if (sceneChangeListenerMap.containsKey(anchor)) {
>> 254:                 ChangeListener<Scene> listener = sceneChangeListenerMap.get(anchor).get();
> 
> It seems unusual to check for the existence of a weak key (containsKey), and then just assume it still exists (get). You should probably get() the value directly without checking containsKey() first, and then check whether the returned value is null.

The conversation is below in the other reviewed code.

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

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