On Fri, 27 Jan 2023 10:51:00 GMT, John Hendrikx <[email protected]> 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
