changeset 9225:79e7a767edc3

8130458: Memory leak in bind/unbind of [Primitive]PropertyBase Reviewed-by: ckyang
author vadim
date Wed, 19 Aug 2015 13:46:38 +0300
parents 155ad1e34c44
children 83440aabf7af
files modules/base/src/main/java/javafx/beans/property/BooleanPropertyBase.java modules/base/src/main/java/javafx/beans/property/DoublePropertyBase.java modules/base/src/main/java/javafx/beans/property/FloatPropertyBase.java modules/base/src/main/java/javafx/beans/property/IntegerPropertyBase.java modules/base/src/main/java/javafx/beans/property/LongPropertyBase.java modules/base/src/test/java/javafx/beans/property/PropertyBaseTest.java
diffstat 6 files changed, 136 insertions(+), 45 deletions(-) [+]
line wrap: on
line diff
--- a/modules/base/src/main/java/javafx/beans/property/BooleanPropertyBase.java	Wed Aug 19 13:46:27 2015 +0300
+++ b/modules/base/src/main/java/javafx/beans/property/BooleanPropertyBase.java	Wed Aug 19 13:46:38 2015 +0300
@@ -165,17 +165,7 @@
         }
 
         final ObservableBooleanValue newObservable = (rawObservable instanceof ObservableBooleanValue) ? (ObservableBooleanValue) rawObservable
-                : new BooleanBinding() {
-                    {
-                        super.bind(rawObservable);
-                    }
-
-                    @Override
-                    protected boolean computeValue() {
-                        final Boolean value = rawObservable.getValue();
-                        return (value == null)? false : value;
-                    }
-                };
+                : new ValueWrapper(rawObservable);
 
         if (!newObservable.equals(observable)) {
             unbind();
@@ -196,6 +186,9 @@
         if (observable != null) {
             value = observable.get();
             observable.removeListener(listener);
+            if (observable instanceof ValueWrapper) {
+                ((ValueWrapper)observable).dispose();
+            }
             observable = null;
         }
     }
@@ -252,4 +245,24 @@
             return wref.get() == null;
         }
     }
+
+    private class ValueWrapper extends BooleanBinding {
+        private ObservableValue<? extends Boolean> observable;
+
+        public ValueWrapper(ObservableValue<? extends Boolean> observable) {
+            this.observable = observable;
+            bind(observable);
+        }
+
+        @Override
+        protected boolean computeValue() {
+            final Boolean value = observable.getValue();
+            return (value == null) ? false : value;
+        }
+
+        @Override
+        public void dispose() {
+            unbind(observable);
+        }        
+    }
 }
--- a/modules/base/src/main/java/javafx/beans/property/DoublePropertyBase.java	Wed Aug 19 13:46:27 2015 +0300
+++ b/modules/base/src/main/java/javafx/beans/property/DoublePropertyBase.java	Wed Aug 19 13:46:38 2015 +0300
@@ -171,10 +171,7 @@
             newObservable = (ObservableDoubleValue)rawObservable;
         } else if (rawObservable instanceof ObservableNumberValue) {
             final ObservableNumberValue numberValue = (ObservableNumberValue)rawObservable;
-            newObservable = new DoubleBinding() {
-                {
-                    super.bind(rawObservable);
-                }
+            newObservable = new ValueWrapper(rawObservable) {
 
                 @Override
                 protected double computeValue() {
@@ -182,10 +179,7 @@
                 }
             };
         } else {
-            newObservable = new DoubleBinding() {
-                {
-                    super.bind(rawObservable);
-                }
+            newObservable = new ValueWrapper(rawObservable) {
 
                 @Override
                 protected double computeValue() {
@@ -214,6 +208,9 @@
         if (observable != null) {
             value = observable.get();
             observable.removeListener(listener);
+            if (observable instanceof ValueWrapper) {
+                ((ValueWrapper)observable).dispose();
+            }
             observable = null;
         }
     }
@@ -270,4 +267,19 @@
             return wref.get() == null;
         }
     }
+
+    private abstract class ValueWrapper extends DoubleBinding {
+
+        private ObservableValue<? extends Number> observable;
+
+        public ValueWrapper(ObservableValue<? extends Number> observable) {
+            this.observable = observable;
+            bind(observable);
+        }
+
+        @Override
+        public void dispose() {
+            unbind(observable);
+        }
+    }
 }
--- a/modules/base/src/main/java/javafx/beans/property/FloatPropertyBase.java	Wed Aug 19 13:46:27 2015 +0300
+++ b/modules/base/src/main/java/javafx/beans/property/FloatPropertyBase.java	Wed Aug 19 13:46:38 2015 +0300
@@ -171,10 +171,7 @@
             newObservable = (ObservableFloatValue)rawObservable;
         } else if (rawObservable instanceof ObservableNumberValue) {
             final ObservableNumberValue numberValue = (ObservableNumberValue)rawObservable;
-            newObservable = new FloatBinding() {
-                {
-                    super.bind(rawObservable);
-                }
+            newObservable = new ValueWrapper(rawObservable) {
 
                 @Override
                 protected float computeValue() {
@@ -182,10 +179,7 @@
                 }
             };
         } else {
-            newObservable = new FloatBinding() {
-                {
-                    super.bind(rawObservable);
-                }
+            newObservable = new ValueWrapper(rawObservable) {
 
                 @Override
                 protected float computeValue() {
@@ -215,6 +209,9 @@
         if (observable != null) {
             value = observable.get();
             observable.removeListener(listener);
+            if (observable instanceof ValueWrapper) {
+                ((ValueWrapper)observable).dispose();
+            }
             observable = null;
         }
     }
@@ -271,4 +268,19 @@
             return wref.get() == null;
         }
     }
+
+    private abstract class ValueWrapper extends FloatBinding {
+
+        private ObservableValue<? extends Number> observable;
+
+        public ValueWrapper(ObservableValue<? extends Number> observable) {
+            this.observable = observable;
+            bind(observable);
+        }
+
+        @Override
+        public void dispose() {
+            unbind(observable);
+        }
+    }
 }
--- a/modules/base/src/main/java/javafx/beans/property/IntegerPropertyBase.java	Wed Aug 19 13:46:27 2015 +0300
+++ b/modules/base/src/main/java/javafx/beans/property/IntegerPropertyBase.java	Wed Aug 19 13:46:38 2015 +0300
@@ -171,10 +171,7 @@
             newObservable = (ObservableIntegerValue)rawObservable;
         } else if (rawObservable instanceof ObservableNumberValue) {
             final ObservableNumberValue numberValue = (ObservableNumberValue)rawObservable;
-            newObservable = new IntegerBinding() {
-                {
-                    super.bind(rawObservable);
-                }
+            newObservable = new ValueWrapper(rawObservable) {
 
                 @Override
                 protected int computeValue() {
@@ -182,10 +179,7 @@
                 }
             };
         } else {
-            newObservable = new IntegerBinding() {
-                {
-                    super.bind(rawObservable);
-                }
+            newObservable = new ValueWrapper(rawObservable) {
 
                 @Override
                 protected int computeValue() {
@@ -214,6 +208,9 @@
         if (observable != null) {
             value = observable.get();
             observable.removeListener(listener);
+            if (observable instanceof ValueWrapper) {
+                ((ValueWrapper)observable).dispose();
+            }
             observable = null;
         }
     }
@@ -270,4 +267,19 @@
             return wref.get() == null;
         }
     }
+
+    private abstract class ValueWrapper extends IntegerBinding {
+
+        private ObservableValue<? extends Number> observable;
+
+        public ValueWrapper(ObservableValue<? extends Number> observable) {
+            this.observable = observable;
+            bind(observable);
+        }
+
+        @Override
+        public void dispose() {
+            unbind(observable);
+        }
+    }
 }
--- a/modules/base/src/main/java/javafx/beans/property/LongPropertyBase.java	Wed Aug 19 13:46:27 2015 +0300
+++ b/modules/base/src/main/java/javafx/beans/property/LongPropertyBase.java	Wed Aug 19 13:46:38 2015 +0300
@@ -171,10 +171,7 @@
             newObservable = (ObservableLongValue)rawObservable;
         } else if (rawObservable instanceof ObservableNumberValue) {
             final ObservableNumberValue numberValue = (ObservableNumberValue)rawObservable;
-            newObservable = new LongBinding() {
-                {
-                    super.bind(rawObservable);
-                }
+            newObservable = new ValueWrapper(rawObservable) {
 
                 @Override
                 protected long computeValue() {
@@ -182,10 +179,7 @@
                 }
             };
         } else {
-            newObservable = new LongBinding() {
-                {
-                    super.bind(rawObservable);
-                }
+            newObservable = new ValueWrapper(rawObservable) {
 
                 @Override
                 protected long computeValue() {
@@ -214,6 +208,9 @@
         if (observable != null) {
             value = observable.get();
             observable.removeListener(listener);
+            if (observable instanceof ValueWrapper) {
+                ((ValueWrapper)observable).dispose();
+            }
             observable = null;
         }
     }
@@ -270,4 +267,19 @@
             return wref.get() == null;
         }
     }
+
+    private abstract class ValueWrapper extends LongBinding {
+
+        private ObservableValue<? extends Number> observable;
+
+        public ValueWrapper(ObservableValue<? extends Number> observable) {
+            this.observable = observable;
+            bind(observable);
+        }
+
+        @Override
+        public void dispose() {
+            unbind(observable);
+        }
+    }
 }
--- a/modules/base/src/test/java/javafx/beans/property/PropertyBaseTest.java	Wed Aug 19 13:46:27 2015 +0300
+++ b/modules/base/src/test/java/javafx/beans/property/PropertyBaseTest.java	Wed Aug 19 13:46:38 2015 +0300
@@ -28,9 +28,11 @@
 import com.sun.javafx.binding.ExpressionHelperUtility;
 import java.util.Arrays;
 import java.util.List;
+import javafx.beans.value.ObservableNumberValue;
+import javafx.beans.value.ObservableValue;
+import javafx.beans.value.ObservableValueBase;
 import static org.junit.Assert.assertEquals;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -70,6 +72,31 @@
         }
     }
 
+    private static class NumberPropertyMock extends ObservableValueBase<Number>
+            implements ObservableNumberValue, Property<Number>
+    {
+        private Number value = 0;
+
+        @Override public int intValue()       { return value.intValue(); }
+        @Override public long longValue()     { return value.longValue(); }
+        @Override public float floatValue()   { return value.floatValue(); }
+        @Override public double doubleValue() { return value.doubleValue(); }
+        @Override public Number getValue()    { return value; }
+        @Override public void setValue(Number value) {
+            this.value = value;
+            fireValueChangedEvent();
+        }
+
+        @Override public void bind(ObservableValue<? extends Number> observable) {}
+        @Override public void unbind() {}
+        @Override public boolean isBound() { return false; }
+        @Override public void bindBidirectional(Property<Number> other) {}
+        @Override public void unbindBidirectional(Property<Number> other) {}
+
+        @Override public Object getBean() { return null; }
+        @Override public String getName() { return ""; }
+    }
+
     @Parameterized.Parameters
     public static List<Object[]> data() {
         return Arrays.asList(new Object[][] {
@@ -84,9 +111,13 @@
             // Property->Listener->Binding->BindingHelperObserver->Value
             { new Factory(() -> new SimpleBooleanProperty(), () -> new SimpleObjectProperty<>(), true) },
             { new Factory(() -> new SimpleDoubleProperty(), () -> new SimpleObjectProperty<>(), 1.0) },
+            { new Factory(() -> new SimpleDoubleProperty(), () -> new NumberPropertyMock(), 1.0) },
             { new Factory(() -> new SimpleFloatProperty(), () -> new SimpleObjectProperty<>(), 1.0f) },
+            { new Factory(() -> new SimpleFloatProperty(), () -> new NumberPropertyMock(), 1.0f) },
             { new Factory(() -> new SimpleIntegerProperty(), () -> new SimpleObjectProperty<>(), 1) },
+            { new Factory(() -> new SimpleIntegerProperty(), () -> new NumberPropertyMock(), 1) },
             { new Factory(() -> new SimpleLongProperty(), () -> new SimpleObjectProperty<>(), 1L) },
+            { new Factory(() -> new SimpleLongProperty(), () -> new NumberPropertyMock(), 1L) },
             // generic
             // Property->Listener->Value
             { new Factory(() -> new SimpleObjectProperty(), () -> new SimpleObjectProperty<>(), new Object()) },
@@ -140,7 +171,6 @@
         assertEquals(1, ExpressionHelperUtility.getInvalidationListeners(observable).size());
     }
 
-    @Ignore("8130458")
     @Test
     public void testUnbindGenericWrapper() {
         property.bind(observable);