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

[John Hendrikx]

On Wed, 10 Jan 2024 20:05:16 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> No, the explicit goal of this construction is to set active to false (in case it exists) so that existing listeners can be released; followed by creating a new active property that is by default set to true until `setMenus` is called again (e.g. after unfocus/focus events).
>> I noticed however that it is not as simple as that, and the current PR introduces regression, as the `SystemMenuBarTest.testMemoryLeak` test is now failing. Listeners are not only registered when `setMenus` is called, but also when menu(Item)s are added to menus. I'm not sure if a single `active` property can address this, as we don't want to remove all listeners for all menuitems in case a particular menuitem is added to a particular menu.
>> @hjohn does that make sense?
>
> thank you for clarification!  
> 
> perhaps we ought to add a comment explaining why: it's one thing to create a single chain of conditions linked to a property, but here we keep creating new property instance, so the side effects are not that obvious.

> I noticed however that it is not as simple as that, and the current PR introduces regression, as the SystemMenuBarTest.testMemoryLeak test is now failing. Listeners are not only registered when setMenus is called, but also when menu(Item)s are added to menus. I'm not sure if a single active property can address this, as we don't want to remove all listeners for all menuitems in case a particular menuitem is added to a particular menu.
@hjohn does that make sense?

So, if I understood correctly, that test is dealing not with the entire menu bar disappearing (for which the `active` property ensures clean up), but with a submenu getting cleared, and then a new item added.  Any old items that were added should be garbage collectable after clearing.

This is a bit tricky.  Basically I think it means that the listeners on a certain `MenuBase` wrapper should be removed in two cases:

1. When the entire menu is no longer needed (what we have now)
2. When the `MenuBase` in question is removed from its parent

I think this may be resolved in one of two ways:

1. Create a specific active condition for `when` that includes a check on parent and on the top level `active` property
2. Gather all listeners added into a single `Subscription` (currently the `Subscription` that is returned is discarded, track this somewhere (probably easiest in a the user property map) taking care not to create a new hard reference (not sure if that will be an issue).  The `Subscription` can then be obtained and unsubscribed early.

Now I think option 1 can be achieved by doing something like the code below.

**Note:** completely untested, and did not check yet if we aren't creating any new problematic hard references; I can test this out over the weekend if you like.

     // given the global `active` property...
     // and a MenuBase mb, for which we can discover if it is a root menu or not...
     // and which exposes a new property hasParentProperty (you can derive this property
     // if you like by doing `parentMenuProperty.map(Objects::nonNull)`)

     BooleanProperty forWhen = active;

     if (mb "is not a root menu") {  // important we don't do this for root
           // include "and" condition that it also must have a parent to be considered "active"
           forWen = active.flatMap(b -> b ? mb.hasParentProperty() : active);
     }

     // then use `forWhen` in your `when` conditions instead of `active` as before.

What the above does is that using `flatMap` it creates a listener on `hasParentProperty`.  If it changes, the entire expression result may change.  In normal Java code it would simply be:

     boolean forWhen = active && mb.hasParent();

... except of course that the above code would not react to changes.

The logical and (`&&`) is achieved with the ternary expression.  If `b` is `true` then we need to check the second part of the `&&` which is `hasParentProperty`.  If `b` was already `false`, then the there is no need to check further.  In any case, a new **property** must be returned from `flatMap`, so that will either be `hasParentProperty` in case `active` is `true` or the `active` property (which then must be `false`).

Such scenario's are rare, but I've been considering adding specific support on `ObservableValue` for this.

There is also the `and` function on `ObservableBooleanValue` -- you can test this one as well, but I fear that it may be using a weak listener in there and that it may get collected early if you don't reference the intermediate expressions.  There is no such risk with `flatMap`.

I haven't looked further into option 2.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1448099725