RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

[Florian Kirmaier]

On Mon, 15 Mar 2021 18:59:40 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> About whether we should use WeakReference here or not. 
>> 
>> It definitely falls into the exception for referring to the Parrent of a Node. (Or to the Parent in a more abstract sense, I think it can also be the parent of the parent, or even from another SceneGraph.)
>> 
>> I don't know the CSS code very well, so I currently don't have the knowledge how to change it.
>> But if we would change this variable, whenever the node is removed from the SceneGraph, my concern would be that it would have an unfavorable complexity. Currently (I hope) the complexity of removing a Node from the SceneGraph is O(1). If we would remove the stylehelper from the node, then the complexity would be O(<nodes-in-the-tree>).
>> 
>> The current change assumes that we don't process the CSS, when a node is removed from the SG. This is why it isn't checked for null. I would argue, if this causes an Error, then it just reveals another issue, which would be preferable over a more complicated fix, and also changing the complexity of removing nodes from the SG.
>
> Below is a fix that I tried, It fixes the issue. The system test passed with this change.
> I was suggesting a solution like this where we can know exactly when to `null` the reference. The change is not extensively tested though. (For example, test what happens if we remove a node and add it back, OR remove a node and add it to a different scenegraph)
> However, in this case using `WeakReference` approach seems harmless. Using `WeakReference` for Listeners is not clean and may cause issues, but a `WeakReference` to Node should not cause any harm.
> 
> I would recommend earlier way to explicitly release references/resources when it is possible. That way we get to have the control instead of gc.
> 
> 
> diff --git a/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java b/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
> index fd02c0c1ad..471d0071b5 100644
> --- a/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
> +++ b/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
> @@ -36,6 +36,7 @@ import java.util.Set;
>  
>  import javafx.beans.property.ObjectProperty;
>  import javafx.beans.property.SimpleObjectProperty;
> +import javafx.beans.value.ChangeListener;
>  import javafx.beans.value.WritableValue;
>  import com.sun.javafx.css.CascadingStyle;
>  import javafx.css.CssMetaData;
> @@ -82,6 +83,14 @@ final class CssStyleHelper {
>          this.triggerStates = new PseudoClassState();
>      }
>  
> +    ChangeListener<Scene> sceneChangeListener;
> +
> +    static void dispose(CssStyleHelper styleHelper, Node node) {
> +        styleHelper.resetToInitialValues(node);
> +        styleHelper.firstStyleableAncestor = null;
> +        node.sceneProperty().removeListener(styleHelper.sceneChangeListener);
> +    }
> +
>      /**
>       * Creates a new StyleHelper.
>       */
> @@ -158,7 +167,7 @@ final class CssStyleHelper {
>                  // If this node had a style helper, then reset properties to their initial value
>                  // since the node won't have a style helper after this call
>                  if (node.styleHelper != null) {
> -                    node.styleHelper.resetToInitialValues(node);
> +                    dispose(node.styleHelper, node);
>                  }
>  
>                  //
> @@ -181,8 +190,14 @@ final class CssStyleHelper {
>          // If this node had a style helper, then reset properties to their initial value
>          // since the style map might now be different
>          if (node.styleHelper != null) {
> -            node.styleHelper.resetToInitialValues(node);
> +            dispose(node.styleHelper, node);
>          }
> +        helper.sceneChangeListener = (ov, oldScene, newScene) -> {
> +            if (newScene == null) {
> +                helper.firstStyleableAncestor = null;
> +            }
> +        };
> +        node.sceneProperty().addListener(helper.sceneChangeListener);
>          return helper;
>      }

I rewrote now the test. The initialization and teardown of JavaFX are now separated from the actual test. Now also none of the variables which is used in the test, are accessible from outside the test, and vise versa.

Should I switch the fix to your solution, or should I keep mine which is based on WeakReferences? As you've mentioned, WeakReference should be fine here too.
As I've mentioned, doing something for every node, whose scene is set to null, might change the complexity of removing a node from O(1) to O(<size-of-tree>). I think it's also quite important to support fast removing/adding subtrees.

-------------

PR: https://git.openjdk.java.net/jfx/pull/424