On Thu, 26 Jan 2023 18:04:41 GMT, Michael Strauß <mstra...@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
