Fixed #6155: stop listening to Property value change and read only status change events on detach to avoid a potential memory leak, resume listening on attach

svn changeset:23406/svn branch:6.8

2 files changed, 113 insertions, 20 deletions

diff --git a/src/com/vaadin/ui/AbstractField.java b/src/com/vaadin/ui/AbstractField.java
index 9ea9bbba2e..ac3e14b2b6 100644
--- a/src/com/vaadin/ui/AbstractField.java
+++ b/src/com/vaadin/ui/AbstractField.java
@@ -139,6 +139,11 @@ public abstract class AbstractField extends AbstractComponent implements Field,
 
     private boolean valueWasModifiedByDataSourceDuringCommit;
 
+    /**
+     * Whether this field currently listens to Property events.
+     */
+    private boolean isListening = false;
+
     /* Component basics */
 
     /*
@@ -593,18 +598,8 @@ public abstract class AbstractField extends AbstractComponent implements Field,
         // Saves the old value
         final Object oldValue = value;
 
-        // Stops listening the old data source changes
-        if (dataSource != null
-                && Property.ValueChangeNotifier.class
-                        .isAssignableFrom(dataSource.getClass())) {
-            ((Property.ValueChangeNotifier) dataSource).removeListener(this);
-        }
-        if (dataSource != null
-                && Property.ReadOnlyStatusChangeNotifier.class
-                        .isAssignableFrom(dataSource.getClass())) {
-            ((Property.ReadOnlyStatusChangeNotifier) dataSource)
-                    .removeListener(this);
-        }
+        // Stop listening to the old data source
+        removePropertyListeners();
 
         // Sets the new data source
         dataSource = newDataSource;
@@ -622,14 +617,8 @@ public abstract class AbstractField extends AbstractComponent implements Field,
             modified = true;
         }
 
-        // Listens the new data source if possible
-        if (dataSource instanceof Property.ValueChangeNotifier) {
-            ((Property.ValueChangeNotifier) dataSource).addListener(this);
-        }
-        if (dataSource instanceof Property.ReadOnlyStatusChangeNotifier) {
-            ((Property.ReadOnlyStatusChangeNotifier) dataSource)
-                    .addListener(this);
-        }
+        // Listen to new data source if possible
+        addPropertyListeners();
 
         // Copy the validators from the data source
         if (dataSource instanceof Validatable) {
@@ -1121,6 +1110,8 @@ public abstract class AbstractField extends AbstractComponent implements Field,
         if (actionManager != null) {
             actionManager.setViewer(getWindow());
         }
+        // No-op if listeners already registered
+        addPropertyListeners();
     }
 
     @Override
@@ -1129,6 +1120,9 @@ public abstract class AbstractField extends AbstractComponent implements Field,
         if (actionManager != null) {
             actionManager.setViewer((Window) null);
         }
+        // Stop listening to data source events on detach to avoid a potential
+        // memory leak. See #6155.
+        removePropertyListeners();
     }
 
     /**
@@ -1329,4 +1323,31 @@ public abstract class AbstractField extends AbstractComponent implements Field,
             focusable.focus();
         }
     }
+
+    private void addPropertyListeners() {
+        if (!isListening) {
+            if (dataSource instanceof Property.ValueChangeNotifier) {
+                ((Property.ValueChangeNotifier) dataSource).addListener(this);
+            }
+            if (dataSource instanceof Property.ReadOnlyStatusChangeNotifier) {
+                ((Property.ReadOnlyStatusChangeNotifier) dataSource)
+                        .addListener(this);
+            }
+            isListening = true;
+        }
+    }
+
+    private void removePropertyListeners() {
+        if (isListening) {
+            if (dataSource instanceof Property.ValueChangeNotifier) {
+                ((Property.ValueChangeNotifier) dataSource)
+                        .removeListener(this);
+            }
+            if (dataSource instanceof Property.ReadOnlyStatusChangeNotifier) {
+                ((Property.ReadOnlyStatusChangeNotifier) dataSource)
+                        .removeListener(this);
+            }
+            isListening = false;
+        }
+    }
 }
\ No newline at end of file
diff --git a/tests/server-side/com/vaadin/tests/server/component/abstractfield/RemoveListenersOnDetach.java b/tests/server-side/com/vaadin/tests/server/component/abstractfield/RemoveListenersOnDetach.java
new file mode 100644
index 0000000000..32b80e0bcd
--- /dev/null
+++ b/tests/server-side/com/vaadin/tests/server/component/abstractfield/RemoveListenersOnDetach.java
@@ -0,0 +1,72 @@
+package com.vaadin.tests.server.component.abstractfield;
+
+import static org.junit.Assert.assertEquals;
+
+import com.vaadin.data.Property;
+import com.vaadin.data.util.AbstractProperty;
+import com.vaadin.ui.AbstractField;
+
+import org.junit.Test;
+
+public class RemoveListenersOnDetach {
+
+    int numValueChanges = 0;
+    int numReadOnlyChanges = 0;
+
+    AbstractField field = new AbstractField() {
+        @Override
+        public Class<?> getType() {
+            return null;
+        }
+
+        @Override
+        public void valueChange(Property.ValueChangeEvent event) {
+            super.valueChange(event);
+            numValueChanges++;
+        }
+
+        @Override
+        public void readOnlyStatusChange(
+                Property.ReadOnlyStatusChangeEvent event) {
+            super.readOnlyStatusChange(event);
+            numReadOnlyChanges++;
+        }
+    };
+
+    Property property = new AbstractProperty() {
+        public Object getValue() {
+            return null;
+        }
+
+        public void setValue(Object newValue) throws ReadOnlyException,
+                ConversionException {
+            fireValueChange();
+        }
+
+        public Class<?> getType() {
+            return null;
+        }
+    };
+
+    @Test
+    public void testAttachDetach() {
+        field.setPropertyDataSource(property);
+
+        property.setValue(null);
+        property.setReadOnly(true);
+        assertEquals(1, numValueChanges);
+        assertEquals(1, numReadOnlyChanges);
+
+        field.attach();
+        property.setValue(null);
+        property.setReadOnly(false);
+        assertEquals(2, numValueChanges);
+        assertEquals(2, numReadOnlyChanges);
+
+        field.detach();
+        property.setValue(null);
+        property.setReadOnly(true);
+        assertEquals(2, numValueChanges);
+        assertEquals(2, numReadOnlyChanges);
+    }
+}