Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274022 Additional Memory Leak in ControlAcceleratorSupport #659

Conversation

FlorianKirmaier
Copy link
Member

@FlorianKirmaier FlorianKirmaier commented Oct 30, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8274022: Additional Memory Leak in ControlAcceleratorSupport

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659
$ git checkout pull/659

Update a local copy of the PR:
$ git checkout pull/659
$ git pull https://git.openjdk.java.net/jfx pull/659/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 659

View PR using the GUI difftool:
$ git pr show -t 659

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/659.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 30, 2021

👋 Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Oct 30, 2021
@mlbridge
Copy link

mlbridge bot commented Oct 30, 2021

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Oct 30, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 30, 2021

  • 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

@@ -83,7 +85,10 @@ public static void addAcceleratorsIntoScene(ObservableList<MenuItem> items, Node
// 2. Removing accelerators when Control is removed from Scene
// Remove previously added listener if any
if (sceneChangeListenerMap.containsKey(anchor)) {
anchor.sceneProperty().removeListener(sceneChangeListenerMap.get(anchor));
ChangeListener<Scene> listener = sceneChangeListenerMap.get(anchor).get();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 289 to 291
WeakReference<ChangeListener<KeyCombination>> listenerW = changeListenerMap.get(menuitem);
ChangeListener<KeyCombination> listener = listenerW == null ? null : listenerW.get();
if (listener != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

removed an unused import
the logic to remove the keys from the map, is now more consistent
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Btw, you still need to change the title of this PR to match the JBS title.

@FlorianKirmaier FlorianKirmaier changed the title 8274022 fixing critical memory leak in the ControlAcceleratorSupport 8274022 Additional Memory Leak in ControlAcceleratorSupport Nov 15, 2021
@FlorianKirmaier
Copy link
Member Author

Great, I've just updated the title!

@kevinrushforth
Copy link
Member

@mstr2 can you re-review this?

Copy link
Collaborator

@mstr2 mstr2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@openjdk
Copy link

openjdk bot commented Nov 15, 2021

@FlorianKirmaier This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8274022: Additional Memory Leak in ControlAcceleratorSupport

Reviewed-by: mstrauss, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 9 new commits pushed to the master branch:

  • 7ddd642: 8232812: [MacOS] Double click title bar does not restore window size
  • d122ed0: 8276915: Crash on iOS 15.1 in GlassRunnable::dealloc
  • effcc86: 8274929: Crash while reading specific clipboard content
  • 6749ab6: 8274854: Mnemonics for menu containing numeric text not working
  • 4d8e12d: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
  • 5fc047b: 8272808: Update constant collections to use the new immutable collections - leftovers
  • 7d6493b: 8275911: Keyboard doesn't show when tapping inside an iOS text input control
  • cde72c8: 8276179: PrismFontFile.isInstalledFont is dead code and should be removed
  • 6c88106: 8222455: JavaFX error loading glass.dll from cache

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Ready to be integrated label Nov 15, 2021
@FlorianKirmaier
Copy link
Member Author

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Nov 15, 2021
@openjdk
Copy link

openjdk bot commented Nov 15, 2021

@FlorianKirmaier
Your change (at version 873c309) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Nov 15, 2021

Going to push as commit 0d5b8f8.
Since your change was applied there have been 9 commits pushed to the master branch:

  • 7ddd642: 8232812: [MacOS] Double click title bar does not restore window size
  • d122ed0: 8276915: Crash on iOS 15.1 in GlassRunnable::dealloc
  • effcc86: 8274929: Crash while reading specific clipboard content
  • 6749ab6: 8274854: Mnemonics for menu containing numeric text not working
  • 4d8e12d: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
  • 5fc047b: 8272808: Update constant collections to use the new immutable collections - leftovers
  • 7d6493b: 8275911: Keyboard doesn't show when tapping inside an iOS text input control
  • cde72c8: 8276179: PrismFontFile.isInstalledFont is dead code and should be removed
  • 6c88106: 8222455: JavaFX error loading glass.dll from cache

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 15, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Nov 15, 2021
@openjdk
Copy link

openjdk bot commented Nov 15, 2021

@kevinrushforth @FlorianKirmaier Pushed as commit 0d5b8f8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants