RFR: 8262518: SwingNode.setContent does not close previous content, resulting in memory leak [v4]

[John Hendrikx]

On Thu, 24 Aug 2023 03:17:03 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Issue is when setting the content of a SwingNode, the old content is not garbage collected owing to the fact
>> JLightweightFrame is never being released by SwingNodeDisposer
>> 
>> The SwingNodeDisposer holds an hard pointer to the JLightweightFrame that prevents its collection
>> 
>> Modified `SwingNode.setContentImpl`  function to use a WeakReference to properly release the memory.
>
> Prasanta Sadhukhan has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - javadoc, null check
>  - javadoc, null check

modules/javafx.swing/src/main/java/com/sun/javafx/embed/swing/Disposer.java line 95:

> 93:      * Remove the DisposerRecord object to prevent memory leak
> 94:      * @param ref Weak reference of the object to be removed
> 95:      */

Looks good, however I think you could say this a bit more general, no need to mention that it is to prevent memory leaks -- it's just normal that you should remove the record if there is no need for that specific dispose task anymore because it was already disposed in another way.

So how about:

Unregisters a previously registered {@link DisposerRecord}, removing it from the list of records to dispose off when the original target goes out of scope. This is useful in cases where the original target is still in use, but the record was already disposed off by other means.

modules/javafx.swing/src/main/java/javafx/embed/swing/SwingNode.java line 377:

> 375:                 Disposer.removeRecord(disposerRecRef);
> 376:                 disposerRecRef = null;
> 377:             }

I think this should be part of the first `if`, or an `if` by itself, and not here.  `lwFrame`, `rec` and `disposerRecRef` will all either be `null` or be set to some value.  The only case this may not happen is that after `lwFrame` is set, an exception occurs.  You could make it very safe, or just assume that when `lwFrame` is not `null` that all clean up actions should be taken -- if an exception did occur during set-up, then it's also fine that one can occur during clean-up, no need to make it more robust for situations that can't happen.

There really is no need to start second guessing yourself when the logic is solid.  Either all those variables are non-`null` and checking one of them is sufficient, or they are all `null` and checking each in turn is pointless (it is just confusing because programmers may assume now that is a situation that can actually occur).

Please revert to the old code.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1219#discussion_r1304931601
PR Review Comment: https://git.openjdk.org/jfx/pull/1219#discussion_r1304935011