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

[Johan Vos]

On Tue, 19 Dec 2023 13:25:38 GMT, John Hendrikx <jhendrikx 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?
>
>> 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 w
 e 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 ...

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`

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

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