Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8296919: Make system tests that detect memory leaks more robust by using JMemoryBuddy utility #1121

Closed
wants to merge 9 commits into from
Closed

8296919: Make system tests that detect memory leaks more robust by using JMemoryBuddy utility #1121

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9bd7ab7
Use JMemoryBuddy in tests currently using System.gc()
lukostyra Apr 21, 2023
fbee1d2
Use JMemoryBuddy in Web leak tests
lukostyra May 1, 2023
158ed40
Address review issues
lukostyra May 2, 2023
0b8b058
Minor review updates
lukostyra May 3, 2023
5ea23cf
Update javafx.web classpath to include JMemoryBuddy
lukostyra May 4, 2023
6444cfe
Remove thread sleeps before JMemoryBuddy calls
lukostyra May 5, 2023
492d670
JMemoryBuddy: Add javadoc comments
lukostyra May 5, 2023
1230c8b
Add an EDT no-op and sleep for a bit
lukostyra May 5, 2023
98e7251
JMemoryBuddy: Update javadoc comments
lukostyra May 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion build.gradle
View file
Open in desktop
@@ -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
Expand Down Expand Up @@ -3465,6 +3465,7 @@ project(":web") {
implementation project(":graphics")
implementation project(":controls")
implementation project(":media")
testImplementation project(":base").sourceSets.test.output
kevinrushforth marked this conversation as resolved.
Show resolved Hide resolved
}

compileJava.dependsOn updateCacheIfNeeded
Expand Down
33 changes: 33 additions & 0 deletions modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -91,6 +92,38 @@ public static void assertCollectable(WeakReference weakReference) {
}
}

/**
* Checks whether an array of WeakReference objects can be collected.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should rather say something like "whether content of all references in the array", rather than array itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (+ others)

* @param weakReferences WeakReference objects to check.
*/
public static void assertCollectable(WeakReference[] weakReferences) {
lukostyra marked this conversation as resolved.
Show resolved Hide resolved
for (WeakReference wr : weakReferences) {
assertCollectable(wr);
}
}

/**
* Checks whether a Collection of WeakReference objects can be collected.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment: "content of all references in a collection"

* @param weakReferences WeakReference objects to check.
*/
public static <T> void assertCollectable(Collection<WeakReference<T>> weakReferences) {
for (WeakReference<T> wr : weakReferences) {
assertCollectable(wr);
}
}

/**
* Checks whether provided WeakReference objects can be collected.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment: "content of references..."

* @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.
Expand Down
2 changes: 1 addition & 1 deletion modules/javafx.web/.classpath
View file
Open in desktop
Expand Up @@ -42,7 +42,7 @@
<classpathentry combineaccessrules="false" kind="src" path="/base">
<attributes>
<attribute name="module" value="true"/>
<attribute name="add-exports" value="javafx.base/com.sun.javafx=javafx.web"/>
<attribute name="add-exports" value="javafx.base/com.sun.javafx=javafx.web:javafx.base/test.util.memory=javafx.web"/>
</attributes>
</classpathentry>
<classpathentry kind="con" path="org.eclipse.jdt.junit.JUNIT_CONTAINER/5">
Expand Down
55 changes: 8 additions & 47 deletions modules/javafx.web/src/test/java/test/javafx/scene/web/LeakTest.java
View file
Open in desktop
@@ -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
Expand Down Expand Up @@ -47,6 +47,7 @@
import org.w3c.dom.Element;
import org.w3c.dom.NodeList;
import static org.junit.Assert.*;
import test.util.memory.JMemoryBuddy;
kevinrushforth marked this conversation as resolved.
Show resolved Hide resolved

public class LeakTest extends TestBase {

Expand Down Expand Up @@ -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();
Expand All @@ -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) {
Expand All @@ -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++) {
Expand All @@ -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
Expand Down Expand Up @@ -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 =
"<html>\n" +
"<head></head>\n" +
Expand Down Expand Up @@ -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);
}
}
32 changes: 12 additions & 20 deletions tests/system/src/test/java/test/javafx/embed/swing/SwingNodeDnDMemoryLeakTest.java
View file
Open in desktop
@@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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<WeakReference<SwingNode>> weakRefArrSN =
new ArrayList(TOTAL_SWINGNODE);

Expand All @@ -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() {
Expand Down Expand Up @@ -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<SwingNode> ref : weakRefArrSN) {
Expand Down
49 changes: 11 additions & 38 deletions tests/system/src/test/java/test/javafx/embed/swing/SwingNodeMemoryLeakTest.java
View file
Open in desktop
@@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -47,6 +49,7 @@
import com.sun.javafx.PlatformUtil;

import test.util.Util;
import test.util.memory.JMemoryBuddy;

public class SwingNodeMemoryLeakTest {

Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my Mac (MacBook Pro x64 running macOS 13.2.1) I saw a one-time test failure here. So there might be a need to wait for the EDT to at least do something, possibly followed by a sleep (on the test thread, not on the EDT).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I will add a runAndWait() noop and a test thread sleep then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean SwingUtilities::invokeAndWait, right?

Btw, I also got a similar one-time failure in SwingNodeDnDMemoryLeakTest, so perhaps the same could be added there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant invokeAndWait. A force of habit from using our Util class...

I also added it to SwingNodeDnDMemoryLeakTest just to be sure. After a few runs it works fine on my end (but also, it did earlier so I can't really fully test it).

JMemoryBuddy.assertCollectable(weakRefArrJL);
}

private void testSwingNodeObjectsInStage() {
Expand Down Expand Up @@ -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<SwingNode> ref : weakRefArrSN) {
Expand Down
28 changes: 13 additions & 15 deletions tests/system/src/test/java/test/javafx/scene/control/AccordionTitlePaneLeakTest.java
View file
Open in desktop
@@ -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
Expand Down Expand Up @@ -42,6 +42,7 @@
import org.junit.Test;

import test.util.Util;
import test.util.memory.JMemoryBuddy;

public class AccordionTitlePaneLeakTest {

Expand All @@ -50,6 +51,8 @@ public class AccordionTitlePaneLeakTest {
static private StackPane root;
static private Stage stage;

private WeakReference<TitledPane> weakRefToPane;

public static class TestApp extends Application {
@Override
public void start(Stage primaryStage) throws Exception {
Expand Down Expand Up @@ -77,19 +80,14 @@ public static void teardownOnce() {

@Test
public void testForTitledPaneLeak() throws Exception {
TitledPane pane = new TitledPane();
accordion.getPanes().add(pane);
WeakReference<TitledPane> 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);
}
}