On Mon, 29 Jan 2024 06:18:50 GMT, Prasanta Sadhukhan <psadhuk...@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
