JDK-8189280 : Memory leak in SwingNode if Stage is not shown
  • Type: Bug
  • Component: javafx
  • Sub-Component: swing
  • Affected Version: 8u144,9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: other
  • CPU: x86
  • Submitted: 2017-10-12
  • Updated: 2022-10-21
  • Resolved: 2017-12-09
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 10 JDK 8
10Fixed 8u162Fixed
Related Reports
Relates :  
Description
FULL PRODUCT VERSION :
java version "9"
Java(TM) SE Runtime Environment (build 9+181)
Java HotSpot(TM) 64-Bit Server VM (build 9+181, mixed mode)

ADDITIONAL OS VERSION INFORMATION :
Microsoft Windows [Version 10.0.15063]

A DESCRIPTION OF THE PROBLEM :
If a stage is never shown, a SwingNode will never be elgible for GC.  If the stage is shown, it will eventually be removed by the GC

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
The memory leak is evident in a profiler.  I used the NB profiler.

Run the test program.  Keep pressing the "Open Stage" button.  Doing so will continually open intermediate stages, closing an existing one if it exists.

If the intermediate stage was being shown (the stage.show() method at line 100 is not commented out), the SwingNodes will be collect appropriately.  If the intermediate stages are not shown (the stage.show() method at line 100 is commented out), they will never be collected.

Don't forget to use the -Djavafx.embed.singleThread=true option.


EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
The SwingNodes will eventually be collected
ACTUAL -
No SwingNodes are collected if the intermediate stages are not shown

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
/*
 * To change this license header, choose License Headers in Project Properties.
 * To change this template file, choose Tools | Templates
 * and open the template in the editor.
 */
package javafxmemoryleak;

import javafx.application.Application;
import javafx.embed.swing.SwingNode;
import javafx.scene.Scene;
import javafx.scene.control.Alert;
import javafx.scene.control.Button;
import javafx.scene.control.ProgressIndicator;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.HBox;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;
import javax.swing.JLabel;

public class JavaFXMemoryLeak extends Application {
	private Stage tempStage;

	@Override
	public void start(final Stage primaryStage) {
		Button openDialog = new Button();

		openDialog.setText("Open Dialog");
		openDialog.setOnAction(e -> openDialog(primaryStage));

		Button openStage = new Button();
		openStage.setText("Open Stage");
		openStage.setOnAction(e -> openStage(primaryStage));

		Button closeStage = new Button();
		closeStage.setText("Close Stage");
		closeStage.setOnAction(e -> closeStage());

		HBox root = new HBox();
		root.getChildren().add(openDialog);
		root.getChildren().add(openStage);
		root.getChildren().add(closeStage);

		Scene scene = new Scene(root, 300, 250);

		primaryStage.setTitle("Memory Test");
		primaryStage.setScene(scene);
		primaryStage.show();
	}
	private void closeStage() {
		if (tempStage != null)
			tempStage.close();
		tempStage = null;
	}
	private void openDialog(Stage primaryStage) {
		StackPane root = new StackPane();
		ProgressIndicator pi = new ProgressIndicator();
		pi.parentProperty().addListener((obs, oldVal, newVal) -> System.out.println("Parent: " + newVal));
		pi.sceneProperty().addListener((obs, oldVal, newVal) -> {
			System.out.println("Dialog Scene: " + newVal);
		});
		root.getChildren().add(pi);

		Alert dialog = new Alert(Alert.AlertType.INFORMATION);
		dialog.getDialogPane().setContent(root);

		dialog.initOwner(primaryStage);
		dialog.showAndWait();

	}
	private void openStage(Stage primaryStage) {
		closeStage();
		BorderPane root = new BorderPane();
		SwingNode sw = new SwingNode();
		sw.setContent(new JLabel("SWING"));
		root.centerProperty().set(sw);

//		ProgressIndicator pi = new ProgressIndicator();
//		pi.parentProperty().addListener((obs, oldVal, newVal) -> System.out.println("Parent: " + newVal));
//		pi.sceneProperty().addListener((obs, oldVal, newVal) -> {
//			System.out.println("Scene: " + newVal);
//		});
//		root.centerProperty().set(pi);
		final Stage stage = new Stage();
		Button btn = new Button();
		btn.setText("Stage Close");
		btn.setOnAction(e -> {
			stage.close();
		});

		root.bottomProperty().set(btn);

		Scene scene = new Scene(root, 150, 100);
		stage.setScene(scene);
		tempStage = stage;
		//If we don't show the stage, resources are never released
//		stage.show();
	}

	/**
	 * @param args the command line arguments
	 */
	public static void main(String[] args) {
		System.out.println("java.version = " + System.getProperty("java.version"));
		System.out.println("java.runtime.version = " + System.getProperty("java.runtime.version"));
		launch(args);
	}

}

---------- END SOURCE ----------


Comments
Looks good. This is approved to push to 8u-dev for 8u172. +1
11-12-2017

8u webrev for review. http://cr.openjdk.java.net/~psadhukhan/fx/8189280/8udev/webrev/
11-12-2017

This looks good to me now with three comments on the test: 1. The following whitespace problem will cause jcheck to fail: tests/system/src/test/java/test/javafx/embed/swing/SwingNodeMemoryLeakTest.java: 124: Tab character 2. The definition of GC_ATTEMPTS is: 54 final static int GC_ATTEMPTS = TOTAL_SWINGNODE; This still suggests to the reader of the code that there is some relation between GC_ATTEMPTS and TOTAL_SWINGNODE. There is not. If you were trying to test the behavior of 1 SwingNode or 100 SwingNodes you would still want a value of GC_ATTEMPTS in the 5 to 10 range -- we use 5 in most of our tests with a somewhat longer sleep of 1 second...you use 10 with a sleep of 1/4 sec, which also seems good. Please set this field equal to '10' instead of 'TOTAL_SWINGNODE'. 3. Typo here: 134 // Attempt gc GC_ATTEMPTS imes No need for a new webrev to fix these three minor issues. Please run 'hg jcheck' before pushing +1 pending these three changes
09-12-2017

After pushing this change to jfx-dev for 10, please prepare a backport webrev for 8u-dev, and then make a request to get the fix into 8u-dev.
09-12-2017

Changeset: 0011c8aee5fe Author: psadhukhan Date: 2017-12-09 20:16 +0530 URL: http://hg.openjdk.java.net/openjfx/jfx-dev/rt/rev/0011c8aee5fe
09-12-2017

Thanks for the review. Just fyi, I will be pushing this patch after addressing the above comments http://cr.openjdk.java.net/~psadhukhan/fx/8189280/webrev.04/
09-12-2017

Modified webrev to address these changes http://cr.openjdk.java.net/~psadhukhan/fx/8189280/webrev.03/ Normally we follow this copyright w/o classpath exception in jdk regression tests, I presume.
09-12-2017

Regarding the missing Classpath exception, I found one more, which is probably where you copied it from, since it is in the same package... tests/system/src/test/java/test/javafx/embed/swing/FXImageConversionTest.java
08-12-2017

The code changes in the .02 webrev all look good. The test looks pretty good, too, but I have a few comments. SwingNodeMemoryLeakTest.java: 1. The copyright header is wrong in that is misses the GPLv2 classpath exception. Where did you copy it from? We might have other files with this bug. As long as you are fixing this, can you add a blank line before the 'package' statement? 2. The test has a bug in the following: 108 for (int i = 0; i < TOTAL_SWINGNODE; i++) { ... 119 Stage stage = new Stage(); ... 123 tempStage = stage; 124 } Note that tempStage is only pointing to the last stage. I recommend changing tempStage to an array (and if you do, make sure you clean up each entry in the test method). As it is, the "stage.close()" you do after calling this method is missed for the first 9 stages, although since you never show the stages that doesn't really matter. You probably should make this change, however, since without it, the following test is fragile: 128 assertEquals(0, getCleanedUpSwingNodeCount()); 129 assertEquals(0, getCleanedUpJLabelCount()); At this point in the test method, you no longer hold a strong reference to the first 9 stages, SwingNodes, and JLabels, so there is nothing that would guarantee the first 9 from getting garbage collected between the time you create them and the time you call this assertion. Making tempStage an array would fix this. I note that the array can be local to the testSwingNodeObjectsInStage method, rather than an instance variable, if you prefer. 3. In general, mutable statics in unit tests should be avoided where possible. I think the following three can be instance variables, along with the methods that reference them: 51 private static Stage tempStage; 52 private static ArrayList<WeakReference<SwingNode>> weakRefArrSN = 53 new ArrayList(TOTAL_SWINGNODE); 54 private static ArrayList<WeakReference<JLabel>> weakRefArrJL = 55 new ArrayList(TOTAL_SWINGNODE); 4. In the following call: 89 attemptGCSwingNode(TOTAL_SWINGNODE); ... 97 attemptGCJLabel(TOTAL_SWINGNODE); The 'n' used in these GC methods bears no relation to the count of objects you are trying to clean up, so this is confusing. You might create a separate constant and set it to 5 (minimum) or 10...something like "GC_ATTEMPTS". In fact you can make this a static final int and not pass it at all if you prefer. 5. MINOR: odd indentation on line 111: 110 SwingNode sw = new SwingNode(); 111 JLabel label = new JLabel("SWING"); 112 sw.setContent(label); 6. MINOR: You might consider removing the debug System.out print statements (or put them behind a "verbose" flag), since the assert statements will provide the needed information.
08-12-2017

Based on offline discussion, - converted WindowFocusListener to a static named class and use WeakReference of SwingNode to invoke requestfocus and ungrabFocus routine - converted SwingNodeContent to a static class and use WeakReference of SwingNode to invoke swingnode's methods - Add a disposer pattern to store SwingNode instance and dispose it off when not reached in this webrev http://cr.openjdk.java.net/~psadhukhan/fx/8189280/webrev.02/
07-12-2017

Yes we can have more than one SwingNode per Scene, so you can't make that assumption. Perhaps we can discuss this offline.
30-11-2017

Modified webrev adding listeners even though it does not seem to be get called and assuming 1 swingnode per scene http://cr.openjdk.java.net/~psadhukhan/fx/8189280/webrev.01/
29-11-2017

I tried adding 1. sceneProperty().addListener((observable, oldValue, newValue) 2. NodeHelper.treeVisibleProperty(this).addListener((observable, oldValue, newValue) 3. window.showingProperty().addListener(windowVisibleListener) to SwingNode.setContentImpl. 2 and 3 is never called as oldValue and newValue are both false. 1 is "sometimes" called with oldValue null and newValue javafx.scene.Scene. I tried calling NodeHelper.isTreeShowing(this) which show it is false sceneProperty().addListener((observable, oldValue, newValue) -> { System.out.println("setContent oldValue " + oldValue + " newValue " + newValue); System.out.println("NodeHelper.isTreeShowing(newValue) " + NodeHelper.isTreeShowing(this)); }); but this listener is not called every time I run same testcase. Also, even when it is called in the testcase, it is not called for every setContent call so I cannot rely to make any fix. Can we have more than 1 SwingNode per scene?
28-11-2017

The case Alex Z mentioned is a perfectly valid case, so yes, you need to handle it. You might be able to add a listener on the isTreeShowing and scene properties of the SwingNode, although one other thing you need to keep in mind is that for printing and snapshot you need to be able to show the content even when there is no Window showing.
27-11-2017

OK. In this case, getScene() is null but even then swingnode needs to be shown, I was not sure of if this is valid usecase so moved the setContent call only when getScene != null. Is it not a bug in fx graphics to return null for getScene() for this usecase? - ok. since setContent() is called before new scene is created, so getScene() returns null. Is there any "property" that will notify swingnode if stage is shown or not?
27-11-2017

SwingNode content is added to JLightweightFrame even when the scene window is not shown. Propsosed fix is to add swingNode content to JLightweightFrame only when window is showing. Also, dispose of the JLightweightFrame when window is made invisible, just as we do in windowVisibleListener(). Request to review the fix http://cr.openjdk.java.net/~psadhukhan/fx/8189280/webrev.00/
27-11-2017

With your fix I can't see a content of a SwingNode if its content set before show: Stage sta = new Stage(); SwingNode node = new SwingNode(); node.setContent(new JLabel("213213")); Scene sc = new Scene(new Group(node)); sta.setScene(sc); sta.show();
27-11-2017

More info from Bug submitter: SwingNode actually leaks a lot. Immediately after the call to setContent, the JComponent is attached to its JLightweightFrame and JLightwidghtFramePeer. This is a huge problem, because I don’t think this is ever cleared. Calling setContent(null) would clear it, but this is not a natural thing to do. The attachment(DSwingNode.java) is a subclass of SwingNode that I think solves the problem and will hopefully provide a some inspiration for a more integrated fix in SwingNode itself. The main things that happens here is that the setContent call keeps a reference to the JComponent passed in, but does not attach it to the JLightweightFrame until it is added to the scene. When the SwingNode is removed from its scene, the component is removed from the JLightweightFrame. (Calling setContent(null) will remove it from the JLightweightFrame.). The purpose of the “Wrapper” class is because we have not found a way to traverse up the hierarchy of swing components and then get the wrapping javafx nodes. We made the wrapper so that we can call getParent() to travel up the stack of JComponents, find this wrapper class by using “instanceof” and then we can grab the SwingNode and access where it is in the scene graph. Maybe there is a better way to do this, but we have not found what it is. The COUNTER and name are just things I introduced so I could track what was going on. I removed the log messages to make the code more concise, but didn’t take out all of this support.
17-10-2017

I tried with 8u152's jvisualvm with jdk10 consolidated repo [built with openjfx10 via --with-import-modules argument]. Profiling with and without -Djavafx.embed.singleThread=true, I observe the following with stage.show() commented For each click on "open stage", swingnode instance in heap dump increases by 1. So, after 5 click on "open stage", swingnode instance is 5. Even if we click on "close stage" or "perform GC" in jvisualvm, swingnode instances does not come down to 0. Then, I tried with stage.show() uncommented. The swingnode instances is 1 even after 5 click on "open stage". But it does not come to 0 after I click on "Perform GC" in jvisualvm. So, in my testing -Djavafx.embed.singleThread=true does not have any bearing. It behaves the same either way. To me, it does not seem to be a swing/interop problem but a "stage" problem. BTW, I commented out openDialog() routine in testcase since it involves ProgressIndicator "controls", which has nothing to do with swing interop. But, even with that, the behaviour is same as I do not press on "open dialog" anytime.
16-10-2017

This is likely not a window-toolkit (aka glass) bug. It is probably a Swing interop bug, but might be a CSS (controls) bug.
13-10-2017

Checked using NB profiler for 9-ea+181. Ran testcase with -Djavafx.embed.singleThread=true option. Case 1 : When stage.show() is commented : Clicking "open stage" increases number of 'SwingNode' instances in profiler with each click. After GC also, number of instances is same. (PFA screenshot MemoryLeak.png) Case 2 : When stage.show() is not commented : each click of "open stage" opens only one stage and increases number of 'SwingNode' instances with each click. But after GC, there is only 1 'SwingNode' instance left. Checked using java VisualVM for 8u144 (with option Djavafx.embed.singleThread=true) : Case 1 : When stage.show() is commented : 4-5 clicks on "open stage" increases count of 'SwingNode' instances by 1. But even after GC, there are still same instances of 'SwingNode' as before GC. Case 2 : When stage.show() is not commented : 4-5 clicks on "open stage" increases count of 'SwingNode' instances by 1. But after GC, count becomes 0.
13-10-2017