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

[Alexey Ivanov]

On Mon, 29 Jan 2024 06:18:50 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:
> 
>   Use removeAll and testcase modified

Changes requested by aivanov (Reviewer).

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

> 1: /*

Update the copyright year in `BasicTreeUI`.

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

> 49: public class TreeCellRendererLeakTest {
> 50: 
> 51:     static int smCount = 0;

What does `sm-` prefix mean?

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

> 58:     private JTree jTree1;
> 59: 
> 60:     static CountDownLatch testDone;

Suggestion:

    static final CountDownLatch testDone = new CountDownLatch(1);

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

> 60:     static CountDownLatch testDone;
> 61: 
> 62:     ArrayList<WeakReference<JLabel>> weakRefArrLabel = new ArrayList(50);

Suggestion:

    final List<WeakReference<JLabel>> weakRefArrLabel = new ArrayList<>(50);

The type could be `List`.  Use the diamond operator on the right side.  
I suggest a simpler name: `labelList`, `labelRefList`, _`weakRefList`_.

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

> 84:             }
> 85: 
> 86:             weakRefArrLabel.add(smCount++, new WeakReference<JLabel>(label));

Suggestion:

            synchronized (weakRefArrLabel) {
                weakRefArrLabel.add(new WeakReference<JLabel>(label));
            }

You have to add items in a synchronized block; you read from the list in another thread. (Reading has be done inside a synchronized block too.)

Just add to the end of the list.

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

> 134:             testDone = new CountDownLatch(1);
> 135:             SwingUtilities.invokeAndWait(() -> {
> 136:                 TreeCellRendererLeakTest tf = new TreeCellRendererLeakTest();

Suggestion:

                new TreeCellRendererLeakTest();

The variable is unused and can safely be removed. The side effects of calling the constructor won't disappear.

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

> 162:                         model.nodeChanged(n);
> 163:                     }
> 164:                 });

The anonymous class can be a lambda expression.

The code inside the `Runnable` can be reduced to


n.setUserObject("runcount " + currentCount);
model.nodeChanged(n);


This implies, you make `n` a field and initialise it in `initComponents`; or after calling `initComponents`.

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

> 179:     public void runInfo() {
> 180:         long time = System.currentTimeMillis();
> 181:         long initialCnt = smCount;

`initialCnt` is unused in the current version.

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

> 182:         while ((System.currentTimeMillis() - time) < (15 * 1000)) {
> 183:             System.gc();
> 184:             System.out.println("Live JLabels:" + smCount);

Access to `smCount` has to be synchronized between the two threads.

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

> 202:                 count++;
> 203:             }
> 204:         }

You have to iterate over `weakRefArrLabel` in a synchronized block. (No, declaring `weakRefArrLabel` as `volatile` is not enough.)
Suggestion:

        synchronized (weakRefArrLabel) {
            for (WeakReference<JLabel> ref : weakRefArrLabel) {
                if (ref.get() == null) {
                    count++;
                }
            }
        }

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

PR Review: https://git.openjdk.org/jdk/pull/17458#pullrequestreview-1851588145
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471471871
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471467202
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471426426
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471432033
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471436951
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471438887
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471446967
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471468908
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471470295
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471458749