8208088: Memory Leak in ControlAcceleratorSupport

[arapte]

The method ControlAcceleratorSupport.doAcceleratorInstall(final List<? extends MenuItem> items, final Scene scene) adds a ChangeListener on MenuItem.acceleratorProperty(). This listener is not removed when the MenuItem is removed from scenegraph.
Adding and removing a MenuItem results in multiple copies of the listener added to MenuItem.acceleratorProperty().

Fix is to remove the listener when MenuItem is removed.
Fix can be verified by checking the number of instances of ControlAcceleratorSupport.lambda using jvisualvm.
Without this fix, the number of ControlAcceleratorSupport.lambda increase in multiple of number of MenuItems being removed and added back.
With fix, the count is always same as number of MenuItems in scenegraph.

Also there is another ListChangeListener added to a ObservableList<MenuItem> items in the method ControlAcceleratorSupport.doAcceleratorInstall(final ObservableList<MenuItem> items, final Scene scene). There was a TODO note to remove this listener.
This listener is added on MenuBarButton.getItems() and not on Menu.getItems(). This MenuBarButton is created by MenuBarSkin to show a Menu. This MenuBarButton gets disposed when the related Menu is removed from scenegraph, and so the added ListChangeListener gets GCed. Hence it is not required to explicitly remove the listener.
Added a comment explaining this behavior in place of the TODO.

---

Progress

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

Issue

• JDK-8208088: Memory Leak in ControlAcceleratorSupport

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 429

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

Using diff file

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

[bridgekeeper]

👋 Welcome back arapte! 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.

[mlbridge]

Webrevs

• 05: Full - Incremental (4a52e4ac)
• 04: Full - Incremental (91d43d8e)
• 03: Full - Incremental (88b5e25d)
• 02: Full - Incremental (62344c7)
• 01: Full - Incremental (4ed3f72)
• 00: Full (93ccb9f)

[kevinrushforth]

/reviewers 2

[kevinrushforth]

Is there a unit test that can validate this fix?

[openjdk]

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

[arapte]

Is there a unit test that can validate this fix?

I have added a unit test using a new shim class. Test verifies size of the map that is added as part of this fix.
So the test won't compile without this PR.

[kevinrushforth]

The new unit test fails on all platforms, as you can see from the GitHub actions log:

2021-03-18T12:45:36.7904530Z test.javafx.scene.control.ControlAcceleratorSupportTest > sanityTestListenerMapShouldBeEmpty FAILED
2021-03-18T12:45:36.8007000Z     java.lang.AssertionError: expected:<0> but was:<22>
2021-03-18T12:45:36.8099570Z         at org.junit.Assert.fail(Assert.java:91)
2021-03-18T12:45:36.8201200Z         at org.junit.Assert.failNotEquals(Assert.java:645)
2021-03-18T12:45:36.8264450Z         at org.junit.Assert.assertEquals(Assert.java:126)
2021-03-18T12:45:36.8366120Z         at org.junit.Assert.assertEquals(Assert.java:470)
2021-03-18T12:45:36.8455230Z         at org.junit.Assert.assertEquals(Assert.java:454)
2021-03-18T12:45:36.8559230Z         at test.javafx.scene.control.ControlAcceleratorSupportTest.sanityTestListenerMapShouldBeEmpty(ControlAcceleratorSupportTest.java:43)
2021-03-18T12:45:36.8886110Z 
2021-03-18T12:45:36.8989430Z test.javafx.scene.control.ControlAcceleratorSupportTest > testNumberOfListenersByRemovingAndAddingMenuItems FAILED
2021-03-18T12:45:36.9092540Z     java.lang.AssertionError: expected:<4> but was:<26>
2021-03-18T12:45:36.9194300Z         at org.junit.Assert.fail(Assert.java:91)
2021-03-18T12:45:36.9296390Z         at org.junit.Assert.failNotEquals(Assert.java:645)
2021-03-18T12:45:36.9393550Z         at org.junit.Assert.assertEquals(Assert.java:126)
2021-03-18T12:45:36.9495860Z         at org.junit.Assert.assertEquals(Assert.java:470)
2021-03-18T12:45:36.9596430Z         at org.junit.Assert.assertEquals(Assert.java:454)
2021-03-18T12:45:36.9700420Z         at test.javafx.scene.control.ControlAcceleratorSupportTest.testNumberOfListenersByRemovingAndAddingMenuItems(ControlAcceleratorSupportTest.java:65)

[arapte]

The new unit test fails on all platforms, as you can see from the GitHub actions log:

The test failed because, change listeners added by other tests get accumulated in the changeListenerMap. These listeners can get GCed only if the tests correctly cleanup themselves. The test passes if executed individually.
The test needed correction that, it should not assume the Map to be empty, instead verify fix by using current size of the changeListenerMap as reference.
The Windows pre-submit test action has failed because Task :graphics:compileDecoraNativeShadersWin FAILED, and not because of the new test.
Please take a look.

[kevinrushforth]

The fix looks good.

The test looks OK as far as it goes, although I note that it is testing for the leak in an indirect way, since it looks at the changeListenerMap rather than checking the listeners of menuitem.acceleratorProperty() which is what we really care about. Is there a way to test it more directly? If not then I think this is fine.

[arapte]

Is there a way to test it more directly?

ControlTestUtils provides a way to get the number of listeners added to a property. Please check the updated test. With this direct way, should we keep the indirect way of testing which uses changeListenerMap ?

[kevinrushforth]

The updated test looks good. I confirm that it fails without the fix and passes with the fix. In order to do that I locally deleted the newly added shim class and comment out all calls to get_ListenerMapSize. I'd recommend to get rid of the indirect way of testing, since it will mean you can remove the newly added ControlAcceleratorSupportShim class and the ControlAcceleratorSupport ::testGetListenerMapSize method.

[arapte]

I'd recommend to get rid of the indirect way of testing

I have removed the indirect way, please take a look.

[openjdk]

@arapte 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:

8208088: Memory Leak in ControlAcceleratorSupport

Reviewed-by: kcr, aghaisas

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 22 new commits pushed to the master branch:

• e8689fe: 8265031: Change default macOS min version for x86_64 to 10.12 and aarch64 to 11.0
• c8384a1: 8264928: Update to Xcode 12.4
• 808b107: 8260245: Update ICU4C to version 68.2
• d808dd1: 8258663: Fixed size TableCells are not removed from sene graph when column is removed
• 28475cb: 8263807: Button types of a DialogPane are set twice, returns a wrong button
• cc94e96: 8217955: Problems with touch input and JavaFX 11
• 9796a83: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock
• 5898858: 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set
• e63931e: 8262366: Update glib to version 2.66.7
• b2f842d: 8259555: Webkit crashes on Apple Silicon
• ... and 12 more: https://git.openjdk.java.net/jfx/compare/8adbc673d095607e8a6109fbb951fa17b9d6caad...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[arapte]

/integrate

[openjdk]

@arapte Since your change was applied there have been 22 commits pushed to the master branch:

• e8689fe: 8265031: Change default macOS min version for x86_64 to 10.12 and aarch64 to 11.0
• c8384a1: 8264928: Update to Xcode 12.4
• 808b107: 8260245: Update ICU4C to version 68.2
• d808dd1: 8258663: Fixed size TableCells are not removed from sene graph when column is removed
• 28475cb: 8263807: Button types of a DialogPane are set twice, returns a wrong button
• cc94e96: 8217955: Problems with touch input and JavaFX 11
• 9796a83: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock
• 5898858: 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set
• e63931e: 8262366: Update glib to version 2.66.7
• b2f842d: 8259555: Webkit crashes on Apple Silicon
• ... and 12 more: https://git.openjdk.java.net/jfx/compare/8adbc673d095607e8a6109fbb951fa17b9d6caad...master

Your commit was automatically rebased without conflicts.

Pushed as commit 05ab799.

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

[RationalityFrontline]

After updating to jfx 17, I detected memory leak in my application (every controller that has menu items won't get garbage collected after closing its stage), with visualvm I found it was caused by class ControlAcceleratorSupport. This kind of memory leak doesn't happen on jfx 16, so I guess there is something wrong with this PR.

I've created a sample project that could reproduce and verify the memory leak: jfx-test (on git branch ControlAcceleratorSupport).

Run command gradlew run and you should see the following ui interface:

Clicking button Call GC and Print MenuItems will call System.gc() and print current ungarbaged menu items to console, clicking menu Restart Stage will call Stage.close() and launch a new same Stage.

After clicking Restart Stage several times, click Call GC and Print MenuItems, you will see lots of ungarbaged menu items. However, by changing jfx version to 16 in build.gradle.kts, you will always see only one menu item.

If menu items are set with action listeners, then these listeners also won't be garbage collected, typically these listeners hold references to controllers, which made all closed controllers leaked. This made jfx 17 unusable.

[Maran23]

After updating to jfx 17, I detected memory leak in my application (every controller that has menu items won't get garbage collected after closing its stage), with visualvm I found it was caused by class ControlAcceleratorSupport. This kind of memory leak doesn't happen on jfx 16, so I guess there is something wrong with this PR.

I've created a sample project that could reproduce and verify the memory leak: jfx-test (on git branch ControlAcceleratorSupport).

Run command gradlew run and you should see the following ui interface:

Clicking button Call GC and Print MenuItems will call System.gc() and print current ungarbaged menu items to console, clicking menu Restart Stage will call Stage.close() and launch a new same Stage.

After clicking Restart Stage several times, click Call GC and Print MenuItems, you will see lots of ungarbaged menu items. However, by changing jfx version to 16 in build.gradle.kts, you will always see only one menu item.

If menu items are set with action listeners, then these listeners also won't be garbage collected, typically these listeners hold references to controllers, which made all closed controllers leaked. This made jfx 17 unusable.

Could you please report the bug at https://bugreport.java.com/ with a minimum reproducible example (written in Java)?

[kevinrushforth]

Could you please report the bug at https://bugreport.java.com/ with a minimum reproducible example (written in Java)?

I was just going to mention the same thing. See the project README.

[mlbridge]

Mailing list message from Daniel Peintner on openjfx-dev:

Hi,

I don't wanna clutter the list but I noticed this issue also and can point
you to a simple test (adding MenuButton is sufficient, no need for any
MenuItem).

See test here:
https://github.com/Sandec/JMemoryBuddy/issues/5#issuecomment-911617849

The test provided fails on JavaFX (v17) and succeeds on v16.
@FlorianKirmaier mentioned that for him the test on Mac works also for
JavaFX17. Maybe it depends on the operating system also...

Note: The test should be usable "as is" since it uses JMemoryBuddy which is
already part of the JFX repo.
(some minor tweaks might be necessary due to the minor assert differences
in Junit4 vs. Junit5)

Hope this helps,

-- Daniel

On Fri, Sep 10, 2021 at 1:40 PM Kevin Rushforth <kcr at openjdk.java.net>
wrote:

[RationalityFrontline]

@Maran23 @kevinrushforth Bug reported via https://bugreport.java.com/bugreport/ with a minimum reproducible example written in Java, internal review ID : 9071415.