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

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport
From:       Michael =?UTF-8?B?U3RyYXXDnw==?= <mstrauss () openjdk ! java ! net>
Date:       2021-10-31 16:39:07
Message-ID: TOf1L_Cd8L25xBunGQ-wdeKAwRRBWm3jsyoOtYs4VdM=.b58df7d1-b2a6-4423-b204-2d220d6ecfd0 () github ! com
[Download RAW message or body]

On Sat, 30 Oct 2021 10:56:40 GMT, Florian Kirmaier <fkirmaier@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.

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;

Minor: whitespace at the beginning of the line

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.

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

> 87:         if (sceneChangeListenerMap.containsKey(anchor)) {
> 88:             ChangeListener<Scene> listener = \
>                 sceneChangeListenerMap.get(anchor).get();
> 89:             if(listener != null) {

Minor: space before brace

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.

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

> 253:             if (sceneChangeListenerMap.containsKey(anchor)) {
> 254:                 ChangeListener<Scene> listener = \
>                 sceneChangeListenerMap.get(anchor).get();
> 255:                 if(listener != null) {

Minor: space before brace

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

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


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