From 6f21052f2d952fd614c22b11e350e404a40761bb Mon Sep 17 00:00:00 2001 From: Henri Sara Date: Mon, 17 Feb 2014 10:27:37 +0200 Subject: [PATCH] Eliminate spacing memory leak on client (#13315) Change-Id: I432ae4a04d74826dd1cce108177bd503774a8463 --- .../AbstractOrderedLayoutConnector.java | 12 ++--- .../vaadin/client/ui/orderedlayout/Slot.java | 10 +++- .../components/orderedlayout/SpacingLeak.java | 54 +++++++++++++++++++ .../orderedlayout/SpacingLeakTest.java | 39 ++++++++++++++ 4 files changed, 107 insertions(+), 8 deletions(-) create mode 100644 uitest/src/com/vaadin/tests/components/orderedlayout/SpacingLeak.java create mode 100644 uitest/src/com/vaadin/tests/components/orderedlayout/SpacingLeakTest.java diff --git a/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java b/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java index ec4307e50b..8ee1921ed7 100644 --- a/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java +++ b/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java @@ -297,9 +297,9 @@ public abstract class AbstractOrderedLayoutConnector extends // remove spacing as it is exists as separate elements that cannot be // removed easily after reordering the contents - Profiler.enter("AOLC.onConnectorHierarchyChange addOrMoveSlot temporarily remove spacing"); + Profiler.enter("AOLC.onConnectorHierarchyChange temporarily remove spacing"); layout.setSpacing(false); - Profiler.leave("AOLC.onConnectorHierarchyChange addOrMoveSlot temporarily remove spacing"); + Profiler.leave("AOLC.onConnectorHierarchyChange temporarily remove spacing"); for (ComponentConnector child : getChildComponents()) { Profiler.enter("AOLC.onConnectorHierarchyChange add children"); @@ -317,12 +317,12 @@ public abstract class AbstractOrderedLayoutConnector extends } // re-add spacing for the elements that should have it - Profiler.enter("AOLC.onConnectorHierarchyChange addOrMoveSlot setSpacing"); + Profiler.enter("AOLC.onConnectorHierarchyChange setSpacing"); // spacings were removed above if (getState().spacing) { layout.setSpacing(true); } - Profiler.leave("AOLC.onConnectorHierarchyChange addOrMoveSlot setSpacing"); + Profiler.leave("AOLC.onConnectorHierarchyChange setSpacing"); for (ComponentConnector child : previousChildren) { Profiler.enter("AOLC.onConnectorHierarchyChange remove children"); @@ -332,9 +332,7 @@ public abstract class AbstractOrderedLayoutConnector extends if (slot.hasCaption()) { slot.setCaptionResizeListener(null); } - if (slot.getSpacingElement() != null) { - slot.setSpacingResizeListener(null); - } + slot.setSpacingResizeListener(null); child.removeStateChangeHandler(childStateChangeHandler); layout.removeWidget(child.getWidget()); } diff --git a/client/src/com/vaadin/client/ui/orderedlayout/Slot.java b/client/src/com/vaadin/client/ui/orderedlayout/Slot.java index efa19895a8..d9037cda3c 100644 --- a/client/src/com/vaadin/client/ui/orderedlayout/Slot.java +++ b/client/src/com/vaadin/client/ui/orderedlayout/Slot.java @@ -165,7 +165,7 @@ public final class Slot extends SimplePanel { } /** - * Attached resize listeners to the widget, caption and spacing elements + * Attaches resize listeners to the widget, caption and spacing elements */ private void attachListeners() { if (getWidget() != null && layout.getLayoutManager() != null) { @@ -204,6 +204,8 @@ public final class Slot extends SimplePanel { lm.removeElementResizeListener(getWidget().getElement(), widgetResizeListener); } + // in many cases, the listener has already been removed by + // setSpacing(false) if (getSpacingElement() != null && spacingResizeListener != null) { lm.removeElementResizeListener(getSpacingElement(), spacingResizeListener); @@ -352,6 +354,12 @@ public final class Slot extends SimplePanel { */ getElement().getParentElement().insertBefore(spacer, getElement()); } else if (!spacing && spacer != null) { + // Remove listener before spacer to avoid memory leak + LayoutManager lm = layout.getLayoutManager(); + if (lm != null && spacingResizeListener != null) { + lm.removeElementResizeListener(spacer, spacingResizeListener); + } + spacer.removeFromParent(); spacer = null; } diff --git a/uitest/src/com/vaadin/tests/components/orderedlayout/SpacingLeak.java b/uitest/src/com/vaadin/tests/components/orderedlayout/SpacingLeak.java new file mode 100644 index 0000000000..647c187568 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/orderedlayout/SpacingLeak.java @@ -0,0 +1,54 @@ +package com.vaadin.tests.components.orderedlayout; + +import com.vaadin.server.VaadinRequest; +import com.vaadin.ui.Button; +import com.vaadin.ui.HorizontalLayout; +import com.vaadin.ui.UI; +import com.vaadin.ui.VerticalLayout; + +/** + * HorizontalLayout and VerticalLayout should not leak .v-spacing elements via + * listeners when removing components from a layout. + * + * @since 7.1.12 + * @author Vaadin Ltd + */ +public class SpacingLeak extends UI { + + private HorizontalLayout spacingLayout; + + @Override + public void init(VaadinRequest req) { + final VerticalLayout root = new VerticalLayout(); + setContent(root); + root.setSizeUndefined(); + + final Button spacingButton = new Button("Add layout with spacing"); + spacingButton.setId("addbutton"); + root.addComponent(spacingButton); + spacingButton.addClickListener(new Button.ClickListener() { + @Override + public void buttonClick(Button.ClickEvent event) { + spacingLayout = new HorizontalLayout(); + spacingLayout.setSpacing(true); + spacingLayout.setWidth("100%"); + + for (int i = 0; i < 100; ++i) { + spacingLayout.addComponent(new Button("" + i)); + } + + root.addComponent(spacingLayout); + } + }); + + final Button removeButton = new Button("Remove layouts"); + removeButton.setId("removebutton"); + root.addComponent(removeButton); + removeButton.addClickListener(new Button.ClickListener() { + @Override + public void buttonClick(Button.ClickEvent event) { + root.removeComponent(spacingLayout); + } + }); + } +} diff --git a/uitest/src/com/vaadin/tests/components/orderedlayout/SpacingLeakTest.java b/uitest/src/com/vaadin/tests/components/orderedlayout/SpacingLeakTest.java new file mode 100644 index 0000000000..3a24cb7620 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/orderedlayout/SpacingLeakTest.java @@ -0,0 +1,39 @@ +/* + * 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.tests.components.orderedlayout; + +import org.junit.Test; +import org.openqa.selenium.By; + +import com.vaadin.tests.tb3.MultiBrowserTest; + +public class SpacingLeakTest extends MultiBrowserTest { + + @Test + public void testSpacingLeak() throws Exception { + setDebug(true); + openTestURL(); + getDriver().findElement(By.id("addbutton")).click(); + getDriver().findElement(By.xpath("//button[@title = 'Clear log']")) + .click(); + getDriver().findElement(By.id("removebutton")).click(); + + // this should be present + getDriver() + .findElement( + By.xpath("//span[text() = 'Measured 0 non connector elements']")); + } +} -- 2.39.5