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
Expand Up @@ -351,6 +351,9 @@ public void close() {
if (this.ptr != 0L) {
_close(this.ptr);
}
if(Window.focusedWindow == this) {
FlorianKirmaier marked this conversation as resolved.
Show resolved Hide resolved
Window.focusedWindow = null;
}
}

private boolean isChild() {
Expand Down Expand Up @@ -1322,7 +1325,7 @@ protected void notifyFocus(int event) {

if (this.isFocused != focused) {
this.isFocused = focused;
if (this.isFocused) {
if (this.isFocused && this.isVisible) {
Copy link
Member

@arapte arapte Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my Window10 machine, with this change, Window.focusedWindow remains null even after the first window (I have not verified with multiple windows though) is shown onto the screen and is focused. And It continues to remain null until some mouse or key action is performed on the window.
I am not sure if this causes any side effects. It looks like the Window.focusedWindow is mostly(only) used for Monocle.
Can you please confirm the behavior that Window.focusedWindow remain null and check for any side effects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned - I don't have a good setup to test this code on Windows.

But I've checked where focusedWindow/getFocusedWindow is used, and I can verify your assumption. I've searched through the whole project and the variable is only used in the MonocleCode.

The fact that focusedWindow get's sometimes set is probably the cause of the irregular happening memoryleak on Window.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reading the comments, I thought you were going to revert the changes to Window.java? Or did I misinterpret what you said earlier?

I can confirm that focusedWindow is no longer correctly set to the focused window when that Window is first shown (I tried this on Windows). This is true for apps with multiple windows as well as single Stage apps. I can also confirm that focusedWindow isn't used on any platform other than Monocle. Even so, since this change isn't needed it seems best to revert it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation for this change was, that I've seen a similar bug on native-windows without monocle.
In this case a leak happened related to the focusedWindow variable.
Sadly I don't have a test for this bug, because it is undeterministic.
For that reason, I thought it would make sense to keep this change, because I think it fixes it.
If I remember correctly, it was related to Stages which were focused but closed at the same time, which only weren't collected because of the focusedWindow variable.
Because the variable also isn't used on NativeWindows it doesn't seem to do much harm.

I've now removed it, but it might be considered to be readded.

setFocusedWindow(this);
} else {
setFocusedWindow(null);
Expand Down
Expand Up @@ -121,7 +121,7 @@ boolean maximizeWindow(MonocleWindow window) {

boolean requestFocus(MonocleWindow window) {
int index = getWindowIndex(window);
if (index != -1) {
if (index != -1 && window.isVisible()) {
FlorianKirmaier marked this conversation as resolved.
Show resolved Hide resolved
focusedWindow = window;
window.notifyFocus(WindowEvent.FOCUS_GAINED);
return true;
Expand Down
142 changes: 142 additions & 0 deletions tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java
@@ -0,0 +1,142 @@
/*
* 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.Application;
import javafx.application.Platform;
import javafx.scene.Node;
import javafx.scene.Scene;
import javafx.scene.control.TextField;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;

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

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

public class FocusedWindowTest {

static CountDownLatch startupLatch;
static Stage stage = null;

static {
System.setProperty("glass.platform","Monocle");
System.setProperty("monocle.platform","Headless");
}
FlorianKirmaier marked this conversation as resolved.
Show resolved Hide resolved

public static class TestApp extends Application {

@Override
public void start(Stage primaryStage) throws Exception {
primaryStage.setTitle("Primary Stage");
primaryStage.setScene(new Scene(new TextField()));

primaryStage.setOnShown(l -> {
Platform.runLater(() -> startupLatch.countDown());
});
primaryStage.show();
Platform.setImplicitExit(false);
stage = primaryStage;
}
}

@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));
Platform.runLater(() -> stage.close());
}
FlorianKirmaier marked this conversation as resolved.
Show resolved Hide resolved

@Test
public void testLeak() throws Exception {
int counter = 0;
while(counter <= 100) {
counter += 1;
testLeakOnce();
FlorianKirmaier marked this conversation as resolved.
Show resolved Hide resolved
}
}

static WeakReference<Stage> closedFocusedStageWeak = null;

public void testLeakOnce() throws Exception {
CountDownLatch leakLatch = new CountDownLatch(1);
closedFocusedStageWeak = null;
Platform.runLater(() -> {
Stage closedFocusedStage = new Stage();
closedFocusedStage.setTitle("Focused Stage");
closedFocusedStageWeak = new WeakReference<>(closedFocusedStage);
TextField textField = new TextField();
closedFocusedStage.setScene(new Scene(textField));
Platform.runLater(() -> {
closedFocusedStage.show();
Platform.runLater(() -> {
closedFocusedStage.close();
Platform.runLater(() -> {
closedFocusedStage.requestFocus();
//textField.requestFocus();
Platform.runLater(() -> {
leakLatch.countDown();
});
});
});
});
});

FlorianKirmaier marked this conversation as resolved.
Show resolved Hide resolved
Assert.assertTrue("Timeout, waiting for runLater. ", leakLatch.await(15, TimeUnit.SECONDS));

assertCollectable(closedFocusedStageWeak);

}

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

System.gc();
System.runFinalization();

while (counter < 10 && weakReference.get() != null) {
Thread.sleep(100);
counter = counter + 1;
System.gc();
System.runFinalization();
}

Assert.assertNull(weakReference.get());
FlorianKirmaier marked this conversation as resolved.
Show resolved Hide resolved
}

@AfterClass
public static void teardownOnce() {
Platform.runLater(() -> {
Platform.exit();
});
}
}