8331616: ChangeListener is not triggered when the InvalidationListener is removed

Reviewed-by: mstrauss, kcr

Author: hjohn

modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java

@@ -355,6 +355,22 @@ protected void fireValueChangedEvent() {
 
             try {
                 locked = true;
+
+                final T oldValue = currentValue;
+
+                if (curChangeSize > 0) {
+
+                    /*
+                     * Because invalidation listeners may get removed during notification, this may
+                     * change the Helper type from Generic to SingleChange. When this transition
+                     * occurs, it is essential the correct current value is passed to the new
+                     * SingleChange instance. This is why the currentValue is already obtained
+                     * before notifying the invalidation listeners.
+                     */
+
+                    currentValue = observable.getValue();
+                }
+
                 for (int i = 0; i < curInvalidationSize; i++) {
                     try {
                         curInvalidationList[i].invalidated(observable);
@@ -363,8 +379,6 @@ protected void fireValueChangedEvent() {
                     }
                 }
                 if (curChangeSize > 0) {
-                    final T oldValue = currentValue;
-                    currentValue = observable.getValue();
                     final boolean changed = (currentValue == null)? (oldValue != null) : !currentValue.equals(oldValue);
                     if (changed) {
                         for (int i = 0; i < curChangeSize; i++) {

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperTest.java

@@ -682,7 +682,7 @@ public void shouldNotForgetCurrentValueWhenMovingFromGenericToSingleChangeImplem
         StringProperty p = new SimpleStringProperty("a") {
             @Override
             protected void invalidated() {
-                removeListener(invalidationListener);
+                removeListener(invalidationListener);  // this removal occurs before notification
             }
         };
 
@@ -693,6 +693,56 @@ protected void invalidated() {
 
         assertFalse(invalidated.get());  // false because the invalidation listener was removed before called
         assertEquals("b", currentValue.get());
+
+        p.set("a");  // if current value wasn't copied correctly (it is still "a") then this wouldn't trigger a change
+
+        assertEquals("a", currentValue.get());
+    }
+
+    @Test
+    public void shouldNotForgetCurrentValueWhenMovingFromChangeListenerAndInvalidationListenerToSingleChangeListener() {
+        AtomicReference<String> currentValue = new AtomicReference<>();
+        StringProperty p = new SimpleStringProperty("a");
+        InvalidationListener invalidationListener = new InvalidationListener() {
+            @Override
+            public void invalidated(Observable obs) {
+                p.removeListener(this);  // this removal occurs during notification
+            }
+        };
+
+        p.addListener(invalidationListener);
+        p.addListener((obs, old, current) -> currentValue.set(current));
+
+        p.set("b");
+
+        assertEquals("b", currentValue.get());
+
+        p.set("a");  // if current value wasn't copied correctly (it is still "a") then this wouldn't trigger a change
+
+        assertEquals("a", currentValue.get());
+    }
+
+    @Test
+    public void shouldNotForgetCurrentValueWhenMovingFromTwoChangeListenersToSingleChangeListener() {
+        AtomicReference<String> currentValue = new AtomicReference<>();
+        StringProperty p = new SimpleStringProperty("a");
+        ChangeListener<String> changeListener = new ChangeListener<>() {
+            @Override
+            public void changed(ObservableValue<? extends String> observable, String oldValue, String newValue) {
+                p.removeListener(this);
+            }
+        };
+
+        p.addListener(changeListener);
+        p.addListener((obs, old, current) -> currentValue.set(current));
+
+        p.set("b");
+
+        assertEquals("b", currentValue.get());
+
+        p.set("a");  // if current value wasn't copied correctly (it is still "a") then this wouldn't trigger a change
+
+        assertEquals("a", currentValue.get());
     }
 
     @Test