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

[John Hendrikx]

On Wed, 10 Jan 2024 23:20:23 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> 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 l...

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

Yeah, the replacing of the property used in `when` is not a normal scenario.  Normally, the property used in the `when` goes out of scope at the same time as the rest of the references, in cases like:

       label.textProperty().when(userInterfaceIsShowing).addListener( ... );

The `userInterfaceIsShowing` is then part of a `Node` based container, or is not referenced anywhere at all (it's just derived from say `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::isShowingProperty)`.

It is important to realize that `when` uses two (normal) listeners:
- One on its condition property (`active`), which is not weak and is **never** removed (iirc, it can't be weak as otherwise the observable created by `when` could get GC'd when the condition is `false`, but shouldn't have been as the condition may yet become `true` again).
- One on its parent property (x in `x.when( ... )`) -- this listener (and thus its hard reference) is only present when the condition evaluates to `true`

In this specific case there is nothing that tells us that the menu disappeared, so the best we can do is that when a **new** menu is created that at that time we discard the old one.  To be able to discard it, we unfortunately need to keep a hard reference to the `active` property we used (so it can be set to `false`).  However, the `when` has a listener on that `active` property (as it needs to know when the condition changes), and a listener points back to the observable value that `when` created (it is not, and can't be, a weak listener).  So in order to also cut that hard reference, we need to replace the `active` property as well after setting it to `false` (if you don't set it to `false`, there will be a listener on the parent of `when` that keeps a hard reference still... setting it to `false` removes that listener first).

So definitely not your run-of-the-mill `when` example, but it will at least always behave deterministically as there are no weak listeners used anywhere.  If there is a bug, it will be easy to reproduce on someone else's machine.

The illustration I added earlier above should hopefully make the above a bit more clear, but of course some documentation in the code to explain this may be good too.

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

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