changeset 7591:86ffa7810b12

RT-38076 Map- and SetExpression invalidation listeners cause eventual NPEs when removing themselves
author Martin Sladecek <martin.sladecek@oracle.com>
date Tue, 29 Jul 2014 15:13:18 +0200
parents df28c1bdf2e1
children c519c8a7ccae
files modules/base/src/main/java/com/sun/javafx/binding/MapExpressionHelper.java modules/base/src/main/java/com/sun/javafx/binding/SetExpressionHelper.java modules/base/src/test/java/javafx/collections/MockMapObserver.java modules/base/src/test/java/javafx/collections/MockSetObserver.java modules/base/src/test/java/javafx/collections/ObservableMapTest.java modules/base/src/test/java/javafx/collections/ObservableSetTest.java
diffstat 6 files changed, 83 insertions(+), 1 deletions(-) [+]
line wrap: on
line diff
--- a/modules/base/src/main/java/com/sun/javafx/binding/MapExpressionHelper.java	Mon Jul 28 19:25:24 2014 +0400
+++ b/modules/base/src/main/java/com/sun/javafx/binding/MapExpressionHelper.java	Tue Jul 29 15:13:18 2014 +0200
@@ -413,6 +413,7 @@
                             if (numMoved > 0) {
                                 System.arraycopy(oldListeners, index+1, invalidationListeners, index, numMoved);
                             }
+                            invalidationSize--;
                             if (!locked) {
                                 invalidationListeners[--invalidationSize] = null; // Let gc do its work
                             }
@@ -472,6 +473,7 @@
                             if (numMoved > 0) {
                                 System.arraycopy(oldListeners, index+1, changeListeners, index, numMoved);
                             }
+                            changeSize--;
                             if (!locked) {
                                 changeListeners[--changeSize] = null; // Let gc do its work
                             }
@@ -531,6 +533,7 @@
                             if (numMoved > 0) {
                                 System.arraycopy(oldListeners, index+1, mapChangeListeners, index, numMoved);
                             }
+                            mapChangeSize--;
                             if (!locked) {
                                 mapChangeListeners[--mapChangeSize] = null; // Let gc do its work
                             }
--- a/modules/base/src/main/java/com/sun/javafx/binding/SetExpressionHelper.java	Mon Jul 28 19:25:24 2014 +0400
+++ b/modules/base/src/main/java/com/sun/javafx/binding/SetExpressionHelper.java	Tue Jul 29 15:13:18 2014 +0200
@@ -404,6 +404,7 @@
                             if (numMoved > 0) {
                                 System.arraycopy(oldListeners, index+1, invalidationListeners, index, numMoved);
                             }
+                            invalidationSize--;
                             if (!locked) {
                                 invalidationListeners[--invalidationSize] = null; // Let gc do its work
                             }
--- a/modules/base/src/test/java/javafx/collections/MockMapObserver.java	Mon Jul 28 19:25:24 2014 +0400
+++ b/modules/base/src/test/java/javafx/collections/MockMapObserver.java	Tue Jul 29 15:13:18 2014 +0200
@@ -47,6 +47,10 @@
         calls.clear();
     }
 
+    public void check0() {
+        assertEquals(0, calls.size());
+    }
+
     public void assertAdded(Tuple<K, V> tuple) {
         assertAdded(0, tuple);
     }
--- a/modules/base/src/test/java/javafx/collections/MockSetObserver.java	Mon Jul 28 19:25:24 2014 +0400
+++ b/modules/base/src/test/java/javafx/collections/MockSetObserver.java	Tue Jul 29 15:13:18 2014 +0200
@@ -50,6 +50,10 @@
         calls.clear();
     }
 
+    public void check0() {
+        assertEquals(0, calls.size());
+    }
+
     public void assertAdded(Tuple<E> tuple) {
         assertAdded(0, tuple);
     }
--- a/modules/base/src/test/java/javafx/collections/ObservableMapTest.java	Mon Jul 28 19:25:24 2014 +0400
+++ b/modules/base/src/test/java/javafx/collections/ObservableMapTest.java	Tue Jul 29 15:13:18 2014 +0200
@@ -37,6 +37,8 @@
 import static javafx.collections.MockMapObserver.Call.call;
 import static javafx.collections.MockMapObserver.Tuple.tup;
 import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
@@ -391,7 +393,39 @@
         assertTrue(observableMap.entrySet().toArray(new Map.Entry[0]).length == 3);
         assertTrue(observableMap.entrySet().toArray().length == 3);
     }
-    
+
+    @Test
+    public void testObserverCanRemoveObservers() {
+        final MapChangeListener<String, String> listObserver = change -> {
+            change.getMap().removeListener(observer);
+        };
+        observableMap.addListener(listObserver);
+        observableMap.put("x", "x");
+        observer.clear();
+        observableMap.put("y", "y");
+        observer.check0();
+        observableMap.removeListener(listObserver);
+
+        final StringMapChangeListener listener = new StringMapChangeListener();
+        observableMap.addListener(listener);
+        observableMap.put("z", "z");
+        assertEquals(listener.counter, 1);
+        observableMap.put("zz", "zz");
+        assertEquals(listener.counter, 1);
+    }
+
+
+    private static class StringMapChangeListener implements MapChangeListener<String, String> {
+
+        private int counter;
+
+        @Override
+        public void onChanged(Change<? extends String, ? extends String> change) {
+            change.getMap().removeListener(this);
+            ++counter;
+        }
+    }
+
     @Test
     public void testEqualsAndHashCode() {
         final Map<String, String> other = new HashMap<>(observableMap);
--- a/modules/base/src/test/java/javafx/collections/ObservableSetTest.java	Mon Jul 28 19:25:24 2014 +0400
+++ b/modules/base/src/test/java/javafx/collections/ObservableSetTest.java	Tue Jul 29 15:13:18 2014 +0200
@@ -37,6 +37,8 @@
 import static javafx.collections.MockSetObserver.Call.*;
 import static javafx.collections.MockSetObserver.Tuple.*;
 import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
@@ -182,6 +184,40 @@
         assertEquals(3, observableSet.size());
         observer.assertRemoved(tup((String)null));
     }
+
+
+    @Test
+    public void testObserverCanRemoveObservers() {
+        final SetChangeListener<String> listObserver = change -> {
+            change.getSet().removeListener(observer);
+        };
+        observableSet.addListener(listObserver);
+        observableSet.add("x");
+        observer.clear();
+        observableSet.add("y");
+        observer.check0();
+        observableSet.removeListener(listObserver);
+
+
+        final StringSetChangeListener listener = new StringSetChangeListener();
+        observableSet.addListener(listener);
+        observableSet.add("z");
+        assertEquals(listener.counter, 1);
+        observableSet.add("zz");
+        assertEquals(listener.counter, 1);
+    }
+
+
+    private static class StringSetChangeListener implements SetChangeListener<String> {
+
+        private int counter;
+
+        @Override
+        public void onChanged(final Change<? extends String> change) {
+            change.getSet().removeListener(this);
+            ++counter;
+        }
+    }
     
     @Test
     public void testEqualsAndHashCode() {