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

[Kevin Rushforth]

On Mon, 22 Mar 2021 14:35:58 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> 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.

I think the fix based on `WeakReference` is fine in this case, for the reasons discussed (it is a back link to an ancestor node) and given the performance concerns. I'd like Ambarish to comment as well.

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

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