8214699: Node.getPseudoClassStates must return the same instance on every call

Reviewed-by: fastegal, kcr

Author: arapte

modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java

@@ -654,23 +654,18 @@ public void defaultConverterCanHandleIncorrectType_2() {
         assertTrue(pseudoClassStates.size() >= 0);
 
         comboBox.setEditable(true);
-        pseudoClassStates = comboBox.getPseudoClassStates();
         assertTrue(pseudoClassStates.contains(PseudoClass.getPseudoClass("editable")));
 
         comboBox.setEditable(false);
-        pseudoClassStates = comboBox.getPseudoClassStates();
-        assertTrue(pseudoClassStates.contains(PseudoClass.getPseudoClass("editable")) == false);
+        assertFalse(pseudoClassStates.contains(PseudoClass.getPseudoClass("editable")));
 
         comboBox.show();
-        pseudoClassStates = comboBox.getPseudoClassStates();
         assertTrue(pseudoClassStates.contains(PseudoClass.getPseudoClass("showing")));
 
         comboBox.hide();
-        pseudoClassStates = comboBox.getPseudoClassStates();
-        assertTrue(pseudoClassStates.contains(PseudoClass.getPseudoClass("showing")) == false);
+        assertFalse(pseudoClassStates.contains(PseudoClass.getPseudoClass("showing")));
 
         comboBox.arm();
-        pseudoClassStates = comboBox.getPseudoClassStates();
         assertTrue(pseudoClassStates.contains(PseudoClass.getPseudoClass("armed")));
 
     }

modules/javafx.graphics/src/main/java/javafx/scene/Node.java

@@ -9367,14 +9367,15 @@ public final void pseudoClassStateChanged(PseudoClass pseudoClass, boolean activ
 
     // package so that StyleHelper can get at it
     final ObservableSet<PseudoClass> pseudoClassStates = new PseudoClassState();
+    private final ObservableSet<PseudoClass> unmodifiablePseudoClassStates =
+            FXCollections.unmodifiableObservableSet(pseudoClassStates);
     /**
      * @return The active pseudo-class states of this Node, wrapped in an unmodifiable ObservableSet
      * @since JavaFX 8.0
      */
+    @Override
     public final ObservableSet<PseudoClass> getPseudoClassStates() {
-
-        return FXCollections.unmodifiableObservableSet(pseudoClassStates);
-
+        return unmodifiablePseudoClassStates;
     }
 
     // Walks up the tree telling each parent that the pseudo class state of

modules/javafx.graphics/src/shims/java/javafx/scene/NodeShim.java

@@ -28,6 +28,8 @@
 import com.sun.javafx.scene.DirtyBits;
 import com.sun.javafx.sg.prism.NGNode;
 
+import javafx.collections.ObservableSet;
+import javafx.css.PseudoClass;
 import javafx.scene.transform.Transform;
 
 public class NodeShim {
@@ -75,4 +77,8 @@ public static void updateBounds(Node n) {
     public static <P extends NGNode> P getPeer(Node n) {
         return n.getPeer();
     }
+
+    public static ObservableSet<PseudoClass> pseudoClassStates(Node n) {
+        return n.pseudoClassStates;
+    }
 }

modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java

@@ -44,7 +44,10 @@
 import test.com.sun.javafx.test.objects.TestStage;
 import com.sun.javafx.tk.Toolkit;
 import com.sun.javafx.util.Utils;
+import javafx.beans.InvalidationListener;
 import javafx.beans.property.*;
+import javafx.collections.SetChangeListener;
+import javafx.css.PseudoClass;
 import javafx.geometry.BoundingBox;
 import javafx.geometry.Bounds;
 import javafx.geometry.NodeOrientation;
@@ -59,8 +62,11 @@
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 
+import java.lang.ref.WeakReference;
 import java.lang.reflect.Method;
 import java.util.Comparator;
+import java.util.Set;
+
 import javafx.scene.Group;
 import javafx.scene.GroupShim;
 import javafx.scene.Node;
@@ -161,6 +167,58 @@ public class NodeTest {
      *                                                                         *
      **************************************************************************/
 
+    @Test
+    public void testGetPseudoClassStatesShouldReturnSameSet() {
+        Rectangle node = new Rectangle();
+        Set<PseudoClass> set1 = node.getPseudoClassStates();
+        Set<PseudoClass> set2 = node.getPseudoClassStates();
+        assertSame("getPseudoClassStates() should always return the same instance",
+                set1, set2);
+    }
+
+    @Test(expected=UnsupportedOperationException.class)
+    public void testPseudoClassStatesIsUnmodifiable() {
+        Node node = new Rectangle();
+        node.getPseudoClassStates().add(PseudoClass.getPseudoClass("dummy"));
+    }
+
+    @Test
+    public void testUnmodifiablePseudoClassStatesEqualsBackingStates() {
+        Rectangle node = new Rectangle();
+        PseudoClass pseudo = PseudoClass.getPseudoClass("Pseudo");
+        node.pseudoClassStateChanged(pseudo, true);
+        assertEquals(1, node.getPseudoClassStates().size());
+        assertEquals(NodeShim.pseudoClassStates(node).size(), node.getPseudoClassStates().size());
+        assertTrue(NodeShim.pseudoClassStates(node).contains(pseudo));
+        assertTrue(node.getPseudoClassStates().contains(pseudo));
+    }
+
+    private boolean isInvalidationListenerInvoked;
+    private boolean isChangeListenerInvoked;
+    @Test
+    public void testPseudoClassStatesListenersAreInvoked() {
+        Rectangle node = new Rectangle();
+        node.getPseudoClassStates().addListener((InvalidationListener) inv -> {
+            isInvalidationListenerInvoked = true;
+        });
+        node.getPseudoClassStates().addListener((SetChangeListener<PseudoClass>) c -> {
+            isChangeListenerInvoked = true;
+        });
+
+        PseudoClass pseudo = PseudoClass.getPseudoClass("Pseudo");
+        node.pseudoClassStateChanged(pseudo, true);
+        assertTrue(isInvalidationListenerInvoked);
+        assertTrue(isChangeListenerInvoked);
+    }
+
+    @Test
+    public void testPseudoClassStatesNotGCed() {
+        Node node = new Rectangle();
+        WeakReference<Set<?>> weakRef = new WeakReference<>(node.getPseudoClassStates());
+        TestUtils.attemptGC(weakRef);
+        assertNotNull("pseudoClassStates must not be gc'ed", weakRef.get());
+    }
+
 // TODO disable this because it depends on TestNode
 //    @Test public void testPeerNotifiedOfVisibilityChanges() {
 //        Rectangle rect = new Rectangle();

modules/javafx.graphics/src/test/java/test/javafx/scene/shape/TestUtils.java

@@ -30,6 +30,7 @@
 import com.sun.prism.paint.Color;
 import javafx.scene.Node;
 
+import java.lang.ref.WeakReference;
 import java.lang.reflect.Method;
 
 import static org.junit.Assert.*;
@@ -126,4 +127,20 @@ public static Object getObjectValue(Node node, String pgPropertyName, boolean is
     public static Object getObjectValue(Node node, String pgPropertyName) throws Exception {
         return getObjectValue(node, pgPropertyName, false);
     }
+
+    public static void attemptGC(WeakReference<?> weakRef) {
+        for (int i = 0; i < 10; i++) {
+            System.gc();
+            System.runFinalization();
+
+            if (weakRef.get() == null) {
+                break;
+            }
+            try {
+                Thread.sleep(50);
+            } catch (InterruptedException e) {
+                fail("InterruptedException occurred during Thread.sleep()");
+            }
+        }
+    }
 }