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

[John Hendrikx]

On Thu, 26 Jan 2023 18:04:41 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Florian Kirmaier has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
>> 
>>  - JDK-8269907
>>    Added missing changes after merge
>>  - Merge remote-tracking branch 'origjfx/master' into JDK-8269907-dirty-and-removed
>>    
>>    # Conflicts:
>>    #	modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
>>    #	modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>>  - Merge remote-tracking branch 'origin/master'
>>  - JDK-8269907
>>    Removed the sync methods for the scene, because they don't work when peer is null, and they are not necessary.
>>  - JDK-8269907
>>    Fixed rare bug, causing bounds to be out of sync.
>>  - JDK-8269907
>>    We now require the rendering lock when cleaning up dirty nodes. To do so, we moved some code required for snapshot into a reusable method.
>>  - JDK-8269907
>>    The bug is now fixed in a new way. Toolkit now supports registering CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks.
>>  - JDK-8269907
>>    Fixing dirty nodes and parent removed, when a window is no longer showing.  This typically happens with context menus.
>
> 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.

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

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

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