JDK-8189265 : Closing stage does not free internal resources
  • Type: Bug
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: 8u144,9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2017-10-12
  • Updated: 2022-10-21
  • Resolved: 2017-11-17
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 8u172Fixed
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 :
Closing a stage does not release it's resources.  If there is a ProgressIndicator in the stage, the animation framework will stay connected to this and the hierarchy will not be eligible for garbage collection.

When a Dialog closes, its close() call stack clears the scene, which allows components to clean up their resources automatically.

Java 8U144 behaves the same.

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
I used the NB profiler to test the memory.

Run the program, opening up a dialog and then closing it does not cause the ProgressIndicator objects to stay in memory.  Opening and then closing the stage does.  Looking in the profiler, you will see that the animator is the reference chain.

I left a workaround in the code (that is commented out) which clears the scene on close.  This eliminates the leak.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
No leaks
ACTUAL -
Objects are not eligible for GC.

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.event.ActionEvent;
import javafx.event.EventHandler;
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;

/**
 *
 * @author ocMAClaassen
 */
public class JavaFXMemoryLeak extends Application {

	@Override
	public void start(final Stage primaryStage) {
		Button btn1 = new Button();
		btn1.setText("Open Dialog");
		btn1.setOnAction(new EventHandler<ActionEvent>() {

			@Override
			public void handle(ActionEvent event) {
				openDialog(primaryStage);
			}
		});
		Button btn2 = new Button();
		btn2.setText("Open Stage");
		btn2.setOnAction(new EventHandler<ActionEvent>() {

			@Override
			public void handle(ActionEvent event) {
				openStage(primaryStage);
			}
		});

		HBox root = new HBox();
		root.getChildren().add(btn1);
		root.getChildren().add(btn2);

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

		primaryStage.setTitle("Memory Test");
		primaryStage.setScene(scene);
		primaryStage.show();
	}
	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) {
		BorderPane root = new BorderPane();
		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(new EventHandler<ActionEvent>() {

			@Override
			public void handle(ActionEvent event) {
				stage.close();
				//Workaround
//				stage.getScene().setRoot(new StackPane());
			}
		});

		root.bottomProperty().set(btn);

		Scene scene = new Scene(root);
		stage.setScene(scene);

		stage.showAndWait();

	}

	/**
	 * @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 ----------

CUSTOMER SUBMITTED WORKAROUND :
When dialogs close, a bogus is set on the scene.   So, imitate this.
stage.getScene().setRoot(new StackPane());
(Heavyweight Dialog has: scene.setRoot(DUMMY_ROOT);)




Comments
I have created JDK-8193041 to address Kevin's question above.
05-12-2017

This is approved to backport to 8u-dev for 8u172.
30-11-2017

Look good. One minor comment and a question both about the test: 1. Minor formatting issue: Line 55 is indented more than it should be: final static int TOTAL_PROGRESS_INDICATORS = 10; No need for a new webrev to fix the indentation. 2. Question: I see you added a new test method for 8 that wasn't in the test ported from 10. Is this test applicable to 10? If so, you might file a follow-on testbug to add this test to 10 (unless you think it is not an interesting test case for 10). +1
30-11-2017

Thanks for detailed review. Here is the updated 8u patch- http://cr.openjdk.java.net/~aghaisas/fx/8189265/8u/webrev.3/ Request to review.
29-11-2017

+ // Re-register change listner for property windowShowingProperty + if (windowShowingProperty != null) { + unregisterChangeListener(windowShowingProperty); + windowShowingProperty = null; + + if ((control.getScene() != null) && (control.getScene().getWindow() != null)) { + windowShowingProperty = control.getScene().getWindow().showingProperty(); + registerChangeListener(windowShowingProperty, "WINDOWSHOWING"); + } + } 1. Those two tests should be independent of one another. Otherwise you will never register a new change listener if it starts out null or ever goes to null. 2. I think you might also need to listen for a Window changing on the scene. 3. Also, as long as you are updating the fix, you spelled "listener" wrong in the comment..
21-11-2017

Thanks for 8u review. Here is the updated patch for 8u : http://cr.openjdk.java.net/~aghaisas/fx/8189265/8u/webrev.2/ Request you to review.
20-11-2017

I shall backport it to 8u once webrev is approved.
17-11-2017

Fixed for JDK 10 ------------------------ URL: http://hg.openjdk.java.net/openjfx/10-dev/rt/rev/d561ad4d6761 Author: aghaisas Reviewed-by: kcr
17-11-2017

As discussed offline, the test for 8u has the wrong package name, and the fix itself might need additional logic for adding / removing the listener for the window showing property (for 10, this wasn't needed because isTreeShowing takes care of it), since the listener is only created if the ProgressIndicator has a non-null scene at the time the skin is constructed. Also, as long as you are updating it, can you fix the copyright date in ProgressIndicatorSkin.java? Please post an updated webrev for 8u when ready and I will review.
17-11-2017

The test looks good now, although you need to fix the trailing whitespace or jcheck will fail: modules/javafx.controls/src/test/java/test/javafx/scene/control/ProgressIndicatorTest.java:103: Trailing whitespace No need for a new webrev to remove the trailing space. +1 (for the JDK 10 fix)
16-11-2017

Thanks for review. I have updated the test case to reflect suggestions. Here are the updated patches : Fix for JDK 10 : http://cr.openjdk.java.net/~aghaisas/fx/8189265/10/webrev.1/ Fix for JDK 8u : http://cr.openjdk.java.net/~aghaisas/fx/8189265/8u/webrev.1/
16-11-2017

This review is for the FX 10 fix. Once the review is finished for 10, I will review the FX 8u fix. The fix itself looks good. I have a few comments on the test: 1. The attemptGC method should check getCleanedUpObjectCount() each time through the loop, right before the sleep, and return early if it reaches the desired value of TOTAL_PROGRESS_INDICATORS, so as to avoid unnecessary sleeping; as it stands now, each of the two new tests will always take 5 seconds. 2. I recommend adding the following two assertions in each of the two new testProgressIndicatorObjectsInXXXX methods, after you have created the scene graph, and prior to creating the Stage or Alert: assertEquals(TOTAL_PROGRESS_INDICATORS, weakRefArr.size()); assertEquals(0, getCleanedUpObjectCount()); 3. The weakRefArray.clear() is not needed since the arrayList is recreated for each test (and if it were needed, it is not the best place to do it, since it would be skipped if there was a test failure). I recommend to just remove this call from the two tests. 4. As a best practice, in the getCleanedUpObjectCount method, I might recommend using either a for-each loop or else use "weakRefArr.size()" as the count rather than a hard-coded value for the array size.
15-11-2017

Behavior : -------------- JDK 9/10 : ProgressIndicator leaks memory if it is a part of Alert and Alert is dismissed. JDK 8u : ProgressIndicator leaks memory if it is part of a stage and stage is closed. Root Cause : ------------------- The indeterminate ProgressIndicator animation is not paused on Alert dismissal or Stage closure. Fixes : ----------- Fix for JDK 10 : http://cr.openjdk.java.net/~aghaisas/fx/8189265/10/webrev.0/ Fix for JDK 8u : http://cr.openjdk.java.net/~aghaisas/fx/8189265/8u/webrev.0/ Request you to review.
15-11-2017

Attached a modified version of the test program that runs GC and prints the total number of live ProgressIndicator instances every two seconds.
14-11-2017

After Ajit's comment, Checked again and here is my observation: ========================================== 8u144 + java VisualVm ------------------------------------- opening and closing Dialog : 11 clicks - instances of 'javafx.scene.control.ProgressIndicator' comes down to 1 But again every click increases no. of instances.. Only 'perform GC' clears instances opening and closing Stage : 24 clicks - 24 instances of 'javafx.scene.control.ProgressIndicator'. clicking 'perform GC' also doesn't clear instances. 8u144 + NB (version 8.2) profiler ------------------------------------- opening and closing Dialog : 5 clicks - comes to 1 , again 20 clicks - instances comes down to 0 opening and closing Stage : 15 clicks - 15 instances of 'javafx.scene.control.ProgressIndicator' 9+181 + NB (Developer version) profiler ------------------------------------- opening and closing Dialog : 15 clicks - 15 instances of 'javafx.scene.control.ProgressIndicator' opening and closing Stage : after sometime of clicking many times, instances comes down to 0
31-10-2017

For me, NB Profiler works only with the JDK version that is selected as Default while installing NB. With above premise, below is the observation : 1) I could reproduce the issue with a little older JDK8 build. (JDK 8u101 - was used as default while installing NB) 2) I uninstalled and reinstalled NB with JDK 9GA as default JDK -- the observation is opposite to what is reported. For me, opening and then closing the stage does not cause memory leak, but, opening and closing dialog causes the leak. I tried this multiple times with same results. 3) The test code does not compile with JDK 8 GA
17-10-2017

Regarding the above comment, this isn't a feature request, but a bug. Closing the stage should release any strong references held by the JavaFX runtime to the stage, its Scene, and all Scene graph objects. If we are holding a strong reference, then it's very likely a bug. I note that we have fixed other memory leaks specific to ProgressIndicator so that may be a place to look.
13-10-2017

Seems like feature request for Stage.close() method. Stage.close() just calls hide() which doesn't release components of referred scene. http://download.java.net/java/jdk9/docs/api/javafx/stage/Stage.html#close-- When checked in 8u144 using java Visual VM, opening stage again and again increases number of instances of "javafx.scene.control.ProgressIndicator" and even after clicking on "Perform GC" doesn't free up object of "javafx.scene.control.ProgressIndicator" unlike in case of Dialog. After enabling workaround, clicking "perform GC" clears all instances of 'ProgressIndicator'. Could not conclude for 9-ea+181 using Oracle JMC as even after GC, instances of "javafx.scene.control.ProgressIndicator" was not coming down in case of both Stage and Dialog.
13-10-2017