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.

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

8236259: MemoryLeak in ProgressIndicator #71

Closed
wants to merge 7 commits into from
@@ -324,7 +324,10 @@ Paint getProgressColor() {
void initialize() {
boolean isIndeterminate = control.isIndeterminate();
if (isIndeterminate) {
// clean up determinateIndicator
// clean up the old determinateIndicator
if (determinateIndicator != null) {
determinateIndicator.unregisterListener();
}
determinateIndicator = null;

// create spinner
@@ -541,6 +544,10 @@ public DeterminateIndicator(ProgressIndicator control, ProgressIndicatorSkin s,
updateProgress(control.getProgress());
}

private void unregisterListener() {
unregisterChangeListeners(text.fontProperty());
}

This conversation was marked as resolved by kevinrushforth
Comment on lines +547 to +550

This comment has been minimized.

Copy link
@kleopatra

kleopatra Dec 19, 2019

Collaborator

just a side-note: this will remove the whole chain of listeners to that property. Not a problem here, because the indicator seems to be the only participant listening. A bit strange using the skin's LambdaMultiplePropertyChangeListenerHandler ...

This comment has been minimized.

Copy link
@FlorianKirmaier

FlorianKirmaier Dec 19, 2019

Author Member

Yes, do you know an API for registerChangeListener which allows me to only remove a specific listener?

This comment has been minimized.

Copy link
@kleopatra

kleopatra Dec 19, 2019

Collaborator

no way, afaik - there had been debates when the api was added somewhere in jbs, don't recall exactly where

This comment has been minimized.

Copy link
@kleopatra

This comment has been minimized.

Copy link
@FlorianKirmaier

FlorianKirmaier Dec 19, 2019

Author Member

I don't see an API in the discussion, on how to remove a specific listener. Did i miss something? It seems to me, that the API is written to support only a single listener per property.

This comment has been minimized.

Copy link
@kleopatra

kleopatra Dec 19, 2019

Collaborator

the api is in a related issue (should be noted in the issue above). The use-cases mentioned in the isssue: it's a do-once scenario with the skin (and its subclasses) as the only user: multiple listeners can be registered, typically by a skin and/or its subclasses, each can be pre/postpended (or replaced) in the chain for that property and all are unregistered at dispose time. So I think it's really bad to use it in another collaborator like the indicator, particularly if that is re-created often and each of the instances need to remove the listener.

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Mar 5, 2020

Member

I think this will be fine. The text node is private to the DeterminateIndicator inner class and not exposed, so there would never be another listener.

This comment has been minimized.

Copy link
@kleopatra

kleopatra Mar 6, 2020

Collaborator

ahh yeah, you are right - somehow missed that text is private to the private indicator

This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

Copy link
@arapte

arapte Mar 11, 2020

The issue can be alternatively fixed using below change at line#510

            text.fontProperty().addListener(new WeakChangeListener<>((observable, oldValue, newValue) -> {
                doneTextWidth = Utils.computeTextWidth(text.getFont(), DONE, 0);
                doneTextHeight = Utils.computeTextHeight(text.getFont(), DONE, 0, TextBoundsType.LOGICAL_VERTICAL_CENTER);
            }));

I am not sure which one is better.
As there are no objections, I am good with the any.

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Mar 11, 2020

Member

Nope. This won't work, since the listener will be garbage collected and stop being called. It could be done by keeping the listener is an instance variable, but I recommend the original fix.

This comment has been minimized.

Copy link
@kleopatra

kleopatra Mar 12, 2020

Collaborator

@arapte the (mine :) general rule to follow is to use the highest abstraction available - here that would be to favour skin's api to un/register listeners to properties (which ideally is thoroughly tested) over ad-hoc manual code (which might not work as expected )

sry if I confused anybody with my earlier comment ...

private void setFillOverride(Paint fillOverride) {
if (fillOverride instanceof Color) {
Color c = (Color)fillOverride;
@@ -0,0 +1,110 @@
/*
* Copyright (c) 2020, 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
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package test.javafx.scene.control;
This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Mar 5, 2020

Member

You need a copyright header for this file.


import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Node;
import javafx.scene.Scene;
import javafx.scene.control.ProgressIndicator;
import javafx.scene.control.skin.ProgressIndicatorSkin;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;

import java.lang.ref.WeakReference;
import java.util.LinkedList;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

Copy link
@arapte

arapte Mar 11, 2020

Please remove the unused imports.

import junit.framework.Assert;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;

public class ProgressIndicatorLeakTest {

static CountDownLatch startupLatch;
static WeakReference<Node> detIndicator = null;
static Stage stage = null;

public static class TestApp extends Application {

@Override
public void start(Stage primaryStage) throws Exception {
ProgressIndicator indicator = new ProgressIndicator(-1);
indicator.setSkin(new ProgressIndicatorSkin(indicator));
Scene scene = new Scene(indicator);
primaryStage.setScene(scene);
stage = primaryStage;
indicator.setProgress(1.0);
Assert.assertEquals("size is wrong", 1, indicator.getChildrenUnmodifiable().size());
detIndicator = new WeakReference<Node>(indicator.getChildrenUnmodifiable().get(0));
indicator.setProgress(-1.0);
indicator.setProgress(1.0);

primaryStage.setOnShown(l -> {
Platform.runLater(() -> startupLatch.countDown());
});
primaryStage.show();
}
This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

Copy link
@arapte

arapte Mar 11, 2020

Use stage or primaryStage uniformly to make Stage calls in the start() method.

}

@BeforeClass
public static void initFX() throws Exception {
startupLatch = new CountDownLatch(1);
new Thread(() -> Application.launch(TestApp.class, (String[]) null)).start();
Assert.assertTrue("Timeout waiting for FX runtime to start", startupLatch.await(15, TimeUnit.SECONDS));
}

@Test
public void memoryTest() throws Exception {
assertCollectable(detIndicator);
}

public static void assertCollectable(WeakReference weakReference) throws Exception {
int counter = 0;

System.gc();
This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Mar 5, 2020

Member

We recommend also calling System.runFinalization(); for GC related tests.

This comment has been minimized.

Copy link
@FlorianKirmaier

FlorianKirmaier Mar 10, 2020

Author Member

Interesting, Thank you for the Information.
Never heard about this method.
I will have to add it also to JMemoryBuddy. :)

System.runFinalization();

while (counter < 10 && weakReference.get() != null) {
Thread.sleep(100);
counter = counter + 1;
System.gc();
This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Mar 5, 2020

Member

Same comment as earlier about also calling System.runFinalization();

System.runFinalization();
}

Assert.assertNull(weakReference.get());
}

@AfterClass
public static void teardownOnce() {
Platform.runLater(() -> {
stage.hide();
Platform.exit();
});
}
}
This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

Copy link
@arapte

arapte Mar 11, 2020

You can get rid of some empty lines like 130, 89, 85.

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.