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

[Prasanta Sadhukhan]

On Thu, 24 Aug 2023 22:30:04 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   javadoc, dispose
>
> 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.

@hjohn I guess i have updated as per suggestion..Let me know if this is ok as per you..if yes, please approve..

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

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