RFR: 6507038: Memory Leak in JTree / BasicTreeUI [v3]

[Alexey Ivanov]

On Wed, 24 Jan 2024 05:35:42 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> When using a TreeCellRenterer which creates new components in getTreeCellRendererComponent() in a JTree that is not visible, changes to the nodes cause a memory leak.
>> When a node is changed, the Method getNodeDimensions() is called to calculate the new dimensions for the node. In this method, getTreeCellRendererComponent() is called to obtain the renderer component (what else...) and [this component is added to rendererPane](https://github.com/openjdk/jdk/blob/36f4b34f1953af736706ec67192204727808bc6c/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java#L3283-L3284). It is not removed from the rendererPane afterwards. 
>> Only when the tree is painted, the paint() method does a removeAll on the rendererPane [in this code](https://github.com/openjdk/jdk/blob/36f4b34f1953af736706ec67192204727808bc6c/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java#L1500)
>> 
>> FIx is added to remove the components from rendererPane when the JTree UI is changed/uninstalled only when tree is not visible since they are already removed when tree is painted in paint() method..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Retain last component in rendererPane. Testcase added

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java line 3285:

> 3283:                     for (int i = 0; i < rendererPane.getComponentCount(); i++) {
> 3284:                         rendererPane.remove(i);
> 3285:                     }

rendererPane.removeAll()?
Suggestion:

                    rendererPane.removeAll();

test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 62:

> 60:         }
> 61: 
> 62:         public void finalize( ) {

The `finalize` method is deprecated. Can't we use `Reference` API for this purpose?

Store a list of [`PhantomReference<JLabel>`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/ref/PhantomReference.html). The size of the list is the number of `JLabel` objects created. Once you receive a reference via the [`ReferenceQueue`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/ref/ReferenceQueue.html), remove it from the list.

test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 90:

> 88:             }
> 89: 
> 90:             return label;

Here, before returning the `label` object, create a `PhantomReference` and add it to a list of references.

test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 101:

> 99:                 runChanges( );
> 100:             }
> 101:         });

This could be a method reference:
Suggestion:

        Thread updateThread = new Thread(this::runChanges);

test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 108:

> 106:                 runInfo( );
> 107:             }
> 108:         });

Suggestion:

        Thread infoThread = new Thread(this::runInfo);

test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 150:

> 148:             while (!done) {
> 149:                 Thread.sleep(100);
> 150:             }

Can't we use [`CountDownLatch`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/concurrent/CountDownLatch.html) instead of a boolean flag and busy-waiting?

test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 183:

> 181:             } catch (Exception ex) {
> 182:                 ex.printStackTrace();
> 183:             }

You may want to handle `InterruptedException` by exiting from the thread…

test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 191:

> 189:     public void runInfo( ) {
> 190:         long time = System.currentTimeMillis();
> 191:         long initialCnt = smCount;

Access to `smCount` or a list of references need to be synchronised.

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

PR Review: https://git.openjdk.org/jdk/pull/17458#pullrequestreview-1842292468
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1465491797
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1465498201
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1465504463
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1465505654
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1465506021
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1465499294
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1465507807
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1465501423