RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]

[Florian Kirmaier]

On Fri, 27 Jan 2023 10:51:00 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line 420:
>> 
>>> 418:                 new WeakHashMap<>();
>>> 419:         final Map<TKPulseListener,AccessControlContext> cleanupList =
>>> 420:                 new WeakHashMap<>();
>> 
>> I wonder why we need `WeakHashMap` here. These maps are short-lived anyway, since they're local variables.
>
> These should not be `WeakHashMap` at all; even if it is was necessary for some reason, there is no guarantee GC will run and potential keys that should have been removed may still be there in many situations. Using weak maps here only serves to make the process unpredictable, making it harder to find bugs if there ever is a situation where a key should have been removed.
> 
> I would be in favor of changing these to normal maps.

I've changed it now to a normal HashMap.

>> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line 494:
>> 
>>> 492:     public void addCleanupListener(TKPulseListener listener) {
>>> 493:         AccessControlContext acc = AccessController.getContext();
>>> 494:         cleanupListeners.put(listener,acc);
>> 
>> Access to `cleanupListeners` is not synchronized on `this` here, but it is synchronized in L426.
>> You should either synchronize each and every access, or none of it.
>
> It should not be removed everywhere, it has to be synchronized (all the other listener lists are as well). This is probably because listeners can be added on different threads.

I've added a synchronized now.

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

PR: https://git.openjdk.org/jfx/pull/584