Skip to content
Permalink
Browse files
8263402: MemoryLeak: Node hardreferences it's previous Parent after c…
…sslayout and getting removed from the scene

Reviewed-by: arapte, kcr
  • Loading branch information
FlorianKirmaier authored and nlisker committed Apr 27, 2021
1 parent 483f171 commit 33bbf3f863cc5d22d0a454ab7a3d9cfbcfaf8fdc
@@ -24,6 +24,7 @@
*/
package javafx.scene;

import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
@@ -176,7 +177,7 @@ static CssStyleHelper createStyleHelper(final Node node) {

helper.cacheContainer = new CacheContainer(node, styleMap, depth);

helper.firstStyleableAncestor = findFirstStyleableAncestor(node);
helper.firstStyleableAncestor = new WeakReference<>(findFirstStyleableAncestor(node));

// If this node had a style helper, then reset properties to their initial value
// since the style map might now be different
@@ -209,7 +210,7 @@ private static void updateParentTriggerStates(Styleable styleable, int depth, Ps
// TODO : check why calling createStyleHelper(parentNode) does not work here?
if (parentNode.styleHelper == null) {
parentNode.styleHelper = new CssStyleHelper();
parentNode.styleHelper.firstStyleableAncestor = findFirstStyleableAncestor(parentNode) ;
parentNode.styleHelper.firstStyleableAncestor = new WeakReference(findFirstStyleableAncestor(parentNode)) ;
}
parentNode.styleHelper.triggerStates.addAll(triggerState);

@@ -232,8 +233,8 @@ private boolean isUserSetFont(Styleable node) {
if (fontStyleableProperty != null && fontStyleableProperty.getStyleOrigin() == StyleOrigin.USER) return true;
}

Styleable styleableParent = firstStyleableAncestor;
CssStyleHelper parentStyleHelper = getStyleHelper(firstStyleableAncestor);
Styleable styleableParent = firstStyleableAncestor.get();
CssStyleHelper parentStyleHelper = getStyleHelper(firstStyleableAncestor.get());

if (parentStyleHelper != null) {
return parentStyleHelper.isUserSetFont(styleableParent);
@@ -300,7 +301,7 @@ private static boolean canReuseStyleHelper(final Node node, final StyleMap style
}

//update ancestor since this node may have changed positions in the scene graph (JDK-8237469)
node.styleHelper.firstStyleableAncestor = findFirstStyleableAncestor(node);
node.styleHelper.firstStyleableAncestor = new WeakReference<>(findFirstStyleableAncestor(node));

// If the style maps are the same instance, we can re-use the current styleHelper if the cacheContainer is null.
// Under this condition, there are no styles for this node _and_ no styles inherit.
@@ -322,7 +323,7 @@ private static boolean canReuseStyleHelper(final Node node, final StyleMap style
return true;
}

CssStyleHelper parentHelper = getStyleHelper(node.styleHelper.firstStyleableAncestor);
CssStyleHelper parentHelper = getStyleHelper(node.styleHelper.firstStyleableAncestor.get());

if (parentHelper != null && parentHelper.cacheContainer != null) {

@@ -349,9 +350,11 @@ private static boolean canReuseStyleHelper(final Node node, final StyleMap style
return false;
}

private static final WeakReference<Node> EMPTY_NODE = new WeakReference<>(null);

/* This is the first Styleable parent (of Node this StyleHelper belongs to)
* having a valid StyleHelper */
private Node firstStyleableAncestor;
private WeakReference<Node> firstStyleableAncestor = EMPTY_NODE;

private CacheContainer cacheContainer;

@@ -1221,7 +1224,7 @@ private CascadingStyle getInheritedStyle(
final Styleable styleable,
final String property) {

Styleable parent = ((Node)styleable).styleHelper.firstStyleableAncestor;
Styleable parent = ((Node)styleable).styleHelper.firstStyleableAncestor.get();
CssStyleHelper parentStyleHelper = getStyleHelper((Node) parent);

if (parent != null && parentStyleHelper != null) {
@@ -1266,7 +1269,7 @@ private CascadingStyle resolveRef(final Styleable styleable, final String proper
} else {
// TODO: This block was copied from inherit. Both should use same code somehow.

Styleable styleableParent = ((Node)styleable).styleHelper.firstStyleableAncestor;
Styleable styleableParent = ((Node)styleable).styleHelper.firstStyleableAncestor.get();
CssStyleHelper parentStyleHelper = getStyleHelper((Node) styleableParent);

if (styleableParent == null || parentStyleHelper == null) {
@@ -0,0 +1,97 @@
/*
* Copyright (c) 2021, 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;

import javafx.application.Platform;
import javafx.scene.Group;
import javafx.scene.control.Button;
import javafx.scene.Scene;
import javafx.stage.Stage;
import junit.framework.Assert;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import test.util.Util;
import test.util.memory.JMemoryBuddy;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;

import static org.junit.Assert.fail;
import static org.junit.Assert.assertTrue;


public class StyleMemoryLeakTest {

@BeforeClass
public static void initFX() throws Exception {
CountDownLatch startupLatch = new CountDownLatch(1);
Platform.startup(() -> {
Platform.setImplicitExit(false);
startupLatch.countDown();
});
assertTrue("Timeout waiting for FX runtime to start", startupLatch.await(15, TimeUnit.SECONDS));
}

@Test
public void testRootNodeMemoryLeak() throws Exception {
JMemoryBuddy.memoryTest((checker) -> {
CountDownLatch showingLatch = new CountDownLatch(1);

Button toBeRemoved = new Button();
Group root = new Group();
AtomicReference<Stage> stage = new AtomicReference<>();

Util.runAndWait(() -> {
stage.set(new Stage());
stage.get().setOnShown(l -> {
Platform.runLater(() -> showingLatch.countDown());
});
stage.get().setScene(new Scene(root));
stage.get().show();
});

try {
assertTrue("Timeout waiting test stage", showingLatch.await(15, TimeUnit.SECONDS));
} catch (InterruptedException e) {
throw new RuntimeException(e);
}

Util.runAndWait(() -> {
root.getChildren().clear();
stage.get().hide();
});

checker.assertCollectable(stage.get());
checker.setAsReferenced(toBeRemoved);
});
}

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

1 comment on commit 33bbf3f

@openjdk-notifier

This comment has been minimized.

Copy link

@openjdk-notifier openjdk-notifier bot commented on 33bbf3f Apr 27, 2021

Please sign in to comment.