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

[John Hendrikx]

On Mon, 4 Mar 2024 19:26:37 GMT, Jose Pereda <jpereda at openjdk.org> wrote:

>> Johan Vos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into 8319779-systemmenu
>>  - Add additional test for IOOBE detection.
>>    This test comes from JDK-8323787
>>  - Revert some of the conditional bindings.
>>    Clear menu construction when an menuitem that is a menu needs to be removed
>>    Add a test for the latter
>>  - Merge remote-tracking branch 'upstream/master' into 8319779-systemmenu
>>  - Cleanup test
>>  - Add shim class so that we can access the references to com.sun.glass.ui.Menu instances.
>>    Add a test to make sure those references are gone.
>>  - Revert WeakInvalidationListeners and use new listener resource management approach.
>>  - Fix more memoryleaks due to listeners never being unregistered.
>>  - These changes are related to JBS-8318841 so we want to have that code in
>>    as well.
>>    
>>    Merge branch 'master' into 8319779-systemmenu
>>  - process reviewers comments
>>  - ... and 1 more: https://git.openjdk.org/jfx/compare/f65f11d2...ec7308df
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java line 226:
> 
>> 224:         mb.textProperty().when(active).addListener(valueModel -> glassMenu.setTitle(parseText(mb)));
>> 225:         mb.disableProperty().when(active).addListener(valueModel -> glassMenu.setEnabled(!mb.isDisable()));
>> 226:         mb.mnemonicParsingProperty().when(active).addListener(valueModel -> glassMenu.setTitle(parseText(mb)));
> 
> Since you are not calling `get()` from the `when` resulting observable (to allow for invalidation), these need to use a `ChangeListener` instead of an `InvalidationListener`, otherwise, while `active` doesn't change, any change in those menuBase properties will trigger just one notification. 
> 
> Alternative, just replace `addListener` with  `subscribe` (as it internally uses a `ChangeListener`).
> 
> @hjohn does this make sense to you?

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)

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

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