Fix making components visible by push (#9934)

Fix connector tracker cleanup for the case where a component is
hidden by a request and is made visible again by push.

This fixes a regression caused by #9305.

Fixes #9905

server/src/main/java/com/vaadin/server/VaadinSession.java

@@ -1028,7 +1028,7 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
                         }
                     }
                     try {
-                        ui.getConnectorTracker().cleanConnectorMap();
+                        ui.getConnectorTracker().cleanConnectorMap(false);
                     } catch (AssertionError | Exception e) {
                         getLogger().log(Level.SEVERE,
                                 "Exception while cleaning connector map for ui "

server/src/main/java/com/vaadin/server/communication/UidlWriter.java

@@ -321,6 +321,7 @@ public class UidlWriter implements Serializable {
             writePerformanceData(ui, writer);
         } finally {
             uiConnectorTracker.setWritingResponse(false);
+            uiConnectorTracker.cleanConnectorMap(true);
         }
     }
 

server/src/main/java/com/vaadin/ui/ConnectorTracker.java

@@ -71,7 +71,7 @@ public class ConnectorTracker implements Serializable {
 
     /**
      * Connectors that have been unregistered and should be cleaned up the next
-     * time {@link #cleanConnectorMap()} is invoked unless they have been
+     * time {@link #cleanConnectorMap(boolean)} is invoked unless they have been
      * registered again.
      */
     private final Set<ClientConnector> unregisteredConnectors = new HashSet<>();
@@ -262,14 +262,30 @@ public class ConnectorTracker implements Serializable {
         return null;
     }
 
+    /**
+     * Cleans the connector map from all connectors that are no longer attached
+     * to the application if there are dirty connectors or the force flag is
+     * true. This should only be called by the framework.
+     *
+     * @param force
+     *            {@code true} to force cleaning
+     * @since
+     */
+    public void cleanConnectorMap(boolean force) {
+        if (force || !dirtyConnectors.isEmpty()) {
+            cleanConnectorMap();
+        }
+    }
+
     /**
      * Cleans the connector map from all connectors that are no longer attached
      * to the application. This should only be called by the framework.
+     *
+     * @deprecated use {@link #cleanConnectorMap(boolean)} instead
      */
+    @Deprecated
     public void cleanConnectorMap() {
-        if (!unregisteredConnectors.isEmpty()) {
-            removeUnregisteredConnectors();
-        }
+        removeUnregisteredConnectors();
 
         cleanStreamVariables();
 

uitest/src/main/java/com/vaadin/tests/application/MissingHierarchyDetection.java

@@ -15,15 +15,7 @@
  */
 package com.vaadin.tests.application;
 
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
-import java.util.logging.Handler;
-import java.util.logging.LogRecord;
-import java.util.logging.Logger;
-
 import com.vaadin.server.VaadinRequest;
-import com.vaadin.server.VaadinSession;
 import com.vaadin.tests.components.AbstractReindeerTestUIWithLog;
 import com.vaadin.ui.Button;
 import com.vaadin.ui.Button.ClickEvent;
@@ -39,8 +31,6 @@ public class MissingHierarchyDetection extends AbstractReindeerTestUIWithLog {
 
     private CssLayout normalLayout = new CssLayout(
             new Label("Normal layout child"));
-    private List<LogRecord> pendingErrors = Collections
-            .synchronizedList(new ArrayList<>());
 
     public class BrokenCssLayout extends CssLayout
             implements SelectiveRenderer {
@@ -59,29 +49,6 @@ public class MissingHierarchyDetection extends AbstractReindeerTestUIWithLog {
 
     @Override
     protected void setup(VaadinRequest request) {
-        // Catch log messages so we can see if there is an error
-        Logger vaadinSessionLogger = Logger
-                .getLogger(VaadinSession.class.getName());
-        vaadinSessionLogger.addHandler(new Handler() {
-
-            @Override
-            public void publish(LogRecord record) {
-                if (record.getThrown() instanceof AssertionError) {
-                    pendingErrors.add(record);
-                    vaadinSessionLogger.removeHandler(this);
-                }
-            }
-
-            @Override
-            public void flush() {
-
-            }
-
-            @Override
-            public void close() throws SecurityException {
-
-            }
-        });
         addComponent(brokenLayout);
         addComponent(normalLayout);
         addComponent(new Button("Toggle properly", new Button.ClickListener() {
@@ -97,16 +64,6 @@ public class MissingHierarchyDetection extends AbstractReindeerTestUIWithLog {
                         toggle(false);
                     }
                 }));
-        addComponent(new Button("Check for errors", new Button.ClickListener() {
-            @Override
-            public void buttonClick(ClickEvent event) {
-                if (!pendingErrors.isEmpty()) {
-                    log(pendingErrors.remove(0).getThrown().getMessage());
-                } else {
-                    log("No errors");
-                }
-            }
-        }));
     }
 
     private void toggle(boolean properly) {

uitest/src/main/java/com/vaadin/tests/components/table/TableRemovedQuicklySendsInvalidRpcCalls.java

@@ -90,6 +90,11 @@ public class TableRemovedQuicklySendsInvalidRpcCalls
             return tracker.getConnector(connectorId);
         }
 
+        @Override
+        public void cleanConnectorMap(boolean force) {
+            tracker.cleanConnectorMap(force);
+        }
+
         @Override
         public void cleanConnectorMap() {
             tracker.cleanConnectorMap();

uitest/src/main/java/com/vaadin/tests/push/MakeComponentVisibleWithPush.java

@@ -0,0 +1,104 @@
+package com.vaadin.tests.push;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import com.vaadin.annotations.Push;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.UI;
+import com.vaadin.ui.VerticalLayout;
+
+@Push
+public class MakeComponentVisibleWithPush extends AbstractTestUI {
+
+    private VerticalLayout rootLayout;
+    private Grid<Person> grid;
+    private SearchThread searchThread;
+    private List<Person> data;
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        data = new ArrayList<>();
+
+        rootLayout = new VerticalLayout();
+        setContent(rootLayout);
+
+        grid = new Grid<Person>();
+        grid.addColumn(Person::getName);
+        grid.setVisible(false);
+        rootLayout.addComponent(grid);
+
+        Button doUpdateButton = new Button("Do Update", event -> {
+            try {
+                doUpdate();
+            } catch (InterruptedException e) {
+            }
+        });
+
+        rootLayout.addComponent(doUpdateButton);
+    }
+
+    private void doUpdate() throws InterruptedException {
+
+        cancelSuggestThread();
+
+        grid.setVisible(false);
+        grid.setItems(data);
+
+        UI ui = UI.getCurrent();
+        searchThread = new SearchThread(ui);
+        searchThread.start();
+
+    }
+
+    class SearchThread extends Thread {
+        private UI ui;
+
+        public SearchThread(UI ui) {
+            this.ui = ui;
+        }
+
+        @Override
+        public void run() {
+            data = new ArrayList<Person>(data);
+            data.add(new Person("Person " + (data.size() + 1)));
+
+            if (!searchThread.isInterrupted()) {
+                ui.access(() -> {
+                    grid.setItems(data);
+                    grid.setVisible(true);
+                });
+            }
+        }
+
+    }
+
+    private void cancelSuggestThread() {
+
+        if ((searchThread != null) && !searchThread.isInterrupted()) {
+            searchThread.interrupt();
+            searchThread = null;
+        }
+    }
+
+    class Person {
+
+        private String name;
+
+        public Person(String name) {
+            this.name = name;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public void setName(String name) {
+            this.name = name;
+        }
+    }
+
+}

uitest/src/test/java/com/vaadin/tests/application/MissingHierarchyDetectionTest.java

@@ -44,11 +44,6 @@ public class MissingHierarchyDetectionTest extends SingleBrowserTest {
         ButtonElement toggleImproperly = $(ButtonElement.class)
                 .caption("Toggle improperly").first();
         toggleImproperly.click();
-
-        $(ButtonElement.class).caption("Check for errors").first().click();
-        Assert.assertTrue(
-                "No error was logged for the missing hierarchy change event",
-                getLogRow(0).contains(
-                        "is no longer visible to the client, but no corresponding hierarchy change was sent."));
+        assertSystemNotification();
     }
 }

uitest/src/test/java/com/vaadin/tests/push/MakeComponentVisibleWithPushTest.java

@@ -0,0 +1,41 @@
+/*
+ * Copyright 2000-2016 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.push;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class MakeComponentVisibleWithPushTest extends SingleBrowserTest {
+
+    @Test
+    public void showingHiddenComponentByPushWorks() {
+        setDebug(true);
+        openTestURL();
+
+        $(ButtonElement.class).first().click();
+        Assert.assertEquals("Unexpected row count", 1,
+                $(GridElement.class).first().getRowCount());
+        $(ButtonElement.class).first().click();
+        Assert.assertEquals("Unexpected row count", 2,
+                $(GridElement.class).first().getRowCount());
+
+        assertNoErrorNotifications();
+    }
+}