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

8241840: Memoryleak: Closed focused Stages are not collected with Monocle. #153

Closed
wants to merge 11 commits into from
@@ -0,0 +1,56 @@
/*
* 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.stage;

import javafx.application.Platform;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;

public class FocusedWindowMonocleTest extends FocusedWindowTestBase {

static {
System.setProperty("glass.platform","Monocle");
System.setProperty("monocle.platform","Headless");
}

@BeforeClass
public static void initFX() throws Exception {
initFXBase();
}

@Test
public void testClosedFocusedStageLeak() throws Exception {
testClosedFocusedStageLeakBase();
}

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

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

Minor: Add missing newline

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 13, 2020
Member

I meant that the file should end with a newline (it doesn't matter whether or not there is a newline before the final closing brace). The red circle with a - in it is GitHub's way of showing you that the file doesn't end with a newline.

This comment has been minimized.

@FlorianKirmaier

FlorianKirmaier Aug 14, 2020
Author Member

done

@@ -0,0 +1,52 @@
/*
* 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.stage;

import javafx.application.Platform;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;

public class FocusedWindowNativeTest extends FocusedWindowTestBase {


@BeforeClass
public static void initFX() throws Exception {
initFXBase();
}

@Test
public void testClosedFocusedStageLeak() throws Exception {
testClosedFocusedStageLeakBase();
}

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

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

Minor: Add missing newline

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 13, 2020
Member

Same as in previous file.

This comment has been minimized.

@FlorianKirmaier

FlorianKirmaier Aug 14, 2020
Author Member

done

@@ -38,23 +38,17 @@
import java.util.concurrent.TimeUnit;

import junit.framework.Assert;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.Ignore;
import test.util.Util;

public class FocusedWindowTest {
@Ignore
abstract public class FocusedWindowTestBase {
This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

The preferred order is public abstract. Also, there is no need for an @Ignore here.


static CountDownLatch startupLatch;
static Stage stage = null;

static {
System.setProperty("glass.platform","Monocle");
System.setProperty("monocle.platform","Headless");
}

This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

Minor: remove extra blank line.

@BeforeClass
public static void initFX() throws Exception {
public static void initFXBase() throws Exception {
startupLatch = new CountDownLatch(1);
Platform.startup(startupLatch::countDown);
Platform.setImplicitExit(false);
@@ -65,8 +59,7 @@ public static void initFX() throws Exception {
static WeakReference<Stage> closedFocusedStageWeak = null;
static Stage closedFocusedStage = null;
This conversation was marked as resolved by FlorianKirmaier

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

These two can be instance fields (which is usually preferred for test variables).

This comment has been minimized.

@FlorianKirmaier

FlorianKirmaier Aug 9, 2020
Author Member

They are called from the following static method:

    @BeforeClass
    public static void initFX() throws Exception {
        initFXBase();
    }

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 13, 2020
Member

No, these two are only used from testClosedFocusedStageLeakBase, which is an instance method. I removed static while testing it and it compiles and runs fine.

This comment has been minimized.

@FlorianKirmaier

FlorianKirmaier Aug 14, 2020
Author Member

You are right, I've probably picked up the other 2 static variables.


@Test
public void testClosedFocusedStageLeak() throws Exception {
public void testClosedFocusedStageLeakBase() throws Exception {
CountDownLatch latch = new CountDownLatch(1);
Util.runAndWait(() -> {
closedFocusedStage = new Stage();
@@ -110,11 +103,4 @@ public static void assertCollectable(WeakReference weakReference) throws Excepti

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

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

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

Minor: Add missing newline

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 13, 2020
Member

Same as in previous file.

This comment has been minimized.

@FlorianKirmaier

FlorianKirmaier Aug 14, 2020
Author Member

done

ProTip! Use n and p to navigate between commits in a pull request.