RFR: 6507038: Memory Leak in JTree / BasicTreeUI

[Phil Race]

On Mon, 22 Jan 2024 06:29:40 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java line 1338:
>> 
>>> 1336:         if(rendererPane != null) {
>>> 1337:             if (!tree.isVisible()) {
>>> 1338:                 rendererPane.removeAll();
>> 
>> if( -> if (
>> 
>> Is it actually necessary to check visibility ? 
>> 
>> But more importantly, in the case of the test in the bug, how and when does this uninstallComponents() method get called ? It isn't clear to me how it changes the reported behaviour of that test.
>> 
>> I thought uninstallComponents() is just part of uninstallUI() like when tearing down and freeing the tree, or at least switching L&F .. whereas the submitter seems to be implying nodeChanged() is the problem
>
> Yes, the submitter mentioned during nodeChanged() but I mentioned it before [in this comment](https://github.com/openjdk/jdk/pull/17458#issuecomment-1897684665)
> that the comment present, implied it's not an issue, if the component is not removed if UI is not changed (as it is in the case when tree is not visible)...I am not sure why it was added and when but should we ignore that comment and try to remove the component even if tree is not visible (and UI is not changing)? 
> 
> I went with the thought that the coder meant what he said and I freed the components at the end (as you told during tearing down) so that there is no leak..

My point is not about whether this is the only case we need to address, I am asking what harm could come from removing the isVisible() test here ?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1464089825