8293444: Creating ScrollPane with same content component causes memory leak

Reviewed-by: kcr, arapte

Author: Andy Goryachev

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java

@@ -36,10 +36,12 @@
 import javafx.animation.Timeline;
 import javafx.beans.InvalidationListener;
 import javafx.beans.Observable;
+import javafx.beans.WeakInvalidationListener;
 import javafx.beans.property.DoubleProperty;
 import javafx.beans.property.DoublePropertyBase;
 import javafx.beans.value.ChangeListener;
 import javafx.beans.value.ObservableValue;
+import javafx.beans.value.WeakChangeListener;
 import javafx.event.EventDispatcher;
 import javafx.event.EventHandler;
 import javafx.geometry.BoundingBox;
@@ -185,6 +187,7 @@ public class ScrollPaneSkin extends SkinBase<ScrollPane> {
         }
     };
 
+    private final WeakInvalidationListener weakNodeListener = new WeakInvalidationListener(nodeListener);
 
     /*
     ** The content of the ScrollPane has just changed bounds, check scrollBar positions.
@@ -242,7 +245,7 @@ else if (newValueX > 1.0) {
         }
     };
 
-
+    private final WeakChangeListener<Bounds> weakBoundsChangeListener = new WeakChangeListener(boundsChangeListener);
 
     /* *************************************************************************
      *                                                                         *
@@ -274,17 +277,17 @@ public ScrollPaneSkin(final ScrollPane control) {
         registerChangeListener(control.contentProperty(), e -> {
             if (scrollNode != getSkinnable().getContent()) {
                 if (scrollNode != null) {
-                    scrollNode.layoutBoundsProperty().removeListener(nodeListener);
-                    scrollNode.layoutBoundsProperty().removeListener(boundsChangeListener);
+                    scrollNode.layoutBoundsProperty().removeListener(weakNodeListener);
+                    scrollNode.layoutBoundsProperty().removeListener(weakBoundsChangeListener);
                     viewContent.getChildren().remove(scrollNode);
                 }
                 scrollNode = getSkinnable().getContent();
                 if (scrollNode != null) {
                     nodeWidth = snapSizeX(scrollNode.getLayoutBounds().getWidth());
                     nodeHeight = snapSizeY(scrollNode.getLayoutBounds().getHeight());
                     viewContent.getChildren().setAll(scrollNode);
-                    scrollNode.layoutBoundsProperty().addListener(nodeListener);
-                    scrollNode.layoutBoundsProperty().addListener(boundsChangeListener);
+                    scrollNode.layoutBoundsProperty().addListener(weakNodeListener);
+                    scrollNode.layoutBoundsProperty().addListener(weakBoundsChangeListener);
                 }
             }
             getSkinnable().requestLayout();
@@ -635,8 +638,8 @@ private void initialize() {
         ParentHelper.setTraversalEngine(getSkinnable(), traversalEngine);
 
         if (scrollNode != null) {
-            scrollNode.layoutBoundsProperty().addListener(nodeListener);
-            scrollNode.layoutBoundsProperty().addListener(boundsChangeListener);
+            scrollNode.layoutBoundsProperty().addListener(weakNodeListener);
+            scrollNode.layoutBoundsProperty().addListener(weakBoundsChangeListener);
         }
 
         viewRect = new StackPane() {

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ScrollPaneSkinTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2022, 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
@@ -28,6 +28,9 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
+import java.lang.ref.WeakReference;
+import java.util.ArrayList;
+
 import javafx.beans.value.ObservableValue;
 import javafx.event.Event;
 import javafx.event.EventType;
@@ -40,7 +43,11 @@
 import javafx.scene.control.ScrollPane;
 import javafx.scene.control.skin.ScrollPaneSkin;
 import javafx.scene.control.skin.ScrollPaneSkinShim;
-import javafx.scene.input.*;
+import javafx.scene.input.MouseButton;
+import javafx.scene.input.MouseEvent;
+import javafx.scene.input.ScrollEvent;
+import javafx.scene.input.SwipeEvent;
+import javafx.scene.layout.BorderPane;
 import javafx.scene.layout.HBox;
 import javafx.scene.layout.Pane;
 import javafx.scene.layout.StackPane;
@@ -52,6 +59,10 @@
 import org.junit.Ignore;
 import org.junit.Test;
 
+import com.sun.javafx.tk.Toolkit;
+
+import test.util.memory.JMemoryBuddy;
+
 
 public class ScrollPaneSkinTest {
     private ScrollPane scrollPane;
@@ -860,4 +871,56 @@ public MouseEvent generateMouseEvent(EventType<MouseEvent> type,
             return event;
         }
     }
+
+    /**
+     * Test that ScrollPane object is not leaked when 'same' node
+     * is used as content for different ScrollPane objects, see JDK-8293444.
+     */
+    @Test
+    public void testScrollPaneObjLeakWhenUsedSameContent() {
+
+        ArrayList<WeakReference<ScrollPane>> refs = new ArrayList<>();
+
+        BorderPane bp = new BorderPane();
+
+        Stage stage = new Stage();
+        stage.setScene(new Scene(bp));
+        stage.show();
+
+        Label content = new Label("content");
+
+        for (int i = 0; i < 10; i++) {
+            ScrollPane sp = new ScrollPane(content);
+            refs.add(new WeakReference<ScrollPane>(sp));
+            bp.setCenter(sp);
+
+            Toolkit.getToolkit().firePulse();
+        }
+
+        bp.setCenter(null);
+        Toolkit.getToolkit().firePulse();
+
+        int ct = 0;
+        for (WeakReference<ScrollPane> ref : refs) {
+            JMemoryBuddy.checkCollectable(ref);
+            if (ref.get() != null) {
+                ct++;
+            }
+        }
+
+        // one instance is still held by the 'content' label
+        assertEquals("One instance should be held by the 'content' label", 1, ct);
+
+        // releasing the last instance
+        content = null;
+
+        ct = 0;
+        for (WeakReference<ScrollPane> ref : refs) {
+            JMemoryBuddy.checkCollectable(ref);
+            if (ref.get() != null) {
+                ct++;
+            }
+        }
+        assertEquals(ct + " references of ScrollPane are not freed.", 0, ct);
+    }
 }