From d9a82d103d60bc38b35ba94a21354c84a75bb22d Mon Sep 17 00:00:00 2001 From: Lukasz Kostyra Date: Mon, 8 May 2023 15:23:03 +0000 Subject: [PATCH] 8296919: Make system tests that detect memory leaks more robust by using JMemoryBuddy utility Reviewed-by: angorya, kcr --- build.gradle | 3 +- .../java/test/util/memory/JMemoryBuddy.java | 33 +++++++++++ modules/javafx.web/.classpath | 2 +- .../java/test/javafx/scene/web/LeakTest.java | 55 +++---------------- .../swing/SwingNodeDnDMemoryLeakTest.java | 32 ++++------- .../embed/swing/SwingNodeMemoryLeakTest.java | 49 ++++------------- .../control/AccordionTitlePaneLeakTest.java | 28 +++++----- .../scene/control/TabPaneHeaderLeakTest.java | 18 ++---- .../scene/shape/ShapeViewOrderLeakTest.java | 15 ++--- .../javafx/stage/FocusedWindowTestBase.java | 19 +------ .../test/robot/javafx/web/TooltipFXTest.java | 10 +--- 11 files changed, 94 insertions(+), 170 deletions(-) diff --git a/build.gradle b/build.gradle index b5edd25301b..2e2d7310076 100644 --- a/build.gradle +++ b/build.gradle @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2013, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -3488,6 +3488,7 @@ project(":web") { implementation project(":graphics") implementation project(":controls") implementation project(":media") + testImplementation project(":base").sourceSets.test.output } compileJava.dependsOn updateCacheIfNeeded diff --git a/modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java b/modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java index 51ecf377fc5..d639d35d6eb 100644 --- a/modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java +++ b/modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java @@ -32,6 +32,7 @@ import java.lang.management.ManagementFactory; import java.lang.ref.WeakReference; import java.text.SimpleDateFormat; +import java.util.Collection; import java.util.Date; import java.util.LinkedList; import java.util.Objects; @@ -91,6 +92,38 @@ public static void assertCollectable(WeakReference weakReference) { } } + /** + * Checks whether the content of all references from a WeakReference array can be collected. + * @param weakReferences WeakReference objects to check. + */ + public static void assertCollectable(WeakReference[] weakReferences) { + for (WeakReference wr : weakReferences) { + assertCollectable(wr); + } + } + + /** + * Checks whether the content of all references from a WeakReference Collection can be collected. + * @param weakReferences WeakReference objects to check. + */ + public static void assertCollectable(Collection> weakReferences) { + for (WeakReference wr : weakReferences) { + assertCollectable(wr); + } + } + + /** + * Checks whether the content of provided WeakReference objects can be collected. + * @param weakReference The WeakReference to check. + * @param otherWeakReferences Other WeakReference objects to check. + */ + public static void assertCollectable(WeakReference weakReference, WeakReference... otherWeakReferences) { + assertCollectable(weakReference); + for (WeakReference wr : otherWeakReferences) { + assertCollectable(wr); + } + } + /** * Checks whether the content of the WeakReference can be collected. * @param weakReference The WeakReference to check. diff --git a/modules/javafx.web/.classpath b/modules/javafx.web/.classpath index 8bf4e21e304..ceb9d72e4c7 100644 --- a/modules/javafx.web/.classpath +++ b/modules/javafx.web/.classpath @@ -42,7 +42,7 @@ - + diff --git a/modules/javafx.web/src/test/java/test/javafx/scene/web/LeakTest.java b/modules/javafx.web/src/test/java/test/javafx/scene/web/LeakTest.java index 59be57eff6d..2312f9a68bf 100644 --- a/modules/javafx.web/src/test/java/test/javafx/scene/web/LeakTest.java +++ b/modules/javafx.web/src/test/java/test/javafx/scene/web/LeakTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -47,6 +47,7 @@ import org.w3c.dom.Element; import org.w3c.dom.NodeList; import static org.junit.Assert.*; +import test.util.memory.JMemoryBuddy; public class LeakTest extends TestBase { @@ -79,7 +80,7 @@ public class LeakTest extends TestBase { @Test public void testGarbageCollectability() throws InterruptedException { final int count = 3; - Reference[] willGC = new Reference[count]; + WeakReference[] willGC = new WeakReference[count]; submit(() -> { WebView webView = new WebView(); @@ -88,21 +89,7 @@ public class LeakTest extends TestBase { willGC[2] = new WeakReference<>(WebEngineShim.getPage(webView.getEngine())); }); - Thread.sleep(SLEEP_TIME); - - for (int i = 0; i < 5; i++) { - System.gc(); - - if (isAllElementsNull(willGC)) { - break; - } - - Thread.sleep(SLEEP_TIME); - } - - assertNull("WebView has not been GCed", willGC[0].get()); - assertNull("WebEngine has not been GCed", willGC[1].get()); - assertNull("WebPage has not been GCed", willGC[2].get()); + JMemoryBuddy.assertCollectable(willGC); } private static boolean isAllElementsNull(Reference[] array) { @@ -116,7 +103,7 @@ private static boolean isAllElementsNull(Reference[] array) { @Test public void testJSObjectGarbageCollectability() throws InterruptedException { final int count = 10000; - Reference[] willGC = new Reference[count]; + WeakReference[] willGC = new WeakReference[count]; submit(() -> { for (int i = 0; i < count; i++) { @@ -125,19 +112,7 @@ private static boolean isAllElementsNull(Reference[] array) { } }); - Thread.sleep(SLEEP_TIME); - - for (int i = 0; i < 5; i++) { - System.gc(); - - if (isAllElementsNull(willGC)) { - break; - } - - Thread.sleep(SLEEP_TIME); - } - - assertTrue("All JSObjects are GC'ed", isAllElementsNull(willGC)); + JMemoryBuddy.assertCollectable(willGC); } // JDK-8170938 @@ -177,7 +152,7 @@ private State getLoadState() { // JDK-8176729 @Test public void testDOMNodeDisposeCount() throws InterruptedException { int count = 7; - Reference[] willGC = new Reference[count]; + WeakReference[] willGC = new WeakReference[count]; final String html = "\n" + "\n" + @@ -236,20 +211,6 @@ private State getLoadState() { assertEquals("Expected NodeImpl(tag:br) HashCount", initialHashCount+7, NodeImplShim.test_getHashCount()); }); - Thread.sleep(SLEEP_TIME); - - for (int i = 0; i < 5; i++) { - System.gc(); - - if (isAllElementsNull(willGC)) { - break; - } - - Thread.sleep(SLEEP_TIME); - } - - // Give disposer a chance to run - Thread.sleep(SLEEP_TIME); - assertEquals("NodeImpl HashCount after dispose", initialHashCount, NodeImplShim.test_getHashCount()); + JMemoryBuddy.assertCollectable(willGC); } } diff --git a/tests/system/src/test/java/test/javafx/embed/swing/SwingNodeDnDMemoryLeakTest.java b/tests/system/src/test/java/test/javafx/embed/swing/SwingNodeDnDMemoryLeakTest.java index 251ba233c94..20bd2fd4088 100644 --- a/tests/system/src/test/java/test/javafx/embed/swing/SwingNodeDnDMemoryLeakTest.java +++ b/tests/system/src/test/java/test/javafx/embed/swing/SwingNodeDnDMemoryLeakTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -33,6 +33,9 @@ import java.util.concurrent.CountDownLatch; import javax.swing.JLabel; +import javax.swing.SwingUtilities; + +import java.lang.reflect.InvocationTargetException; import javafx.application.Application; import javafx.embed.swing.SwingNode; @@ -45,12 +48,12 @@ import org.junit.Test; import test.util.Util; +import test.util.memory.JMemoryBuddy; public class SwingNodeDnDMemoryLeakTest { final static int TOTAL_SWINGNODE = 10; static CountDownLatch launchLatch = new CountDownLatch(1); - final static int GC_ATTEMPTS = 10; ArrayList> weakRefArrSN = new ArrayList(TOTAL_SWINGNODE); @@ -65,12 +68,16 @@ public static void teardownOnce() { } @Test - public void testSwingNodeMemoryLeak() { + public void testSwingNodeMemoryLeak() throws InvocationTargetException, InterruptedException { Util.runAndWait(() -> { testSwingNodeObjectsInStage(); }); - attemptGCSwingNode(); - assertEquals(TOTAL_SWINGNODE, getCleanedUpSwingNodeCount()); + + // Invoke a noop on EDT thread and wait for a bit to make sure EDT processed node objects + SwingUtilities.invokeAndWait(() -> {}); + Util.sleep(500); + + JMemoryBuddy.assertCollectable(weakRefArrSN); } private void testSwingNodeObjectsInStage() { @@ -112,21 +119,6 @@ private void testSwingNodeObjectsInStage() { } } - private void attemptGCSwingNode() { - // Attempt gc GC_ATTEMPTS times - for (int i = 0; i < GC_ATTEMPTS; i++) { - System.gc(); - if (getCleanedUpSwingNodeCount() == TOTAL_SWINGNODE) { - break; - } - try { - Thread.sleep(500); - } catch (InterruptedException e) { - System.err.println("InterruptedException occurred during Thread.sleep()"); - } - } - } - private int getCleanedUpSwingNodeCount() { int count = 0; for (WeakReference ref : weakRefArrSN) { diff --git a/tests/system/src/test/java/test/javafx/embed/swing/SwingNodeMemoryLeakTest.java b/tests/system/src/test/java/test/javafx/embed/swing/SwingNodeMemoryLeakTest.java index a1c3f47d0c8..0363565def2 100644 --- a/tests/system/src/test/java/test/javafx/embed/swing/SwingNodeMemoryLeakTest.java +++ b/tests/system/src/test/java/test/javafx/embed/swing/SwingNodeMemoryLeakTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -29,10 +29,12 @@ import static org.junit.Assume.assumeTrue; import java.lang.ref.WeakReference; +import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.concurrent.CountDownLatch; import javax.swing.JLabel; +import javax.swing.SwingUtilities; import javafx.application.Application; import javafx.embed.swing.SwingNode; @@ -47,6 +49,7 @@ import com.sun.javafx.PlatformUtil; import test.util.Util; +import test.util.memory.JMemoryBuddy; public class SwingNodeMemoryLeakTest { @@ -69,18 +72,17 @@ public static void teardownOnce() { } @Test - public void testSwingNodeMemoryLeak() { - if (PlatformUtil.isMac()) { - assumeTrue(Boolean.getBoolean("unstable.test")); // JDK-8196614 - } + public void testSwingNodeMemoryLeak() throws InterruptedException, InvocationTargetException { Util.runAndWait(() -> { testSwingNodeObjectsInStage(); }); - attemptGCSwingNode(); - assertEquals(TOTAL_SWINGNODE, getCleanedUpSwingNodeCount()); - attemptGCJLabel(); - assertEquals(TOTAL_SWINGNODE, getCleanedUpJLabelCount()); + // Invoke a noop on EDT thread and wait for a bit to make sure EDT processed node objects + SwingUtilities.invokeAndWait(() -> {}); + Util.sleep(500); + + JMemoryBuddy.assertCollectable(weakRefArrSN); + JMemoryBuddy.assertCollectable(weakRefArrJL); } private void testSwingNodeObjectsInStage() { @@ -120,35 +122,6 @@ private void testSwingNodeObjectsInStage() { } } - private void attemptGCSwingNode() { - // Attempt gc GC_ATTEMPTS times - for (int i = 0; i < GC_ATTEMPTS; i++) { - System.gc(); - if (getCleanedUpSwingNodeCount() == TOTAL_SWINGNODE) { - break; - } - try { - Thread.sleep(250); - } catch (InterruptedException e) { - System.err.println("InterruptedException occurred during Thread.sleep()"); - } - } - } - - private void attemptGCJLabel() { - // Attempt gc GC_ATTEMPTS times - for (int i = 0; i < GC_ATTEMPTS; i++) { - System.gc(); - if (getCleanedUpJLabelCount() == TOTAL_SWINGNODE) { - break; - } - try { - Thread.sleep(250); - } catch (InterruptedException e) { - System.err.println("InterruptedException occurred during Thread.sleep()"); - } - } - } private int getCleanedUpSwingNodeCount() { int count = 0; for (WeakReference ref : weakRefArrSN) { diff --git a/tests/system/src/test/java/test/javafx/scene/control/AccordionTitlePaneLeakTest.java b/tests/system/src/test/java/test/javafx/scene/control/AccordionTitlePaneLeakTest.java index 0337243cfa4..bef83b74814 100644 --- a/tests/system/src/test/java/test/javafx/scene/control/AccordionTitlePaneLeakTest.java +++ b/tests/system/src/test/java/test/javafx/scene/control/AccordionTitlePaneLeakTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -42,6 +42,7 @@ import org.junit.Test; import test.util.Util; +import test.util.memory.JMemoryBuddy; public class AccordionTitlePaneLeakTest { @@ -50,6 +51,8 @@ public class AccordionTitlePaneLeakTest { static private StackPane root; static private Stage stage; + private WeakReference weakRefToPane; + public static class TestApp extends Application { @Override public void start(Stage primaryStage) throws Exception { @@ -77,19 +80,14 @@ public static void teardownOnce() { @Test public void testForTitledPaneLeak() throws Exception { - TitledPane pane = new TitledPane(); - accordion.getPanes().add(pane); - WeakReference weakRefToPane = new WeakReference<>(pane); - pane = null; - accordion.getPanes().clear(); - for (int i = 0; i < 10; i++) { - System.gc(); - if (weakRefToPane.get() == null) { - break; - } - Util.sleep(500); - } - // Ensure accordion's skin no longer hold a ref to titled pane. - Assert.assertNull("Couldn't collect TitledPane", weakRefToPane.get()); + Util.runAndWait(() -> { + TitledPane pane = new TitledPane(); + accordion.getPanes().add(pane); + weakRefToPane = new WeakReference<>(pane); + pane = null; + accordion.getPanes().clear(); + }); + + JMemoryBuddy.assertCollectable(weakRefToPane); } } diff --git a/tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java b/tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java index 21c359f9eba..b107b62dd6e 100644 --- a/tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java +++ b/tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2020, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -41,8 +41,9 @@ import org.junit.BeforeClass; import org.junit.Test; -import junit.framework.Assert; import test.util.Util; +import test.util.memory.JMemoryBuddy; + public class TabPaneHeaderLeakTest { @@ -92,16 +93,7 @@ public void testTabPaneHeaderLeak() throws Exception { tabPane.getTabs().clear(); root.getChildren().clear(); }); - for (int i = 0; i < 10; i++) { - System.gc(); - if (tabWeakRef.get() == null && - textFieldWeakRef.get() == null) { - break; - } - Util.sleep(500); - } - // Ensure that Tab and TextField are GCed. - Assert.assertNull("Couldn't collect Tab", tabWeakRef.get()); - Assert.assertNull("Couldn't collect TextField", textFieldWeakRef.get()); + + JMemoryBuddy.assertCollectable(tabWeakRef, textFieldWeakRef); } } diff --git a/tests/system/src/test/java/test/javafx/scene/shape/ShapeViewOrderLeakTest.java b/tests/system/src/test/java/test/javafx/scene/shape/ShapeViewOrderLeakTest.java index a17ab676a93..0718b6b2008 100644 --- a/tests/system/src/test/java/test/javafx/scene/shape/ShapeViewOrderLeakTest.java +++ b/tests/system/src/test/java/test/javafx/scene/shape/ShapeViewOrderLeakTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2020, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -42,8 +42,8 @@ import org.junit.BeforeClass; import org.junit.Test; -import junit.framework.Assert; import test.util.Util; +import test.util.memory.JMemoryBuddy; public class ShapeViewOrderLeakTest { @@ -94,14 +94,7 @@ public void testShapeViewOrderLeak() throws Exception { group.getChildren().clear(); root.getChildren().clear(); }); - for (int i = 0; i < 10; i++) { - System.gc(); - if (shapeWeakRef.get() == null) { - break; - } - Util.sleep(500); - } - // Ensure that Shape is GCed. - Assert.assertNull("Couldn't collect Shape", shapeWeakRef.get()); + + JMemoryBuddy.assertCollectable(shapeWeakRef); } } diff --git a/tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java b/tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java index a682d3e13f9..a0846bcb23b 100644 --- a/tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java +++ b/tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2020, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -37,6 +37,7 @@ import org.junit.Assert; import test.util.Util; +import test.util.memory.JMemoryBuddy; public abstract class FocusedWindowTestBase { @@ -76,20 +77,6 @@ public void testClosedFocusedStageLeakBase() throws Exception { closedFocusedStage.requestFocus(); closedFocusedStage = null; - assertCollectable(closedFocusedStageWeak); - } - - public static void assertCollectable(WeakReference weakReference) throws Exception { - int counter = 0; - - System.gc(); - - while (counter < 10 && weakReference.get() != null) { - Thread.sleep(100); - counter = counter + 1; - System.gc(); - } - - Assert.assertNull(weakReference.get()); + JMemoryBuddy.assertCollectable(closedFocusedStageWeak); } } diff --git a/tests/system/src/test/java/test/robot/javafx/web/TooltipFXTest.java b/tests/system/src/test/java/test/robot/javafx/web/TooltipFXTest.java index 55e4d1e50fb..05e620ca34e 100644 --- a/tests/system/src/test/java/test/robot/javafx/web/TooltipFXTest.java +++ b/tests/system/src/test/java/test/robot/javafx/web/TooltipFXTest.java @@ -49,6 +49,7 @@ import org.junit.Test; import test.util.Util; +import test.util.memory.JMemoryBuddy; public class TooltipFXTest { @@ -164,13 +165,6 @@ public static void teardown() { scene = null; }); - for (int j = 0; j < 5; ++j) { - System.gc(); - if (webViewRef.get() == null) { - break; - } - Util.sleep(SLEEP_TIME); - } - assertNull("webViewRef is not null", webViewRef.get()); + JMemoryBuddy.assertCollectable(webViewRef); } }