JDK-8094078 : ProgressIndicator/ProgressBar continue to animate when removed or made invisible
  • Type: Bug
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: 8u20
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2014-10-03
  • Updated: 2016-03-06
  • Resolved: 2014-10-08
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 8
8u40Fixed
Related Reports
Relates :  
JDK-8094829 - Memory leak in JavaFX ProgressIndicator
Relates :  
JDK-8148827 - Indeterminate ProgressBar initially tree invisible does not animate until layoutChildren()
Relates :  
JDK-8151166 - Progress Indicator indeterminate transition not stopped when component is not visible
Relates :  
JDK-8151165 - ProgressIndicator indeterminate transition initializated even in non visible controls
Description
It's possible for ProgressIndicators/Bars to "leak" Animations and thus keep the scene pulsing and cpu burning. I wasn't able to figure out why this happens by reading the code, as the right codepaths do seem to be present, but nonetheless if I do setVisible(false) on a parent of an indeterminate ProgressIndicator, or remove the parent of an indeterminate ProgressBar from the scene graph, I can clearly see the animations are still registered in Toolkit.getToolkit().getMasterTimer().receivers and cpu usage remains high.

My hack around this issue is to set the progress bar/indicator to some definite percentage before it has a chance to be set invisible or removed from the scene graph. This forces it to stop the animation. If I don't do this, the animation stays alive somehow.
Comments
Yeah, it seems unwise to change it. In any case, it would need to be a separate JIRA...
09-10-2014
Hmmm ... looking more, the treeVisible is accessed directly in syncPeer() and elsewhere so we should not change it.
09-10-2014
> This does not appear to be the case - all we require is for a Node to be visible AND either have a null parent or a parent that is tree visible Tree visible is used by: ProgressBarSkin ProgressIndicatorSkin SwingNode MediaView It might be possible change the meaning of "tree visible" to have the meaning we expect. A node is "tree visible" when all of its parents are visible and also connected to the scene graph. Thoughts?
09-10-2014
I agree. The question is whether there is existing code depending on the current behavior.
08-10-2014
For what it's worth I had the same misunderstanding about what treeVisible does, which is why I couldn't figure it out. It seems like the intuitive definition of treeVisible would have it be false if not a part of the scene graph.
08-10-2014
Tomas, the code I was looking at is Node.updateTreeVisible(boolean). In my repo, the relevant section reads as follows: boolean isTreeVisible = isVisible(); final Node parentNode = getParent() != null ? getParent() : clipParent != null ? clipParent : getSubScene() != null ? getSubScene() : null; if (isTreeVisible) { isTreeVisible = parentNode == null || parentNode.impl_isTreeVisible(); } The misinterpretation that I had (assuming it is a misinterpretation and not a bug in the Node code) is that I assumed that for treeVisible to be true, a Node had to be connected to a Scene's root node. This does not appear to be the case - all we require is for a Node to be visible AND either have a null parent or a parent that is tree visible. Of course, the Scene's root node will have a null parentNode, so the root Node is implicitly tree visible (assuming it isVisible()). But we also have tree visible true for a scene graph of nodes that are not connected to any Scene - which is my misunderstanding. This leads to the issue in ProgressIndicator - even though we've removed the ProgressIndicator from the scenegraph (i.e. it has been removed from its parents children list), the ProgressIndicator is still considered to be treeVisible as it meets the criteria above - it is visible, and it has a null parent node. To me this seems counter intuitive - but that is said as someone unfamiliar with the intentions of the treeVisible API. My expectation would be that treeVisible is false because the ProgressIndicator is not visible from the root of the tree. In any case, I've resolved this issue in a different manner now by bringing back some previously removed code. I'm still interested in understanding if I'm just wrong or if something is wrong about this code. I can't say with any certainty as I've not been involved in the design and development of the API.
08-10-2014
Hi Jonathan, > What appears to be the case is that a node is tree visible whenever the node is visible OR whenever a node has a non-null parent node (even if the parent is itself not tree visible). This last part seems unintuitive to me, so I wouldn't mind another set of eyes cast over this. If I'm looking at the same version of Node.updateTreeVisible(boolean), my understanding is that a node is tree visible if it is visible (isVisible()) AND its parent, if any, is tree visible. In other words, a node is tree visible if it is visible and all its parents are visible. This seems OK to me. The method does not check whether the whole tree is in the scene graph at all. It just reflects whether all of the ancestors have visibleProperty() set to true.
08-10-2014
Changeset: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/105708815d36 Updated test program (to also test ProgressBar): import com.sun.javafx.tk.TKPulseListener; import com.sun.javafx.tk.Toolkit; import javafx.application.Application; import javafx.scene.Scene; import javafx.scene.control.Button; import javafx.scene.control.ProgressBar; import javafx.scene.control.ProgressIndicator; import javafx.scene.control.ToggleButton; import javafx.scene.layout.VBox; import javafx.stage.Stage; public class ProgressIndicatorAnim extends Application { private int tickCount = 0; private final TKPulseListener listener = () -> System.err.println("pulse: " + (++tickCount)); @Override public void start(Stage stage) { stage.setTitle("ProgressIndicator Animation"); VBox root = new VBox(); Scene scene = new Scene(root, 600, 450); ProgressIndicator pInd2 = new ProgressIndicator(); pInd2.setPrefSize(50, 50); pInd2.setLayoutX(25); pInd2.setLayoutY(40); pInd2.setVisible(true); ProgressIndicator pInd1 = new ProgressIndicator(); pInd1.setPrefSize(150, 150); pInd1.setLayoutX(200); pInd1.setLayoutY(20); pInd1.setProgress(0.25f); pInd1.setVisible(true); ProgressBar pBar2 = new ProgressBar(); pBar2.setPrefSize(50, 50); pBar2.setLayoutX(25); pBar2.setLayoutY(40); pBar2.setVisible(true); ProgressBar pBar1 = new ProgressBar(); pBar1.setPrefSize(150, 150); pBar1.setLayoutX(200); pBar1.setLayoutY(20); pBar1.setProgress(0.25f); pBar1.setVisible(true); Button removeButton = new Button("Remove Indeterminate Progress"); removeButton.setOnAction(e -> { root.getChildren().remove(pInd2); root.getChildren().remove(pBar2); // System.out.println(pInd2.getParent() + " " + pInd2.impl_isTreeVisible()); // pInd2.setVisible(false); }); ToggleButton toggleProgressButton = new ToggleButton("Toggle Progress"); toggleProgressButton.setOnAction(e -> { if (toggleProgressButton.isSelected()) { pInd2.setProgress(1.0); pBar2.setProgress(1.0); } else { pInd2.setProgress(-1); pBar2.setProgress(-1); } }); root.getChildren().addAll(removeButton, toggleProgressButton, pInd1, pInd2, pBar1, pBar2); stage.setScene(scene); stage.show(); Toolkit.getToolkit().addSceneTkPulseListener(listener); } public static void main(String[] args) { Application.launch(args); } }
08-10-2014
Here's the changeset where the regression was introduced: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/35b2725a3dce I'll take a closer look asap, but it seems that we removed a bunch of listeners in the code for scene, visible, and parent, and in so doing we most likely lost our ability to stop the animation when no longer visible. The changeset is actually the combination of three changes due to the fact there were three developers all working in the same class files at the same time, so it will take a little to unravel. Still, the question remains whether treeVisible is sufficient (and incorrectly implemented) or if I need to re-add in some listeners to ensure the animation stop in certain situations.
07-10-2014
The first bad revision is: changeset: 7087:35b2725a3dce user: jgiles date: Wed May 21 11:29:14 2014 +1200 summary: RT-37117: [ProgressBar] Animation in progress bar disappears after few run with Java 8 which was running successfully with java 7 (regression)
07-10-2014
Apparently, this is a regression. Would it help to find the change set that caused it?
07-10-2014
David's correctly identified the code that is in ProgressIndicatorSkin that is responsible for pausing the animation. The problem appears to be that impl_treeVisibleProperty does not work as I expected it to. My assumption was that impl_treeVisibleProperty is false whenever a node is not accessible from a scene root node. However, looking at Node.updateTreeVisible(boolean), this doesn't seem correct. What appears to be the case is that a node is tree visible whenever the node is visible OR whenever a node has a non-null parent node (even if the parent is itself not tree visible). This last part seems unintuitive to me, so I wouldn't mind another set of eyes cast over this. To confirm that generally the code works (where impl_treeVisibleProperty toggles to false), I changed a section of Kevin's code (in the attached file) to the following: removeButton.setOnAction(e -> { // root.getChildren().remove(pInd2); pInd2.setVisible(false); }); When I toggle the visibility, the animation pauses as expected. If I change the code in Node.updateTreeVisible(boolean), I can also get the expected result. For example, the relevant line of code is the following: isTreeVisible = parentNode == null || parentNode.impl_isTreeVisible(); If I change this to the following, I can run Kevin's test application and get the expected results (and also, impl_treeVisibleProperty overall makes more sense to me): final Scene scene = getScene(); isTreeVisible = (parentNode != null && parentNode.impl_isTreeVisible()) || scene == null || (parentNode == null && scene != null && scene.getRoot() == this); This isn't my suggested fix - it's just an observation (especially as it breaks a few unit tests in javafx.scene.NodeTest and javafx.scene.FocusTest). There are two options: 1) We discuss treeVisible and decide whether it is or is not implemented correctly, and fix it there if applicable. 2) We rewrite the code in the ProgressIndicatorSkin to instead observe other properties (i.e. visible and scene, most probably). I'll await further discussion before proceeding with option 2.
07-10-2014
This is a regression. ProgressIndicator/ProgressBar animations should not run if the control is not visible in the scene-graph. The code that does this is in ProgressIndicatorSkin: protected final InvalidationListener treeVisibleListener = observable -> { final boolean isTreeVisible = getSkinnable().impl_isTreeVisible(); if (indeterminateTransition != null) { pauseTimeline(! isTreeVisible); } else if (isTreeVisible) { createIndeterminateTimeline(); } }; And private IndeterminateSpinner(boolean spinEnabled, Paint fillOverride) { // does not need to be a weak listener since it only listens to its own property impl_treeVisibleProperty().addListener(treeVisibleListener);
06-10-2014
One way to work around is to use the custom subclass of ProgressBar below, and replace setProgress(x) and progressProperty() in your code with setMyProgress(x) and myProgressProperty(), respectively. class MyProgressBar extends ProgressBar { private final DoubleProperty myProgress = new SimpleDoubleProperty(); public DoubleProperty myProgressProperty() { return myProgress; } public void setMyProgress(double progress) { myProgress.set(progress); } public MyProgressBar() { progressProperty().bind( Bindings.when(Bindings.isNotNull(sceneProperty())) .then(myProgress) .otherwise(0); ); } } Note that this only works for removing the progress bar from the scene graph. It does not stop the animation when the progress bar or any of its parents is made invisible (e.g. setVisible(false) or setOpacity(0.0)).
03-10-2014
import com.sun.javafx.tk.Toolkit; import com.sun.scenario.animation.AbstractMasterTimer; import javafx.application.Application; import javafx.application.Platform; import javafx.scene.Scene; import javafx.scene.control.ProgressIndicator; import javafx.scene.layout.HBox; import javafx.scene.layout.VBox; import javafx.stage.Stage; import java.lang.reflect.Field; public class ProgressLeaker extends Application { @Override public void start(Stage primaryStage) throws Exception { VBox vbox = new VBox(); final ProgressIndicator indicator = new ProgressIndicator(-1); HBox hBox = new HBox(indicator); vbox.getChildren().addAll(hBox); primaryStage.setScene(new Scene(vbox)); primaryStage.show(); runAfter(1000, () -> { vbox.getChildren().removeAll(hBox); }); Runnable runnable = () -> { try { Field receivers = AbstractMasterTimer.class.getDeclaredField("receivers"); receivers.setAccessible(true); Object[] array = (Object[]) receivers.get(Toolkit.getToolkit().getMasterTimer()); for (Object o : array) { if (o != null) System.out.println(o); } System.out.println("---"); } catch (Exception e) { e.printStackTrace(); } }; runnable.run(); runAfter(2000, runnable); } static void runAfter(long millis, Runnable runnable) { new Thread(() -> { try { Thread.sleep(millis); Platform.runLater(runnable::run); } catch (InterruptedException e) { } }).start(); } public static void main(String[] args) { launch(args); } }
03-10-2014
Steps to reproduce: 1) Run attached program 2) Press the "Remove Progress" button Notice that the animation continues 3) Press the "Toggle Progress" button Notice that the animation stops.
03-10-2014
I was able to reproduce it trivially (I will attach the simple program). The workaround of setting the progress to a determinate value works either before or after the progress indicator is removed.
03-10-2014
Can you provide a simple test case that shows the leak?
03-10-2014