Eliminate spacing memory leak on client (#13315)

Change-Id: I432ae4a04d74826dd1cce108177bd503774a8463

4 files changed, 107 insertions, 8 deletions

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']"));
+    }
+}