jfx 8319779: SystemMenu: memory leak due to listener never being removed

trafficstars

A listener was added but never removed. This patch removes the listener when the menu it links to is cleared. Fix for https://bugs.openjdk.org/browse/JDK-8319779

Progress

[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8319779: SystemMenu: memory leak due to listener never being removed (Bug - P4)
JDK-8323787: Mac System MenuBar throws IOB exception (Bug - P3)

Reviewers

Jose Pereda (@jperedadnr - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1283/head:pull/1283
$ git checkout pull/1283

Update a local copy of the PR:
$ git checkout pull/1283
$ git pull https://git.openjdk.org/jfx.git pull/1283/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1283

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1283.diff

Webrev

Link to Webrev Comment

Nov 10 '23 10:11 johanvos

:wave: Welcome back jvos! 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.

Nov 10 '23 10:11 bridgekeeper[bot]

Webrevs

Nov 10 '23 10:11 mlbridge[bot]

/reviewers 2

Nov 10 '23 13:11 kevinrushforth

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Nov 10 '23 13:11 openjdk[bot]

@arapte can you be the primary reviewer for this?

Nov 14 '23 15:11 kevinrushforth

Looks all good to me. Is it possible to add an automated test ?

Dec 12 '23 07:12 arapte

About adding an automated test, the leak that PR tries to fix happens in com.sun.javafx.tk.quantum.GlassSystemMenu, which is package private, and adding new tests to check javafx.scene.control.Menu, or related public classes, won't catch that leak, will it?

Unless we open it (and all the related classes/methods in com.sun.glass.ui.*), I don't think it can be done?

Dec 13 '23 09:12 jperedadnr

As for the testing issue: the memoryleak before this fix leads to an increasing number of com.sun.glass.ui.MenuItem instances. We could test the previous memoryleak, as that was creating an increasing number of javafx.scene.control.MenuItem instances, which could be handled inside a JavaFX application and stored as WeakReferences in a list, that could then be required to be empty after GC. Our systemtest does not have access to the com.sun.glass.ui.MenuItem instances, so we can't use the same approach. In theory, it should be possible to mock the whole path that leads to this leak, but it is complex. I do understand the value of tests for this, though, as the fix is brittle. When a new InvalidationListener is added referencing the to-be-removed menuItems, the memoryleak will re-occur and go unnoticed.

Dec 18 '23 13:12 johanvos

Moving this to [WIP] while I'm working on automated tests.

Dec 19 '23 08:12 johanvos

Would it be an idea to do deterministic clean-up?

The JIRA tickets involved are light on details, and I don't have a Mac, so I can't test the leak (I think?), but it seems to me that you will want to remove the listeners when the menu bar isn't visible (and re-add them if it becomes visible again?).

From what I understand, these are normal JavaFX controls wrapped into adapters, and so any listeners installed through the adapter interface will end up on controls. The GlobalMenuAdapter seems to be tracking the showing property, and calls show / hide accordingly.

The ticket says:

when an application loses focus, the SystemMenu is removed. When it gets focus again, the SystemMenu is "back".

My question is, is there any event that can be used to trigger clean-up when this happens? Do the controls go invisible, are they no longer showing (is hide called?) There must be something. If there is something, then some kind of disposal can be triggered for deterministic clean-up (ie. add dispose method).

If you can go further and distill this information into a boolean property then a pattern like this can be used:

    mb.textProperty()
         .when(showingProperty)  // assumes showing property becomes false when focus is lost...
         .addListener(valueModel -> glassMenu.setTitle(parseText(mb)));

The showingProperty can be fed from the hide/show event handlers to set it to the right value, perhaps combined with focus information.

Dec 19 '23 09:12 hjohn

@hjohn You're absolutely right. It's another reason why I moved this PR to [WIP]. Instead of guessing and trying, I want to understand the deterministic path that leads to the issue with listeners being added and not removed. The one fixed in the first commit is clear, and well understood. Still, I saw uncollected listeners, with the GC root trace pointing to those other listeners (textProperty, mnemonicParsingProperty etc). Rather than trying to "fix" this with weak listeners, it is much better to find the real cause for this. In the process of finding out, I'm adding a few shim classes and unit tests so that we can clearly test for regression.

Dec 19 '23 10:12 johanvos

As for the memory leak issue: there were several. The first commit in this PR fixed a clear memory leak, but the one that is still left is not described in the issue. It occurs because whenever the SystemMenuBar is shown after it was not shown, all MenuItems will be readded using GlassSystemMenu.setMenu. This will invoke GlassSystemMenu.insertMenuItem(... MenuItemBase menuItem...) Inside this method, platform-specific menuItems will be created, that will adapt when the MenuItemBase properties are changing. The annoying thing, as stated in the first comment of that method, is that we don't know if the passed MenuItemBase instance was previously used in this method (in which case the listeners are regisered to this instance) or not. That is why for example the visibilityListener is unconditionally removed (even if we don't know if it was ever added) and then added. We can do the same for the other properties (preventing the use of weak listeners), but it would be nice if we could address this case using the api introduced in the deterministic listeners management (I would very much prefer that over weak listeners). I'm not too familiar with the new API though, but it seems difficult to do this inside this method, as we would still add a listener each time the method is invoked with an item that already has a listener registered?

Dec 19 '23 12:12 johanvos

As for the memory leak issue: there were several. The first commit in this PR fixed a clear memory leak, but the one that is still left is not described in the issue. It occurs because whenever the SystemMenuBar is shown after it was not shown, all MenuItems will be readded using GlassSystemMenu.setMenu. This will invoke GlassSystemMenu.insertMenuItem(... MenuItemBase menuItem...) Inside this method, platform-specific menuItems will be created, that will adapt when the MenuItemBase properties are changing. The annoying thing, as stated in the first comment of that method, is that we don't know if the passed MenuItemBase instance was previously used in this method (in which case the listeners are regisered to this instance) or not. That is why for example the visibilityListener is unconditionally removed (even if we don't know if it was ever added) and then added. We can do the same for the other properties (preventing the use of weak listeners), but it would be nice if we could address this case using the api introduced in the deterministic listeners management (I would very much prefer that over weak listeners). I'm not too familiar with the new API though, but it seems difficult to do this inside this method, as we would still add a listener each time the method is invoked with an item that already has a listener registered?

It's still a bit unclear for me. So what I write below may be off the mark.

From what I can see, setMenus will first remove all items, then re-add them. The clearMenu function seems like a logical place to remove listeners that may be present. The problem is where to keep track off the listeners. I see a few options:

Track the listeners in a few Maps in GlassSystemMenu, something like Map<MenuBase, InvalidationListener>, with a different Map for each listener type (text, disabled, mnemonic, etc). Or something more elaborate with a listener holder, nested maps, etc.

These maps can then iterated when setMenus is called to clear out ALL the listeners (or as part of clearMenu.

Similar to above but store the listener information in the properties of Menu
Using the new resource management system. Attach all listeners with a when condition based on a SimpleBooleanProperty that you replace each time setMenus is called (replacement is needed in this case as we don't know if the menus were used before or not, but it is still relatively clean).

The last one works like this:

Create a new non-final field: BooleanProperty active

In setMenus do:

  if (active != null) {
        active.set(false);  // this releases all listeners attached with `when(active)`
  }

  active = new SimpleBooleanProperty(true);  // replace active with a new property, as we can't reuse it

Then attach all listeners conditionally like so:

  mb.textProprty().when(active).addListener( ... );

The idea here is that because active is a different property each time setMenus is called, all old listeners will be removed when active goes to false, and will be eligible for GC because the active property was also replaced (and the old one, which we just set to false, is not referenced anywhere anymore).

Illustration:

Dashed lines are references that are removed, while normal arrows represent hard references.

I draw GlassMenu as something that may disappear in this illustration, but if it has a reference from elsewhere it can still be kept around (which is fine I think). More importantly is that GlassMenu has no reference back to the listener, so the listener (and all that it refers) can be GC'd.

Dec 19 '23 13:12 hjohn

Thanks @hjohn for the clear explanation. I replaced the Weak listeners with the new listener resource management approach, and that works fine.

I'm still looking into adding a test. The challenge is that the (previously) uncollected items are of type com.sun.glass.ui.MenuItem, and afaik we can't use JMemoryBuddy to check the collectable state of instances on the heap that we don't have a reference to ourselves. I see 2 options:

a unit test, with new shim classes so that we get access to com.sun.glass.ui.MenuItem from the test. The problem here is that this quickly becomes a test for the shim class and not for the real classes
a systemtest that can somehow inspect the heap and count the instances of com.sun.glass.ui.MenuItem

Dec 26 '23 10:12 johanvos

I'm still looking into adding a test. The challenge is that the (previously) uncollected items are of type com.sun.glass.ui.MenuItem, and afaik we can't use JMemoryBuddy to check the collectable state of instances on the heap that we don't have a reference to ourselves. I see 2 options:

a unit test, with new shim classes so that we get access to com.sun.glass.ui.MenuItem from the test. The problem here is that this quickly becomes a test for the shim class and not for the real classes

a systemtest that can somehow inspect the heap and count the instances of com.sun.glass.ui.MenuItem

Perhaps add a package private method to get access to GlassSystemMenu#systemMenus (or use reflection)? I think you could then put a weak reference on the returned list (or items) and see if that disappears.

Dec 26 '23 15:12 hjohn

I added a systemtest using a new Shim (GlassSystemMenuShim) that keeps track of the com.sun.glass.ui.Menu instances that are created in the GlassSystemMenu. I had to do a few more hacks since you don't normally access GlassSystemMenu yourself (the MenuBarSkin is doing most of the work). Note that this is very platform-dependent, as the creation of the com.sun.glass.ui.Menu is done by the com.sun.glass.ui.Application

Jan 09 '24 13:01 johanvos

A few things about the latest commit:

The usage of the active property caused regression: the memoryLeak test that was introduced in the fix for JDK-8318841 now failed. A number of listeners were hard referenced from the active property. The complexity is that there are different entry points by which a listener should be removed. What we try to do in this PR, is making sure we remove listeners after a defocus/focus operation that would otherwise stay referenced. A focus operation will result in GlassSystemMenu.setMenus() being called. I reverted some of the dependencies on the active property, and kept those that were directly created as a consequence of the setMenus call.
I added a test that demonstrates the issue when not removing menu's completely inside the listener, as reported by @jperedadnr and this issue is fixed as well now.
All tests now pass, but I noticed that in some cases, the systemtests do not correctly work with the application lifecycle management (see https://mail.openjdk.org/pipermail/openjfx-dev/2024-January/044516.html). For now, I consider this anomaly to be independent from JDK-8319779

Jan 11 '24 14:01 johanvos

@johanvos ticket We found a bug, which is also fixed by this PR. This is the ticket: https://bugs.openjdk.org/browse/JDK-8323787 It's an IndexOutOfBounds exception, related to the visible flag/listeners.

unittest We wrote a unit test, which goes green with this PR. Can you do us a favor, and add the test from the ticket to your PR? Then we can be sure, that this issue will be solved and never reappears.

Jan 16 '24 10:01 FlorianKirmaier

@FlorianKirmaier Thanks for the additional test. There was already a test in the SystemMenuBarTest, but it's really good to have an additional test for this -- I added it.

Jan 20 '24 20:01 johanvos

Thanks for the additional test. There was already a test in the SystemMenuBarTest, but it's really good to have an additional test for this -- I added it.

@johanvos 8323787 seems like a distinct bug, so I recommend adding it to this PR with /issue add 8323787.

Feb 02 '24 18:02 kevinrushforth

/issue add 8323787

Feb 24 '24 17:02 johanvos

@johanvos Adding additional issue to issue list: 8323787: Mac System MenuBar throws IOB exception.

Feb 24 '24 17:02 openjdk[bot]

Getting back to this.... I ran the test on macOS and it now works as expected. However, one of the new tests fails on Linux:

SystemMenuBarTest > testFocusMemoryLeak FAILED
    org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
        at app//org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:40)
        at app//org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:35)
        at app//org.junit.jupiter.api.Assertions.assertFalse(Assertions.java:227)
        at app//test.com.sun.javafx.tk.quantum.SystemMenuBarTest.testFocusMemoryLeak(SystemMenuBarTest.java:198)

Btw, all of my tests were done in a local branch with the latest upstream master merged in.

@johanvos since the source branch is now a bit out of date, can you merge in the latest upstream master?

Also, I think there are still some unaddressed questions from John and Andy.

Regarding the following:

All tests now pass, but I noticed that in some cases, the systemtests do not correctly work with the application lifecycle management (see https://mail.openjdk.org/pipermail/openjfx-dev/2024-January/044516.html). For now, I consider this anomaly to be independent from JDK-8319779

Yes, this is unrelated to the memory leak being fixed by this PR. We should file a bug for the activation problem.

Feb 26 '24 15:02 kevinrushforth

@kevinrushforth thanks for the feedback. I merged master and I see the failing test on Linux now as well. That is very interesting. I'll investigate and fix, and address the other comments as well. Also, I got some offline feedback about edge cases that I'm still trying to reproduce.

Feb 27 '24 09:02 johanvos

The failure on Linux has an interesting cause. The GlassSystemMenu (in com.sun.javafx.tk.quantum) has this code:

            /*
             * Leave the Apple menu in place
             */
            for (int index = existingSize - 1; index >= 0; index--) {
                Menu menu = existingMenus.get(index);
                clearMenu(menu);
                glassSystemMenuBar.remove(index);
            }

As a consequence, the first menu is not cleared -- regardless of the presence of an apple or not. There are plenty quick and dirty ways to fix this, but this platform-specific check probably does not belong in this package. @kevinrushforth what do you prefer to do in cases like these?

Mar 01 '24 14:03 johanvos

The failure on Linux has an interesting cause. The GlassSystemMenu (in com.sun.javafx.tk.quantum) has this code:
            /*
             * Leave the Apple menu in place
             */
            for (int index = existingSize - 1; index >= 0; index--) {
                Menu menu = existingMenus.get(index);
                clearMenu(menu);
                glassSystemMenuBar.remove(index);
            }
As a consequence, the first menu is not cleared -- regardless of the presence of an apple or not. There are plenty quick and dirty ways to fix this, but this platform-specific check probably does not belong in this package. @kevinrushforth what do you prefer to do in cases like these?

I missed responding to this. Presuming this really does need to be different on Mac than on other platforms, I guess the cleanest thing would be to move that logic to a method that can be overridden on Mac. If that's not feasible, then checking PlatformUtil.isMac() would be the other way.

One question, though: Why does this prevent the first menu from being cleared? Unless I'm missing something, doesn't that loop cover the whole range of menus? And since Linux doesn't even implement system menus, I wonder why this code is active for Linux at all?

Mar 05 '24 21:03 kevinrushforth

Answering my own questions...

The existing for loop quoted above actually loops while >= 1 rather than >= 0:

            for (int index = existingSize - 1; index >= 1; index--) {

so indeed the last menu would be held.

And I think that the answer to my other question (why is this even being used on Linux) is that the createAndRefocusMenuBarStage() method in the newly added test is calling GlassSystemMenuBar::createMenuBar unconditionally (via the Shim), when the actual logic in WindowStage qualifies by whether or not system menu is supported (which it only is for Mac).

So I think this is a test bug. I might recommend adding something like the following to the test:

assumeTrue(glassSystemMenu.isSupported());

Mar 05 '24 22:03 kevinrushforth

I think the logic in GlassSystemMenu should ultimately be platform-independent. Hence, moving the Apple-menu specific code to a Mac-specific class (in glass) sounds the right thing to do. However, it might be more pragmatic that we wait with this refactory until we have a second (or third) platform implementation. I can imagine there will need to be more changes if we deal with different linux window management systems. Hence, for now I agree having an assumeTrue(glassSystemMenu.isSupported()) is the easiest solution (and not an incorrect one).

Mar 06 '24 07:03 johanvos

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

8319779: SystemMenu: memory leak due to listener never being removed
8323787: Mac System MenuBar throws IOB exception

Reviewed-by: jhendrikx, arapte, jpereda

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

81f11c4a39eb505d17c57a49c5e084f75a169856: 8335630: Crash if Platform::exit called with fullScreen Stage on macOS 14
34bbf85362fae946c6306eb52a8478aa2ca3ef5f: 8328994: Update WebKit to 619.1
e3c15957488256ec53c5fb9978e336c59b69d65e: 8336272: SizeToSceneTest: fullScreen tests fail on Ubuntu 22.04
5b448b8bfddcfd062fb376132a48467e0d180a3c: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed
0ce4e6f9540178ddf4761de21a86c769d57497fa: 8335218: Eclipse Config: Remove Gradle Integration
1c6ef3638057e05565d83c79acbfad83046b12c7: 8335934: Change JavaFX release version to 24
a41dcf3cb7259af0b5feac404aa94c3c1b247460: 8336110: Update copyright header for files modified in 2024
e0fdb42b9fc0c09d8d7bf751f5adb9d8f07c0c4c: 8331603: Cleanup native AbstractSurface methods getRGBImpl, setRGBImpl
dbda2cce0352692c622e6cc1bc664d779c013fcb: 8295945: Revert unintended change to copyright start year
9723a9c4ce3a4490df962b83da0df71b1e5145f1: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty

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.

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

Mar 13 '24 20:03 openjdk[bot]

@johanvos this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8319779-systemmenu
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Apr 10 '24 21:04 openjdk[bot]

jfx jfx copied to clipboard

8319779: SystemMenu: memory leak due to listener never being removed

Progress

Issues

Reviewers

Reviewing

Webrev

Webrevs

jfx
jfx copied to clipboard