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

[Ambarish Rapte]

On Fri, 12 Mar 2021 15:32:10 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> I think others can review this. I do have a couple questions:
>> 1. In general, I don't like the idea of just making everything a weak reference, since it often masks a design issue. Two exceptions are for caches and for back references to a "parent" or controlling object that has a strong reference to "this" object (most of our weak listeners fall into this latter pattern). It sounds like latter case also applies here. Can you confirm that?
>> 2. Have you verified that all the places that use the fields that are now WeakReferences are prepared to deal with `get()` returning a null object?
>
> 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;
     }

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

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