Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v5]
On Mon, 15 Nov 2021 08:24:04 GMT, Florian Kirmaier 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 > the logic to remove the keys from the map, is now more consistent Looks good. Btw, you still need to change the title of this PR to match the JBS title. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Fri, 12 Nov 2021 23:42:30 GMT, Kevin Rushforth 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 291:
>
>> 289: WeakReference> listenerW
>> = changeListenerMap.get(menuitem);
>> 290: ChangeListener 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.
Yes, you are right. I now use the same logic in all 3 places.
I guess it doesn't matter because the key always references the listener - but
the code is now more straight line.
-
PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v5]
> 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 the logic to remove the keys from the map, is now more consistent - Changes: - all: https://git.openjdk.java.net/jfx/pull/659/files - new: https://git.openjdk.java.net/jfx/pull/659/files/d2a1c448..873c3090 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=659=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=659=03-04 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v4]
> 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 removed an unused import - Changes: - all: https://git.openjdk.java.net/jfx/pull/659/files - new: https://git.openjdk.java.net/jfx/pull/659/files/bbc39f24..d2a1c448 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=659=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=659=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Fri, 12 Nov 2021 22:42:35 GMT, Kevin Rushforth 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 31: > >> 29: import javafx.beans.property.ReadOnlyObjectProperty; >> 30: import javafx.beans.value.ChangeListener; >> 31: import javafx.beans.value.WeakChangeListener; > > Unused import. removed! - PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Tue, 2 Nov 2021 08:16:41 GMT, Florian Kirmaier 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> listenerW
> = changeListenerMap.get(menuitem);
> 290: ChangeListener 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
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Tue, 2 Nov 2021 08:16:41 GMT, Florian Kirmaier 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 Marked as reviewed by mstrauss (Author). - PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/659/files - new: https://git.openjdk.java.net/jfx/pull/659/files/402fcd27..bbc39f24 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=659=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=659=01-02 Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Sun, 31 Oct 2021 16:32:49 GMT, Michael Strauß 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 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 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
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v2]
> 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 fixed some whitesapce formatting issues - Changes: - all: https://git.openjdk.java.net/jfx/pull/659/files - new: https://git.openjdk.java.net/jfx/pull/659/files/324188f1..402fcd27 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=659=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=659=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport
On Sat, 30 Oct 2021 10:56:40 GMT, Florian Kirmaier
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 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 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 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 listener =
> sceneChangeListenerMap.get(anchor).get();
> 255: if(listener != null) {
Minor: space before brace
-
PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport
On Sat, 30 Oct 2021 10:56:40 GMT, Florian Kirmaier 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. > * [JDK-8274022](https://bugs.openjdk.java.net/browse/JDK-8274022): > Additional Memory Leak in ControlAcceleratorSupport ⚠️ Title mismatch between > PR and JBS. @FlorianKirmaier as a reminder, the PR title must match the JBS title. Note that I modified the JBS title slightly, so when you do fix this, you will need to grab the updated title: 8274022: Additional Memory Leak in ControlAcceleratorSupport - PR: https://git.openjdk.java.net/jfx/pull/659
RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport
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. - Commit messages: - 8274022 fixing memory leak in the ControlAcceleratorSupport Changes: https://git.openjdk.java.net/jfx/pull/659/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=659=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274022 Stats: 39 lines in 2 files changed: 30 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
