From 35e2a34c9c4dfcf9117db31f6403401129c9befc Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Mon, 14 Apr 2014 10:19:17 +0300 Subject: Fix FieldGroup and TransactionalPropertyWrapper memory leaks (#13438) Change-Id: Ifafb6d87b4280f8bd9e631235fff62f42de4b4c8 --- .../data/util/TransactionalPropertyWrapper.java | 26 +++++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) (limited to 'server/src/com/vaadin/data/util') diff --git a/server/src/com/vaadin/data/util/TransactionalPropertyWrapper.java b/server/src/com/vaadin/data/util/TransactionalPropertyWrapper.java index 6b0119c503..3bac5bdfb8 100644 --- a/server/src/com/vaadin/data/util/TransactionalPropertyWrapper.java +++ b/server/src/com/vaadin/data/util/TransactionalPropertyWrapper.java @@ -48,18 +48,32 @@ public class TransactionalPropertyWrapper extends AbstractProperty private boolean inTransaction = false; private boolean valueChangePending; private T valueBeforeTransaction; + private final ValueChangeListener listener = new ValueChangeListener() { + + @Override + public void valueChange(ValueChangeEvent event) { + fireValueChange(); + } + }; public TransactionalPropertyWrapper(Property wrappedProperty) { this.wrappedProperty = wrappedProperty; if (wrappedProperty instanceof ValueChangeNotifier) { ((ValueChangeNotifier) wrappedProperty) - .addListener(new ValueChangeListener() { + .addValueChangeListener(listener); + } + } - @Override - public void valueChange(ValueChangeEvent event) { - fireValueChange(); - } - }); + /** + * Removes the ValueChangeListener from wrapped Property that was added by + * TransactionalPropertyWrapper. + * + * @since 7.1.14 + */ + public void detachFromProperty() { + if (wrappedProperty instanceof ValueChangeNotifier) { + ((ValueChangeNotifier) wrappedProperty) + .removeValueChangeListener(listener); } } -- cgit v1.2.3 From 6e91bdf4224d5e95cf1746775855c13c25e9b82c Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Mon, 14 Apr 2014 15:56:05 +0300 Subject: Add test for TransactionalPropertyWrapper memory leaks Change-Id: I69d0d759f95100f1dd9e2dbba57ec2c246e3aca9 --- .../data/util/TransactionalPropertyWrapper.java | 2 +- .../util/TransactionalPropertyWrapperTest.java | 116 +++++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 server/tests/src/com/vaadin/data/util/TransactionalPropertyWrapperTest.java (limited to 'server/src/com/vaadin/data/util') diff --git a/server/src/com/vaadin/data/util/TransactionalPropertyWrapper.java b/server/src/com/vaadin/data/util/TransactionalPropertyWrapper.java index 3bac5bdfb8..3c52ab9afc 100644 --- a/server/src/com/vaadin/data/util/TransactionalPropertyWrapper.java +++ b/server/src/com/vaadin/data/util/TransactionalPropertyWrapper.java @@ -68,7 +68,7 @@ public class TransactionalPropertyWrapper extends AbstractProperty * Removes the ValueChangeListener from wrapped Property that was added by * TransactionalPropertyWrapper. * - * @since 7.1.14 + * @since 7.1.15 */ public void detachFromProperty() { if (wrappedProperty instanceof ValueChangeNotifier) { diff --git a/server/tests/src/com/vaadin/data/util/TransactionalPropertyWrapperTest.java b/server/tests/src/com/vaadin/data/util/TransactionalPropertyWrapperTest.java new file mode 100644 index 0000000000..8e83db5aef --- /dev/null +++ b/server/tests/src/com/vaadin/data/util/TransactionalPropertyWrapperTest.java @@ -0,0 +1,116 @@ +/* + * Copyright 2000-2013 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.data.util; + +import static org.junit.Assert.assertTrue; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.Test; + +import com.vaadin.data.fieldgroup.FieldGroup; +import com.vaadin.ui.TextField; + +/** + * Test verifying that TransactionalPropertyWrapper removes it's listener from + * wrapped Property + * + * @since 7.1.15 + * @author Vaadin Ltd + */ +public class TransactionalPropertyWrapperTest { + + @SuppressWarnings("serial") + public class TestingProperty extends + ObjectProperty { + + private List listeners = new ArrayList(); + + public TestingProperty(Object value) { + super(value); + } + + @Override + public void addValueChangeListener(ValueChangeListener listener) { + super.addValueChangeListener(listener); + listeners.add(listener); + } + + @Override + public void removeValueChangeListener(ValueChangeListener listener) { + super.removeValueChangeListener(listener); + if (listeners.contains(listener)) { + listeners.remove(listener); + } + } + + public boolean hasListeners() { + return !listeners.isEmpty(); + } + } + + private final TextField nameField = new TextField("Name"); + private final TextField ageField = new TextField("Age"); + private final TextField unboundField = new TextField("No FieldGroup"); + private final TestingProperty unboundProp = new TestingProperty( + "Hello World"); + private final PropertysetItem item = new PropertysetItem(); + + @Test + public void fieldGroupBindAndUnbind() { + item.addItemProperty("name", new TestingProperty( + "Just some text")); + item.addItemProperty("age", new TestingProperty("42")); + + final FieldGroup binder = new FieldGroup(item); + binder.setBuffered(false); + + for (int i = 0; i < 2; ++i) { + binder.bind(nameField, "name"); + binder.bind(ageField, "age"); + unboundField.setPropertyDataSource(unboundProp); + + assertTrue("No listeners in Properties", fieldsHaveListeners(true)); + + binder.unbind(nameField); + binder.unbind(ageField); + unboundField.setPropertyDataSource(null); + + assertTrue("Listeners in Properties after unbinding", + fieldsHaveListeners(false)); + } + } + + /** + * Check that all listeners have same hasListeners() response + * + * @param expected + * expected response + * @return true if all are the same as expected. false if not + */ + private boolean fieldsHaveListeners(boolean expected) { + for (Object id : item.getItemPropertyIds()) { + TestingProperty itemProperty = (TestingProperty) item + .getItemProperty(id); + + if (itemProperty.hasListeners() != expected) { + return false; + } + } + return unboundProp.hasListeners() == expected; + } +} -- cgit v1.2.3