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

[John Hendrikx]

On Tue, 5 Mar 2024 08:55:41 GMT, Jose Pereda <jpereda at openjdk.org> wrote:

>> This could be a problem normally, but I think in this case you won't be able to get this to produce incorrect results.
>> 
>> `parseText` is accessing the property (and it calls `isMnemonicParsing`), but only if the text is not empty.  It's sort of "safe" as an empty text can't contain a mnemonic anyway, and you'd need to change the text at a minimum first to see the effect of turning mnemonic parsing on/off.  Changing the text would trigger the `textProperty` listener, which will call `parseText` and read the mnemonic property.
>> 
>> A change listener would be more obviously correct, but it is not strictly needed here.
>> 
>> (not all `subscribe`s use change listener, the one that takes a `Runnable` uses invalidation)
>
> @hjohn There is an important issue using `when()`, because the listener is added to a new property, not to the original property. 
> 
> Therefore, using an invalidation listener doesn't trigger any more notifications if you are changing values more than once to the original property.
> 
> This can happen in this SystemMenuBar case with the disabled property of any given `Menu` or `MenuItem`, if you simply change it twice or more times, while the application is active. You will notice that it will remain always with the same first state.
>  
> It is hard to add a system test in this case because the public Menu/MenuItem and the private peers MenuBase/MenuBaseItem are in sync, and only the communication to the native part is missing. But it can be easily viewed if you run it manually.
> 
> Let me clarify it with a very simple test, that reproduces the case of the disabledProperty:
> 
> mb.disableProperty().when(active).addListener(valueModel -> glassMenu.setEnabled(!mb.isDisable()));
> 
> 
> Simply run this manually:
> 
>         BooleanProperty b = new SimpleBooleanProperty();
>         BooleanProperty active = new SimpleBooleanProperty(true);
> 
>         b.addListener(o -> System.out.println("b: " + b.get()));
>         b.when(active).addListener(o -> System.out.println("when: " + b.get()));
>         b.set(true);
>         b.set(false);
> 
> 
> while you probably would be expecting:
> 
> 
> b: true
> when: true
> b: false
> when: false
> 
> 
> the test prints:
> 
> 
> b: true
> when: true
> b: false
> 
> 
> so using `when()` we miss the changes after the first invalidation.
> 
> I could go into details of why this happens, but just notice that we have a _new property_ after all, and if you are not calling `when.getValue()`, it doesn't get validated anymore after the first change.
> 
> I see three possible fixes to this issue.
> 
> 1. If you change the sample to:
> 
>         BooleanProperty b = new SimpleBooleanProperty();
>         BooleanProperty active = new SimpleBooleanProperty(true);
> 
>         b.addListener(o -> System.out.println("b: " + b.get()));
>         ObservableValue<Boolean> c = b.when(active);
>         c.addListener(o -> System.out.println("when: " + c.getValue()));
>         b.set(true);
>         b.set(false);
> 
> then you get the expected result:
> 
> 
> b: true
> when: true
> b: false
> when: false
> 
> 
> 2. Since this change implies some more refactoring, that is why I suggested using the change listener instead:
> 
> 
>         BooleanProperty b = new SimpleBooleanProperty();
>         BooleanProperty active = new SimpleBooleanProperty(true);
> 
>         b.addListener(...

Thanks, you are right.  The property generated by `when` is not getting revalidated, because its specific `getValue` is never called when using an invalidation listener on it. This seems to be a bit of pitfall that applies to all such mapped properties (not exclusive to `when`).

I'm fine with any of the options.  Option 3 certainly is the most convenient.

I'm more wondering if there should be some action taken in for the `map`, `flatMap`, `when`, `orElse` constructs to make them work better with invalidation listeners, or if that's not possible, a note explaining that adding an invalidation listener to such a construct probably is not doing what you think it is doing.

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

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