RFR: [WIP] 8319779: SystemMenu: memory leak due to listener never being removed [v4]

[John Hendrikx]

On Tue, 19 Dec 2023 12:24:14 GMT, Johan Vos <jvos at openjdk.org> wrote:

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

1. Track the listeners in a few `Map`s 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`.

2. Similar to above but store the listener information in the properties of `Menu`

3. 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:

![image](https://github.com/openjdk/jfx/assets/995917/d5b67994-cab0-4743-a2d8-68127d5223af)

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

PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-1862755206